refactor(LayoutAnimations): stop taking over UIManagerAnimationDelegate#9901
Draft
bartlomiejbloniarz wants to merge 2 commits into
Draft
refactor(LayoutAnimations): stop taking over UIManagerAnimationDelegate#9901bartlomiejbloniarz wants to merge 2 commits into
bartlomiejbloniarz wants to merge 2 commits into
Conversation
0c21666 to
cfd6053
Compare
Reanimated registered LayoutAnimationsProxy_Legacy as the UIManagerAnimationDelegate only to receive stopSurface - the other three delegate methods were no-ops. Occupying the delegate slot overwrites the LayoutAnimationDriver that React Native installs there, which silently disables LayoutAnimation.configureNext for the whole app. The proxy now detects surface teardown itself: it registers a commit hook and marks a surface in surfacesToRemove_ whenever a commit's root has no children, which is what SurfaceHandler::stop's commitEmptyTree produces. The mark lands before the teardown transaction reaches pullTransaction, so the consumer side is unchanged. App-level empty renders don't trigger this - rendering null from the app still leaves the AppContainer view under the root, so only genuine teardown (and display-mode suspend) commits an empty root. The teardown check deliberately avoids reading the ShadowTreeRegistry: pullTransaction and commit hooks can run under the registry's shared lock, and re-locking it there can deadlock with a pending writer (#8579 is that exact bug in another spot). The delegate slot stays with React Native, so configureNext works next to Reanimated again, with both mounting override delegates chained by the MountingCoordinator. uiManager moves from the Android-only constructor parameters of the layout animation proxies to a shared one, since the commit hook registration needs it on both platforms.
cfd6053 to
fb6c63f
Compare
…me transaction With the animation delegate freed, RN's LayoutAnimationDriver runs next to the reanimated override delegate. For configureNext delete animations the driver withholds Remove/Delete mutations and replays them after the animation, in the same pull as the final keyframe update for those views. React's differ never pairs an update with a delete for one tag, but the driver does - and since we emit removals before updates, the update reached the mounting layer after its view was already unregistered, aborting with RCTComponentViewRegistry: Attempt to query unregistered component (reproduced in the Bluesky app on any configureNext that removes views). Updates targeting views deleted in the same transaction are dropped - the views are gone this frame either way.
d244f99 to
e999165
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
We register
LayoutAnimationsProxy_Legacyas theUIManagerAnimationDelegateonly to receivestopSurface— the other three delegate methods are no-ops. Occupying that slot overwrites theLayoutAnimationDriverthat RN installs there, which silently killsLayoutAnimation.configureNextfor the whole app.The proxy now detects teardown itself: it registers a commit hook and marks a surface in
surfacesToRemove_whenever a commit's root has no children — which is whatSurfaceHandler::stop→commitEmptyTreeproduces, and lands before that transaction reachespullTransaction. The pull consumes the mark only on a transaction that actually removes children of the root, so pulls emitted for animation frames (which run with empty mutations) can't consume it early. App-levelnullrenders don't trigger the mark (the AppContainer view stays under the root), so only real teardown and display-mode suspend commit an empty root — verified empirically and by enumerating everycommitEmptyTreecaller.The check deliberately doesn't read the
ShadowTreeRegistry:pullTransaction/commit hooks can run under the registry's shared lock, and re-locking there deadlocks with a pending writer (#8579 is that exact bug elsewhere).Freeing the slot restores
configureNextto stock RN behavior per platform: it animates on iOS (enableLayoutAnimationsOnIOSdefaults to true) and on Android when the app enablesenableLayoutAnimationsOnAndroid(off by default in RN core — without itconfigureNextsnaps in stock RN too). TheMountingCoordinatorchains RN's driver and our override delegate; the driver's ownstopSurfaceflushes its withheld removals into the teardown transaction, so the consume condition always holds there.uiManagermoves from the Android-only proxy constructor args to a shared one; the commit hook registration needs it on both platforms.Test plan
fabric-example with
ENABLE_SHARED_ELEMENT_TRANSITIONS=false(legacy proxy), iOS sim + Android emulator (withenableLayoutAnimationsOnAndroidforce-enabled on Android):configureNextanimates: video frame measurements show full easeInEaseOut interpolation on both platforms (iOS: 119 frames, 180→720px; Android: 158→275→388→468→514→630px)configureNextdelete animations (transitions that remove views): the driver replays withheld Remove/Delete together with a final keyframe update; our reordering used to mount that update after the delete and abort (found in the Bluesky app, deterministic). Fixed in the second commit; covered by the bench's remove-views button on both platformsconfigureNextmid-flight at teardown — 11/11 teardowns skipped exits with zero false positives on live surfaces, no crashes/ANRs/ghost viewsnullat the app root: exits animate normally (surface alive), UI recoversManual test bench:
remove-animation-delegate-testbenchbranch (flipENABLE_SHARED_ELEMENT_TRANSITIONSto false in apps/fabric-example/package.json locally, then build).