From 419f6146f01acdb14dcfcc0598b806cef3a60cf4 Mon Sep 17 00:00:00 2001 From: Oskar Pawica Date: Mon, 2 Nov 2026 21:09:58 +0100 Subject: [PATCH 1/3] Release view transition holds of removed views in addViewAt A parent that holds a child with `ViewGroup.startViewTransition` keeps `child.parent` after the child leaves its child list. When Fabric then inserts that view into another parent, `addViewAt` sees a stale parent. Its recovery calls `removeView`, which does nothing for a view that is no longer listed, so `addView` throws "failed to insert view". The recovery also marks the tag as re-added, so a later Remove of that tag is skipped. React Native Screens holds the content of a removed screen this way. Reanimated shared element transitions move the source view out of such a screen into the surface root, and crash on Android when going back. Before the recovery, end the view transition when the old parent no longer lists the view, then read the parent again. If it is cleared, insert the view normally. If not, keep the existing recovery. Holds of removed views that are not inserted again stay in place. --- .../fabric/mounting/SurfaceMountingManager.kt | 12 + ...urfaceMountingManagerViewTransitionTest.kt | 232 ++++++++++++++++++ 2 files changed, 244 insertions(+) create mode 100644 packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt index 0c8c7bf05502..9ea07feb1093 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt @@ -335,6 +335,7 @@ internal constructor( logViewHierarchy(parentView, false) } + endViewTransitionIfRemovedFromParent(view) val currParentView = view.parent if (currParentView != null) { val actualParentId = if (currParentView is ViewGroup) currParentView.id else View.NO_ID @@ -395,6 +396,17 @@ internal constructor( } } + /** + * Ends a [ViewGroup.startViewTransition] hold that keeps [view] attached to a parent which no + * longer lists it as a child. + */ + private fun endViewTransitionIfRemovedFromParent(view: View) { + val parent = view.parent + if (parent is ViewGroup && parent.indexOfChild(view) == -1) { + parent.endViewTransition(view) + } + } + @UiThread public fun removeViewAt(tag: Int, parentTag: Int, index: Int) { UiThreadUtil.assertOnUiThread() diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt new file mode 100644 index 000000000000..299c9e15e938 --- /dev/null +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt @@ -0,0 +1,232 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +@file:Suppress("DEPRECATION") + +package com.facebook.react.fabric + +import android.app.Activity +import android.view.View +import android.view.ViewGroup +import com.facebook.react.ReactRootView +import com.facebook.react.bridge.JavaOnlyMap +import com.facebook.react.bridge.ReactTestHelper +import com.facebook.react.fabric.mounting.MountingManager +import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor +import com.facebook.react.fabric.mounting.SurfaceMountingManager +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests +import com.facebook.react.uimanager.ThemedReactContext +import com.facebook.react.uimanager.ViewManager +import com.facebook.react.uimanager.ViewManagerRegistry +import com.facebook.react.views.view.ReactViewManager +import com.facebook.testutils.shadows.ShadowSoLoader +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Tests for [SurfaceMountingManager.addViewAt] with views that a parent holds with + * [ViewGroup.startViewTransition] after their removal, as React Native Screens does for the content + * of a removed screen. + */ +@RunWith(RobolectricTestRunner::class) +@Config(shadows = [ShadowSoLoader::class]) +class SurfaceMountingManagerViewTransitionTest { + private lateinit var mountingManager: MountingManager + private lateinit var themedReactContext: ThemedReactContext + private val surfaceId = 1 + + @Before + fun setUp() { + ReactNativeFeatureFlagsForTests.setUp() + val reactContext = ReactTestHelper.createTestReactApplicationContext() + themedReactContext = ThemedReactContext(reactContext, reactContext, null, -1) + mountingManager = + MountingManager( + ViewManagerRegistry(listOf>(ReactViewManager())), + MountItemExecutor {}, + ) + } + + @Test + fun addViewAt_movesHeldChildAndAllowsItsFinalRemoval() { + val smm = startSurface() + smm.createViews(SCREEN, CARRIER) + smm.addViewAt(surfaceId, SCREEN, 0) + smm.addViewAt(SCREEN, CARRIER, 0) + val screen = smm.viewGroup(SCREEN) + val carrier = smm.getView(CARRIER) + + screen.startViewTransition(carrier) + smm.removeViewAt(CARRIER, SCREEN, 0) + assertThat(carrier.parent).isSameAs(screen) + + smm.addViewAt(surfaceId, CARRIER, 1) + assertThat(carrier.parent).isSameAs(smm.viewGroup(surfaceId)) + + smm.removeViewAt(CARRIER, surfaceId, 1) + assertThat(carrier.parent).isNull() + assertThat(smm.viewGroup(surfaceId).indexOfChild(carrier)).isEqualTo(-1) + } + + @Test + fun addViewAt_movesNestedHeldChildrenAndAllowsTheirFinalRemoval() { + val smm = startSurface() + smm.createViews(SCREEN, CONTENT, CARRIER, INNER_CARRIER) + smm.addViewAt(surfaceId, SCREEN, 0) + smm.addViewAt(SCREEN, CONTENT, 0) + smm.addViewAt(CONTENT, CARRIER, 0) + smm.addViewAt(CARRIER, INNER_CARRIER, 0) + val root = smm.viewGroup(surfaceId) + val carrier = smm.getView(CARRIER) + val innerCarrier = smm.getView(INNER_CARRIER) + + repeat(2) { + smm.viewGroup(SCREEN).startViewTransition(smm.getView(CONTENT)) + smm.viewGroup(CONTENT).startViewTransition(carrier) + smm.viewGroup(CARRIER).startViewTransition(innerCarrier) + } + smm.removeViewAt(INNER_CARRIER, CARRIER, 0) + smm.removeViewAt(CARRIER, CONTENT, 0) + assertThat(carrier.parent).isSameAs(smm.viewGroup(CONTENT)) + assertThat(innerCarrier.parent).isSameAs(carrier) + + smm.addViewAt(surfaceId, CARRIER, 1) + smm.addViewAt(surfaceId, INNER_CARRIER, 2) + assertThat(carrier.parent).isSameAs(root) + assertThat(innerCarrier.parent).isSameAs(root) + + smm.removeViewAt(INNER_CARRIER, surfaceId, 2) + smm.removeViewAt(CARRIER, surfaceId, 1) + assertThat(innerCarrier.parent).isNull() + assertThat(carrier.parent).isNull() + assertThat(root.childCount).isEqualTo(1) + } + + @Test + fun addViewAt_movesHeldChildOfRootAttachedToWindow() { + val smm = startSurface() + attachToWindow(smm.getView(surfaceId)) + smm.createViews(SCREEN, CARRIER) + smm.addViewAt(surfaceId, SCREEN, 0) + smm.addViewAt(SCREEN, CARRIER, 0) + val carrier = smm.getView(CARRIER) + + smm.viewGroup(SCREEN).startViewTransition(carrier) + smm.removeViewAt(CARRIER, SCREEN, 0) + assertThat(carrier.isAttachedToWindow).isTrue() + + smm.addViewAt(surfaceId, CARRIER, 1) + assertThat(carrier.parent).isSameAs(smm.viewGroup(surfaceId)) + assertThat(carrier.isAttachedToWindow).isTrue() + + smm.removeViewAt(CARRIER, surfaceId, 1) + assertThat(carrier.parent).isNull() + assertThat(carrier.isAttachedToWindow).isFalse() + } + + @Test + fun addViewAt_reinsertsHeldChildIntoItsOldParent() { + val smm = startSurface() + smm.createViews(SCREEN, CONTENT, CARRIER) + smm.addViewAt(surfaceId, SCREEN, 0) + smm.addViewAt(SCREEN, CONTENT, 0) + smm.addViewAt(SCREEN, CARRIER, 1) + val screen = smm.viewGroup(SCREEN) + val carrier = smm.getView(CARRIER) + + screen.startViewTransition(carrier) + smm.removeViewAt(CARRIER, SCREEN, 1) + smm.addViewAt(SCREEN, CARRIER, 0) + assertThat(screen.indexOfChild(carrier)).isEqualTo(0) + + smm.removeViewAt(CARRIER, SCREEN, 0) + assertThat(carrier.parent).isNull() + } + + @Test + fun addViewAt_keepsHoldOfRemovedChildThatIsNotReinserted() { + val smm = startSurface() + smm.createViews(SCREEN, CONTENT, NEXT_SCREEN) + smm.addViewAt(surfaceId, SCREEN, 0) + smm.addViewAt(SCREEN, CONTENT, 0) + val screen = smm.viewGroup(SCREEN) + val content = smm.getView(CONTENT) + + screen.startViewTransition(content) + smm.removeViewAt(CONTENT, SCREEN, 0) + smm.addViewAt(surfaceId, NEXT_SCREEN, 1) + + assertThat(content.parent).isSameAs(screen) + assertThat(screen.indexOfChild(content)).isEqualTo(-1) + } + + @Test + fun addViewAt_movesChildThatIsNotHeld() { + val smm = startSurface() + smm.createViews(SCREEN, NEXT_SCREEN, CARRIER) + smm.addViewAt(surfaceId, SCREEN, 0) + smm.addViewAt(surfaceId, NEXT_SCREEN, 1) + smm.addViewAt(SCREEN, CARRIER, 0) + val carrier = smm.getView(CARRIER) + + smm.removeViewAt(CARRIER, SCREEN, 0) + smm.addViewAt(NEXT_SCREEN, CARRIER, 0) + assertThat(carrier.parent).isSameAs(smm.viewGroup(NEXT_SCREEN)) + + smm.removeViewAt(CARRIER, NEXT_SCREEN, 0) + assertThat(carrier.parent).isNull() + } + + @Test + fun addViewAt_keepsRecoveryForChildThatAnotherParentStillLists() { + val smm = startSurface() + smm.createViews(SCREEN, NEXT_SCREEN, CARRIER) + smm.addViewAt(surfaceId, SCREEN, 0) + smm.addViewAt(surfaceId, NEXT_SCREEN, 1) + smm.addViewAt(SCREEN, CARRIER, 0) + val nextScreen = smm.viewGroup(NEXT_SCREEN) + val carrier = smm.getView(CARRIER) + + smm.addViewAt(NEXT_SCREEN, CARRIER, 0) + assertThat(carrier.parent).isSameAs(nextScreen) + assertThat(smm.viewGroup(SCREEN).indexOfChild(carrier)).isEqualTo(-1) + + smm.removeViewAt(CARRIER, NEXT_SCREEN, 0) + assertThat(carrier.parent).isSameAs(nextScreen) + } + + private fun startSurface(): SurfaceMountingManager { + val rootView = ReactRootView(themedReactContext) + mountingManager.startSurface(surfaceId, themedReactContext, rootView) + return mountingManager.getSurfaceManagerEnforced(surfaceId, "test") + } + + private fun attachToWindow(view: View) { + Robolectric.buildActivity(Activity::class.java).setup().get().setContentView(view) + } + + private fun SurfaceMountingManager.createViews(vararg tags: Int) { + for (tag in tags) { + createView("RCTView", tag, JavaOnlyMap.of(), null, null, true) + } + } + + private fun SurfaceMountingManager.viewGroup(tag: Int): ViewGroup = getView(tag) as ViewGroup + + private companion object { + const val SCREEN = 10 + const val CONTENT = 11 + const val CARRIER = 12 + const val INNER_CARRIER = 13 + const val NEXT_SCREEN = 14 + } +} From d0d8519d286fef4f99184bdb1c5c3cf4761332da Mon Sep 17 00:00:00 2001 From: Oskar Pawica Date: Tue, 3 Nov 2026 10:30:52 +0100 Subject: [PATCH 2/3] Remove library reference from SurfaceMountingManagerViewTransitionTest doc --- .../react/fabric/SurfaceMountingManagerViewTransitionTest.kt | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt index 299c9e15e938..985f7db4be39 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerViewTransitionTest.kt @@ -33,9 +33,8 @@ import org.robolectric.RobolectricTestRunner import org.robolectric.annotation.Config /** - * Tests for [SurfaceMountingManager.addViewAt] with views that a parent holds with - * [ViewGroup.startViewTransition] after their removal, as React Native Screens does for the content - * of a removed screen. + * Tests for [SurfaceMountingManager.addViewAt] with views that a parent still holds with + * [ViewGroup.startViewTransition] after their removal. */ @RunWith(RobolectricTestRunner::class) @Config(shadows = [ShadowSoLoader::class]) From d7addde14f97a195e2bc00158d1bdb47b81cc84b Mon Sep 17 00:00:00 2001 From: Oskar Pawica Date: Tue, 3 Nov 2026 11:22:16 +0100 Subject: [PATCH 3/3] Inline the view transition release in addViewAt --- .../fabric/mounting/SurfaceMountingManager.kt | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt index 9ea07feb1093..8830c63a4e40 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt @@ -335,8 +335,11 @@ internal constructor( logViewHierarchy(parentView, false) } - endViewTransitionIfRemovedFromParent(view) - val currParentView = view.parent + var currParentView = view.parent + if (currParentView is ViewGroup && currParentView.indexOfChild(view) == -1) { + currParentView.endViewTransition(view) + currParentView = view.parent + } if (currParentView != null) { val actualParentId = if (currParentView is ViewGroup) currParentView.id else View.NO_ID ReactSoftExceptionLogger.logSoftException( @@ -396,17 +399,6 @@ internal constructor( } } - /** - * Ends a [ViewGroup.startViewTransition] hold that keeps [view] attached to a parent which no - * longer lists it as a child. - */ - private fun endViewTransitionIfRemovedFromParent(view: View) { - val parent = view.parent - if (parent is ViewGroup && parent.indexOfChild(view) == -1) { - parent.endViewTransition(view) - } - } - @UiThread public fun removeViewAt(tag: Int, parentTag: Int, index: Int) { UiThreadUtil.assertOnUiThread()