Skip to content

Commit 5d84f62

Browse files
SatyamBansalmeta-codesync[bot]
authored andcommitted
fix(image): emit onLoadStart before subscribing the response observer on Fabric (#58314)
Summary: Fixes #54120. On the New Architecture on iOS, changing the `source` of an already-mounted `<Image>` delivers the load events out of order: ``` onLoad -> onLoadEnd -> onLoadStart // onLoadStart arrives LAST ``` `RCTImageComponentView.updateState:oldState:` subscribes the image response observer **before** it emits `onLoadStart()`. `ImageResponseObserverCoordinator::addObserver` does not queue a finished request — it replays it synchronously (`case Completed:` → `observer->didReceiveImage(...)`, and `case Failed:` → `didReceiveFailure(...)`). `RCTImageResponseObserverProxy` forwards through `RCTExecuteOnMainQueue`, which is `if (RCTIsMainQueue()) { block(); }` — inline, and mounting is on the main thread. So when a request has already completed by the time the mount transaction applies, `didReceiveImage:` emits `onLoad` + `onLoadEnd` from *inside* the subscribe call, and `onLoadStart` follows afterwards. This moves the `onLoadStart()` emission above the subscribe call, restoring the ordering the old architecture guarantees — `RCTImageView.reloadImage` emits `_onLoadStart` before it calls `loadImageWithURLRequest:`, so it cannot invert. Android is likewise unaffected: `ReactImageView` binds `onLoadStart` to Fresco's `onSubmit` and `onLoad`/`onLoadEnd` to `onFinalImageSet`, and submission always precedes delivery. The move is safe with respect to the guard condition: both `oldImageState` and `newImageState` are captured from `_state` / `state` **before** the subscribe call, so hoisting the emission above it does not change what the condition sees. ### Why it matters Any component that shows a spinner on `onLoadStart` and hides it on `onLoadEnd` is left with a permanently visible spinner over a fully decoded image — the event that turns it on arrives after the event that would turn it off. #54120 has reports of this from three separate users; the only workaround in the thread is a guessed `setTimeout`. ## Changelog: [IOS] [FIXED] - Emit `Image`'s `onLoadStart` before `onLoad`/`onLoadEnd` when the image request has already completed Pull Request resolved: #58314 Test Plan: Reproducer (community template, only `App.tsx` differs): https://github.com/SatyamBansal/rn-repro-image-onloadstart-order It mounts four `<Image>`s and records the order of `onLoadStart` / `onLoad` / `onLoadEnd` using a counter incremented **inside each callback** (so the log is the order events reached JS, not a render artifact), with `performance.now()` timestamps. Pressing **Swap E source** changes one mounted `<Image>`'s `source` between two `data:` URIs. Verified on an iPhone 17 Pro simulator, iOS 26.3, New Architecture, debug, `react-native` 0.87.1 built **from source** (`RCT_USE_PREBUILT_RNCORE=0`, so this file is actually compiled — with the default prebuilt core a local edit here has no effect). **Before** — swap E's source: ``` #1 912798714.91 onLoad E #2 912798715.06 onLoadEnd E #3 912798715.08 onLoadStart E ``` Reproducible on every press. All three events land within 0.2 ms, inside one mount transaction — not an asynchronous race. **After** this patch, same build, same gesture: ``` #1 913716961.86 onLoadStart E #2 913716961.92 onLoad E #3 913716961.95 onLoadEnd E ``` ``` #1 913727913.01 onLoadStart E #2 913727913.15 onLoad E #3 913727913.21 onLoadEnd E ``` First-mount ordering is unchanged (it was already correct — on a fresh mount the request is still `Loading` when the view subscribes, because `RCTImageManager.requestImage` dispatches the fetch to a background queue after returning). Not covered by this test plan, and not run: Android and the iOS old architecture. Neither goes through this file, and both are argued above from source rather than measured. Reviewed By: christophpurrer Differential Revision: D119093213 Pulled By: javache fbshipit-source-id: caa9f260252543dd0c11a171f304e332a552019a
1 parent 022458f commit 5d84f62

1 file changed

Lines changed: 7 additions & 3 deletions

File tree

packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,19 +94,23 @@ - (void)updateState:(const State::Shared &)state oldState:(const State::Shared &
9494
auto oldImageState = std::static_pointer_cast<const ImageShadowNode::ConcreteState>(_state);
9595
auto newImageState = std::static_pointer_cast<const ImageShadowNode::ConcreteState>(state);
9696

97-
[self _setStateAndResubscribeImageResponseObserver:newImageState];
98-
9997
bool havePreviousData = oldImageState && oldImageState->getData().getImageSource() != ImageSource{};
10098

10199
if (!havePreviousData ||
102100
(newImageState && newImageState->getData().getImageSource() != oldImageState->getData().getImageSource())) {
103101
// Loading actually starts a little before this, but this is the first time we know
104-
// the image is loading and can fire an event from this component
102+
// the image is loading and can fire an event from this component.
103+
//
104+
// This has to be emitted before subscribing below: the observer coordinator
105+
// replays an already-`Completed` (or `Failed`) response synchronously, so
106+
// subscribing first can deliver `onLoad`/`onLoadEnd` ahead of `onLoadStart`.
105107
static_cast<const ImageEventEmitter &>(*_eventEmitter).onLoadStart();
106108

107109
// TODO (T58941612): Tracking for visibility should be done directly on this class.
108110
// For now, we consolidate instrumentation logic in the image loader, so that pre-Fabric gets the same treatment.
109111
}
112+
113+
[self _setStateAndResubscribeImageResponseObserver:newImageState];
110114
}
111115

112116
- (void)_setStateAndResubscribeImageResponseObserver:(const ImageShadowNode::ConcreteState::Shared &)state

0 commit comments

Comments
 (0)