fix(Android): going back on fabric with nested list #2383

alduzy · 2024-10-03T14:19:38Z

Description

This PR intents to fix crash happening on Android (fabric) when navigating back from a screen with nested list.

Before

The crash happens whenever the nested list is visible on the screen, as shown below:

✅	❌	❌	❌	✅

After

Screen.Recording.2024-10-08.at.13.58.22.mov

Nested lists are rendered as ViewGroups, thus the check for ReactScrollView fails. This PR fixes the issue by checking wether the view is an indirect child of a ScrollView with removeClippedSubviews enabled.

Fixes: #2341

Changes

added isInsideScrollViewWithRemoveClippedSubviews check to startTransitionRecursive
modified Test2282.tsx repro

Test code and steps to reproduce

use Test2282.tsx repro

Checklist

Included code example that can be used to test this change
Ensured that CI passes

…back-with-nested-list

WoLewicki

This logic keeps on growing and I am not happy about it at all. But until we find better way of handling it, I think we have to try and fix those edge cases.

kkafar

I really don't like the idea of iterating up the whole tree for each child for each transition, but I guess that is better than crashing 🤷🏻‍♂️

We should add a comment above this function that it serves purposes of a workaround, refer to the startTransitionRecursive function and link this PR.

Also: do we actually know what changed that this started crashing (maybe in react-native)? Because it looks like the root cause of the issue is on react-native side. The issue reporter of #2341 noted that this happens since 0.75 onwards. What has changed? @WoLewicki do you have some context?

kkafar · 2024-10-08T12:51:49Z

android/src/main/java/com/swmansion/rnscreens/Screen.kt

