Fix bug with header hide prop on the screen under the modal #2229
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
This was referenced Aug 23, 2024
This was referenced Aug 27, 2024
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
…oftware-mansion#2229) ## Description When we push multiple screens with different configuration of `headerShown` prop (e.g. 4 screens with visible header and one screen with hidden header), open a modal and trigger rerenders on screens (e.g. by changing the theme) there's a high chance that value of header hide prop will be incorrect on the screen under the modal. Big kudos to @WoLewicki for helping with debugging! ## Changes `isPresentingVC` flag wasn't checking if current view controller was on the top, which was causing header updates for all screens in the stack (moreover it was done in non-deterministic order as context updates can cause non-deterministic rerenders of particular screens), so check `&& vc == nav.topViewController` was added to `isPresentingVC` flag in `ios/RNSScreenStackHeaderConfig.mm` file. After that change, we still update the screen which is directly under the modal, so fix merged in software-mansion#1228 is still valid. ## Screenshots / GIFs ### Before https://github.com/software-mansion/react-native-screens/assets/44415389/cf9b07fc-4e95-4362-95c7-ab387a94148e ### After https://github.com/software-mansion/react-native-screens/assets/44415389/94a5dd72-498b-4959-a2de-1e6008d63895 ## Test code and steps to reproduce 1. `Test2229` was added, so just render it in `TestsExample` app 2. Push a few screens with header 3. Push screen without header 4. Open modal 5. Background and open the app or change theme 6. Close the modal 7. Before the fix header should be visible on the screen although it should be a screen without a header ## Checklist - [X] 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
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
When we push multiple screens with different configuration of
headerShownprop (e.g. 4 screens with visible header and one screen with hidden header), open a modal and trigger rerenders on screens (e.g. by changing the theme) there's a high chance that value of header hide prop will be incorrect on the screen under the modal.Big kudos to @WoLewicki for helping with debugging!
Changes
isPresentingVCflag wasn't checking if current view controller was on the top, which was causing header updates for all screens in the stack (moreover it was done in non-deterministic order as context updates can cause non-deterministic rerenders of particular screens), so check&& vc == nav.topViewControllerwas added toisPresentingVCflag inios/RNSScreenStackHeaderConfig.mmfile. After that change, we still update the screen which is directly under the modal, so fix merged in #1228 is still valid.Screenshots / GIFs
Before
Screen.Recording.2024-07-02.at.18.17.23.mov
After
Screen.Recording.2024-07-03.at.15.54.19.mov
Test code and steps to reproduce
Test2229was added, so just render it inTestsExampleappChecklist