fix(Android): going back on fabric with removeClippedSubviews (#2495)

## Description This PR intents to fix going back on fabric issue when using a `View` with [removeClippedSubviews](https://reactnative.dev/docs/view#removeclippedsubviews) prop set to true. Previous bug fixes addressed this issue primarily for FlatLists, where it's set to true by default on Android. See #2383. Additionally, this PR greatly improves the performance of `startTransitionRecursive` as it does not climb up the tree in search for a parent with `removeClippedSubview` set to true anymore. Fixes #2491 . ## Changes - removed redundant code - updated `Test2282.tsx` repro  ## Test code and steps to reproduce - use `Test2282.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <[email protected]>
software-mansion · Nov 15, 2024 · 0ec5c56 · 0ec5c56
1 parent 40f318a
commit 0ec5c56
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 32 deletions.
diff --git a/android/src/main/java/com/swmansion/rnscreens/Screen.kt b/android/src/main/java/com/swmansion/rnscreens/Screen.kt
@@ -20,13 +20,14 @@ import com.facebook.react.uimanager.PixelUtil
 import com.facebook.react.uimanager.UIManagerHelper
 import com.facebook.react.uimanager.UIManagerModule
 import com.facebook.react.uimanager.events.EventDispatcher
+import com.facebook.react.views.scroll.ReactHorizontalScrollView
+import com.facebook.react.views.scroll.ReactScrollView
 import com.google.android.material.bottomsheet.BottomSheetBehavior
 import com.google.android.material.shape.CornerFamily
 import com.google.android.material.shape.MaterialShapeDrawable
 import com.google.android.material.shape.ShapeAppearanceModel
 import com.swmansion.rnscreens.events.HeaderHeightChangeEvent
 import com.swmansion.rnscreens.events.SheetDetentChangedEvent
-import com.swmansion.rnscreens.ext.isInsideScrollViewWithRemoveClippedSubviews
 import java.lang.ref.WeakReference
 
 @SuppressLint("ViewConstructor") // Only we construct this view, it is never inflated.
@@ -399,11 +400,12 @@ class Screen(
                     startTransitionRecursive(child.toolbar)
                 }
                 if (child is ViewGroup) {
-                    // The children are miscounted when there's a FlatList with
-                    // removeClippedSubviews set to true (default).
-                    // We add a simple view for each item in the list to make it work as expected.
-                    // See https://github.com/software-mansion/react-native-screens/pull/2383
-                    if (child.isInsideScrollViewWithRemoveClippedSubviews()) {
+                    // The children are miscounted when there's removeClippedSubviews prop
+                    // set to true (which is the default for FlatLists).
+                    // Unless the child is a ScrollView it's safe to assume that it's true
+                    // and add a simple view for each possibly clipped item to make it work as expected.
+                    // See https://github.com/software-mansion/react-native-screens/pull/2495
+                    if (child !is ReactScrollView && child !is ReactHorizontalScrollView) {
                         for (j in 0 until child.childCount) {
                             child.addView(View(context))
                         }

diff --git a/android/src/main/java/com/swmansion/rnscreens/ext/ViewExt.kt b/android/src/main/java/com/swmansion/rnscreens/ext/ViewExt.kt
@@ -3,9 +3,6 @@ package com.swmansion.rnscreens.ext
 import android.graphics.drawable.ColorDrawable
 import android.view.View
 import android.view.ViewGroup
-import com.facebook.react.views.scroll.ReactHorizontalScrollView
-import com.facebook.react.views.scroll.ReactScrollView
-import com.swmansion.rnscreens.ScreenStack
 
 internal fun View.parentAsView() = this.parent as? View
 
@@ -33,24 +30,3 @@ internal fun View.maybeBgColor(): Int? {
     }
     return null
 }
-
-internal fun View.isInsideScrollViewWithRemoveClippedSubviews(): Boolean {
-    if (this is ReactHorizontalScrollView || this is ReactScrollView) {
-        return false
-    }
-    var parentView = this.parent
-    while (parentView is ViewGroup && parentView !is ScreenStack) {
-        when (parentView) {
-            is ReactHorizontalScrollView -> {
-                return parentView.removeClippedSubviews
-            }
-            is ReactScrollView -> {
-                return parentView.removeClippedSubviews
-            }
-            else -> {
-                parentView = parentView.parent
-            }
-        }
-    }
-    return false
-}
diff --git a/apps/src/tests/Test2282.tsx b/apps/src/tests/Test2282.tsx
@@ -11,6 +11,7 @@ import {
   ViewProps,
   Image,
   FlatListProps,
+  findNodeHandle,
 } from 'react-native';
 
 enableScreens(true);
@@ -34,13 +35,52 @@ function Home({ navigation }: any) {
 
 function ListScreen() {
   return (
-    <View style={{ flex: 1, backgroundColor: 'slateblue' }}>
+    <View
+      style={{ flex: 1, backgroundColor: 'slateblue' }}
+      removeClippedSubviews>
       <ParentFlatlist />
+      <View removeClippedSubviews>
+        <View style={{ backgroundColor: 'pink', width: '100%', height: 50 }} />
+      </View>
       <ParentFlatlist horizontal />
     </View>
   );
 }
 
+function ListScreenSimplified({secondVisible}: {secondVisible?: (visible: boolean) => void}) {
+  const containerRef = React.useRef<View>(null);
+  const innerViewRef = React.useRef<View>(null);
+  const childViewRef = React.useRef<View>(null);
+
+  React.useEffect(() => {
+    if (containerRef.current != null) {
+      const tag = findNodeHandle(containerRef.current);
+      console.log(`Container has tag [${tag}]`);
+    }
+    if (innerViewRef.current != null) {
+      const tag = findNodeHandle(innerViewRef.current);
+      console.log(`InnerView has tag [${tag}]`);
+    }
+    if (childViewRef.current != null) {
+      const tag = findNodeHandle(childViewRef.current);
+      console.log(`ChildView has tag [${tag}]`);
+    }
+  }, [containerRef.current, innerViewRef.current, childViewRef.current]);
+
+  return (
+    <View
+      ref={containerRef}
+      style={{ flex: 1, backgroundColor: 'slateblue', overflow: 'hidden' }}
+      removeClippedSubviews={false}>
+      <View ref={innerViewRef} removeClippedSubviews style={{ height: '100%' }}>
+        <View ref={childViewRef} style={{ backgroundColor: 'pink', width: '100%', height: 50 }} removeClippedSubviews={false}>
+          {secondVisible && (<Button title='Hide second' onPress={() => secondVisible(false)} />)}
+        </View>
+      </View>
+    </View>
+  );
+}
+
 function ParentFlatlist(props: Partial<FlatListProps<number>>) {
   return (
     <FlatList
@@ -100,12 +140,29 @@ export default function App(): React.JSX.Element {
     <NavigationContainer>
       <Stack.Navigator screenOptions={{ animation: 'slide_from_right' }}>
         <Stack.Screen name="Home" component={Home} />
-        <Stack.Screen name="List" component={ListScreen} />
+        <Stack.Screen name="List" component={ListScreenSimplified}/>
       </Stack.Navigator>
     </NavigationContainer>
   );
 }
 
+export function AppSimple(): React.JSX.Element {
+  const [secondVisible, setSecondVisible] = React.useState(false);
+
+  return (
+    <View style={{ flex: 1, backgroundColor: 'lightsalmon' }}>
+      {!secondVisible && (
+        <View style={{ flex: 1, backgroundColor: 'lightblue' }} >
+          <Button title='Show second' onPress={() => setSecondVisible(true)} />
+        </View>
+      )}
+      {secondVisible && (
+        <ListScreenSimplified secondVisible={setSecondVisible} />
+      )}
+    </View>
+  );
+}
+
 const styles = StyleSheet.create({
   container: {
     flex: 1,