+            return false
+        }
+        var parentView = child.parent
+        while (parentView is ViewGroup && parentView !is ScreenStack) {


It might be enough to check for Screen, right? Screen will be direct descendant of ScreenStack and we don't want to look further up. Am I right here?

Suggested change

while (parentView is ViewGroup && parentView !is ScreenStack) {

while (parentView is ViewGroup && parentView !is Screen) {

The loop actually finishes quicker with this check for ScreenStack. It's because the ActionMenuView goes through the loop as well and there's no Screen between it and the ScreenStack. I didn't want to add yet another check for that.

android/src/main/java/com/swmansion/rnscreens/Screen.kt

WoLewicki · 2024-10-08T13:33:05Z

What has changed? @WoLewicki do you have some context?

Nope, but would be nice to find out what changed and maybe iterate on it to handle it in core instead.

kkafar

I think we're good. Thank you!

…#2383) ## Description This PR intents to fix crash happening on Android (fabric) when navigating back from a screen with nested list. ### Before The crash happens whenever the nested list is visible on the screen, as shown below: | ✅ | ❌ | ❌ | ❌ | ✅ | |----------|----------|----------|----------|----------| | <img width="247" alt="Screenshot 2024-10-03 at 16 12 16" src="https://github.com/user-attachments/assets/55f36653-9451-48a1-b4fc-9d419622fe2b"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 25" src="https://github.com/user-attachments/assets/5cc1af46-4418-40de-9391-6dc5168e08af"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 34" src="https://github.com/user-attachments/assets/d2ec614a-fbeb-4717-8aeb-328e3a890cd6"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 40" src="https://github.com/user-attachments/assets/5d634fbd-45c9-4612-9dcf-25cf8b9295a0"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 47" src="https://github.com/user-attachments/assets/0881dd80-ff98-4ba2-992e-933d549eed19"> | ### After https://github.com/user-attachments/assets/fb36d030-ef53-493d-a8cc-38be670ee504 Nested lists are rendered as ViewGroups, thus the check for ReactScrollView fails. This PR fixes the issue by checking wether the view is an indirect child of a ScrollView with [removeClippedSubviews](https://reactnative.dev/docs/optimizing-flatlist-configuration#removeclippedsubviews) enabled. Fixes: software-mansion#2341 ## Changes - added `isInsideScrollViewWithRemoveClippedSubviews` check to `startTransitionRecursive` - modified `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

## Description This PR handles yet another case for going back on fabric. In the previous one (#2383) I omitted a case where a horizontal list is simply rendered w/o being nested in any other list! Fixes #2341 ## Changes - updated `Test2292.tsx` repro - handle `parentView is ReactHorizontalScrollView`  ## 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

This PR intents to fix crash happening on Android (fabric) when navigating back from a screen with nested list. The crash happens whenever the nested list is visible on the screen, as shown below: | ✅ | ❌ | ❌ | ❌ | ✅ | |----------|----------|----------|----------|----------| | <img width="247" alt="Screenshot 2024-10-03 at 16 12 16" src="https://github.com/user-attachments/assets/55f36653-9451-48a1-b4fc-9d419622fe2b"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 25" src="https://github.com/user-attachments/assets/5cc1af46-4418-40de-9391-6dc5168e08af"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 34" src="https://github.com/user-attachments/assets/d2ec614a-fbeb-4717-8aeb-328e3a890cd6"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 40" src="https://github.com/user-attachments/assets/5d634fbd-45c9-4612-9dcf-25cf8b9295a0"> | <img width="247" alt="Screenshot 2024-10-03 at 16 12 47" src="https://github.com/user-attachments/assets/0881dd80-ff98-4ba2-992e-933d549eed19"> | https://github.com/user-attachments/assets/fb36d030-ef53-493d-a8cc-38be670ee504 Nested lists are rendered as ViewGroups, thus the check for ReactScrollView fails. This PR fixes the issue by checking wether the view is an indirect child of a ScrollView with [removeClippedSubviews](https://reactnative.dev/docs/optimizing-flatlist-configuration#removeclippedsubviews) enabled. Fixes: #2341 - added `isInsideScrollViewWithRemoveClippedSubviews` check to `startTransitionRecursive` - modified `Test2282.tsx` repro  - use `Test2282.tsx` repro - [x] Included code example that can be used to test this change - [x] Ensured that CI passes (cherry picked from commit b67af86)

## Description This PR handles yet another case for going back on fabric. In the previous one (#2383) I omitted a case where a horizontal list is simply rendered w/o being nested in any other list! Fixes #2341 ## Changes - updated `Test2292.tsx` repro - handle `parentView is ReactHorizontalScrollView`  ## 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 (cherry picked from commit d40e108)

## 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]>

alduzy added 7 commits October 3, 2024 16:09

added repro

1d94cc0

Merge remote-tracking branch 'origin/main' into @alduzy/fabric-going-…

d0749f4

…back-with-nested-list

updated test 2282 and removed 2341

f62206e

checking for indirect scrollview parent

27ab2f8

updated repro

ae2f417

optimised check

b47f354

handling horizontal scrollviews

40a58ce

alduzy requested review from maciekstosio, WoLewicki and kkafar October 8, 2024 12:06

alduzy marked this pull request as ready for review October 8, 2024 12:06

typo fix

19b97d8

alduzy changed the title ~~[WIP] fix(Android): going back on fabric with nested list~~ fix(Android): going back on fabric with nested list Oct 8, 2024

WoLewicki approved these changes Oct 8, 2024

View reviewed changes

kkafar requested changes Oct 8, 2024

View reviewed changes

moved check to ViewExt

c1e59b1

alduzy requested a review from kkafar October 8, 2024 15:48

updated comment

94bda4b

kkafar approved these changes Oct 9, 2024

View reviewed changes

alduzy merged commit b67af86 into main Oct 9, 2024
4 checks passed

alduzy deleted the @alduzy/fabric-going-back-with-nested-list branch October 9, 2024 07:57

kkafar mentioned this pull request Oct 10, 2024

Android, Breaks ScrollView/FlatList when inside a stack with goBackGesture #2362

Closed

This was referenced Oct 10, 2024

Android - "cannot remove child at index X" error on new architecture #2339

Closed

Android RN 0.75.3, app crashes when returning to the previous screen #2341

Closed

fix(Android): going back on fabric with horizontal list crash #2403

Merged

alduzy mentioned this pull request Nov 12, 2024

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

Merged

2 tasks

kkafar mentioned this pull request Dec 30, 2024

fix: remove workaround for removing clipped subviews #2596

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Android): going back on fabric with nested list #2383

fix(Android): going back on fabric with nested list #2383

alduzy commented Oct 3, 2024 •

edited

Loading

WoLewicki left a comment

kkafar left a comment •

edited

Loading

kkafar Oct 8, 2024

alduzy Oct 8, 2024

WoLewicki commented Oct 8, 2024

kkafar left a comment

	while (parentView is ViewGroup && parentView !is ScreenStack) {
	while (parentView is ViewGroup && parentView !is Screen) {

fix(Android): going back on fabric with nested list #2383

fix(Android): going back on fabric with nested list #2383

Conversation

alduzy commented Oct 3, 2024 • edited Loading

Description

Before

After

Changes

Test code and steps to reproduce

Checklist

WoLewicki left a comment

Choose a reason for hiding this comment

kkafar left a comment • edited Loading

Choose a reason for hiding this comment

kkafar Oct 8, 2024

Choose a reason for hiding this comment

alduzy Oct 8, 2024

Choose a reason for hiding this comment

WoLewicki commented Oct 8, 2024

kkafar left a comment

Choose a reason for hiding this comment

alduzy commented Oct 3, 2024 •

edited

Loading

kkafar left a comment •

edited

Loading