fix!: always set Zindex to undefined for InnerScreen #2351
Merged
Conversation
This file contains 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
adrianryt
changed the title
adrianryt
changed the title
adrianryt
force-pushed
the
always-set-zindex-to-undefined-for-InnerScreen
branch
from
65b1b74 to
f4d0502
Compare
Co-authored-by: Kacper Kafara <[email protected]>
adrianryt
force-pushed
the
always-set-zindex-to-undefined-for-InnerScreen
branch
from
f4d0502 to
dd33cdb
Compare
kkafar
changed the title
WoLewicki
reviewed
Sep 26, 2024
| @@ -125,6 +126,7 @@ export const InnerScreen = React.forwardRef<View, ScreenProps>( | |||
| <DelayedFreeze freeze={freezeOnBlur && activityState === 0}> | |||
| <AnimatedScreen | |||
| {...props} | |||
| style={[style, { zIndex: undefined }]} | |||
There was a problem hiding this comment.
I'd add a comment that we are handling the order of screens on the native side and changing it here causes issues, with the link to the issue.
kkafar
requested changes
Sep 26, 2024
There was a problem hiding this comment.
Yeah, beside the comment and testing on react-navigation v6 I think we're good. Let me know once the comment is added (mark me for another review).
I've enhanced the PR description, so we can better reconstruct the error mechanism in case its needed in few months.
| @@ -125,6 +126,7 @@ export const InnerScreen = React.forwardRef<View, ScreenProps>( | |||
| <DelayedFreeze freeze={freezeOnBlur && activityState === 0}> | |||
| <AnimatedScreen | |||
| {...props} | |||
| style={[style, { zIndex: undefined }]} | |||
kkafar
approved these changes
Sep 26, 2024
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…n#2351) ## Description Together with @kkafar we noticed that when switching back between BottomTabsView (A -> B -> A) , React is calling removeScreenAt and addScreen for tab we are leaving - in the same transaction! Because of that and asynchronous nature of react-native-screens, error described in software-mansion#2345 exist. We also noticed that this weir behaviour is related to setting ZIndex in InnerScreen. Fixes software-mansion#2345 Test 2232 was failing due to change in headerBackTitleVisible property. > [!Caution] @kkafar: Note that this change might be potentially breaking since we're effectively removing possibility of managing `Screen` components through "z indices", breaking public API. @kkafar: The error mechanism is as follows: 1. `zIndex` being set causes RN diffing mechanism to include two mutations on the same view in the same transaction - effectively detaching and attaching it, 2. Thus when we navigate from tab B to A, react first detaches B from screen container and in the same transaction it attaches it again - however at the moment of reattach react expects B to be detached. This is not case due to the fact, that we don't execute updates synchronously, but rather we just schedule them in another block on UI thread; 3. React asserts the invariant from point 2., and when we violate it, its internal state gets corrupted later leading to crash. We detected that getting rid of setting `zIndex` on screens prevents the two consecutive operations on the same screen to appear, thus effectively solving the problem. Note, however, that we still won't support such cases with multiple mount/unmount mutations related to the same component in single transaction. ## Changes - For every InnerScreen created we set/override ZIndex style to undefined. - Use headerBackButtonDisplayMode instead of headerBackTitleVisible. ## Test code and steps to reproduce ## Checklist - [ ] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <[email protected]>
kkafar
pushed a commit
that referenced
this pull request
Oct 25, 2024
## Description Together with @kkafar we noticed that when switching back between BottomTabsView (A -> B -> A) , React is calling removeScreenAt and addScreen for tab we are leaving - in the same transaction! Because of that and asynchronous nature of react-native-screens, error described in #2345 exist. We also noticed that this weir behaviour is related to setting ZIndex in InnerScreen. Fixes #2345 Test 2232 was failing due to change in headerBackTitleVisible property. > [!Caution] @kkafar: Note that this change might be potentially breaking since we're effectively removing possibility of managing `Screen` components through "z indices", breaking public API. @kkafar: The error mechanism is as follows: 1. `zIndex` being set causes RN diffing mechanism to include two mutations on the same view in the same transaction - effectively detaching and attaching it, 2. Thus when we navigate from tab B to A, react first detaches B from screen container and in the same transaction it attaches it again - however at the moment of reattach react expects B to be detached. This is not case due to the fact, that we don't execute updates synchronously, but rather we just schedule them in another block on UI thread; 3. React asserts the invariant from point 2., and when we violate it, its internal state gets corrupted later leading to crash. We detected that getting rid of setting `zIndex` on screens prevents the two consecutive operations on the same screen to appear, thus effectively solving the problem. Note, however, that we still won't support such cases with multiple mount/unmount mutations related to the same component in single transaction. ## Changes - For every InnerScreen created we set/override ZIndex style to undefined. - Use headerBackButtonDisplayMode instead of headerBackTitleVisible. ## Test code and steps to reproduce ## Checklist - [ ] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation: <!-- For adding new props to native-stack --> - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <[email protected]> (cherry picked from commit 53695c3)
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.
Description
Together with @kkafar we noticed that when switching back between BottomTabsView (A -> B -> A) , React is calling removeScreenAt and addScreen for tab we are leaving - in the same transaction! Because of that and asynchronous nature of react-native-screens, error described in #2345 exist. We also noticed that this weir behaviour is related to setting ZIndex in InnerScreen.
Fixes #2345
Test 2232 was failing due to change in headerBackTitleVisible property.
Caution
@kkafar:
Note that this change might be potentially breaking since we're effectively removing possibility of managing
Screencomponents through "z indices", breaking public API.@kkafar:
The error mechanism is as follows:
zIndexbeing set causes RN diffing mechanism to include two mutations on the same view in the same transaction - effectively detaching and attaching it,We detected that getting rid of setting
zIndexon screens prevents the two consecutive operations on the same screen to appear, thus effectively solving the problem.Note, however, that we still won't support such cases with multiple mount/unmount mutations related to the same component in single transaction.
Changes
Test code and steps to reproduce
Checklist