From 0ec5c569eb3911750776bd2685e7e3b8fbd0a7d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Du=C5=BCy?= <91994767+alduzy@users.noreply.github.com> Date: Fri, 15 Nov 2024 17:16:33 +0100 Subject: [PATCH] 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 https://github.com/software-mansion/react-native-screens/pull/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 --- .../java/com/swmansion/rnscreens/Screen.kt | 14 +++-- .../com/swmansion/rnscreens/ext/ViewExt.kt | 24 -------- apps/src/tests/Test2282.tsx | 61 ++++++++++++++++++- 3 files changed, 67 insertions(+), 32 deletions(-) diff --git a/android/src/main/java/com/swmansion/rnscreens/Screen.kt b/android/src/main/java/com/swmansion/rnscreens/Screen.kt index a2259e330f..df99973234 100644 --- 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 index f48561da18..f9258e1ed1 100644 --- 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 index 587334e580..f41ba4b008 100644 --- 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 ( - + + + + ); } +function ListScreenSimplified({secondVisible}: {secondVisible?: (visible: boolean) => void}) { + const containerRef = React.useRef(null); + const innerViewRef = React.useRef(null); + const childViewRef = React.useRef(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 ( + + + + {secondVisible && (