fix(iOS): header left and right layout on fabric #2248
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
alduzy
changed the title
…-left-and-right-layout-fix
kkafar
reviewed
Jul 18, 2024
There was a problem hiding this comment.
Looks promising, I just left a question on "whys".
If you're to spend too much time investigating then I think it is not worth it & feel free to say so.
One general note: add link to this PR in comment above the line you added, because it is definitely not obvious why the line should be there.
| @@ -72,6 +72,7 @@ - (void)updateLayoutMetrics:(const react::LayoutMetrics &)layoutMetrics | |||
| self); | |||
| } else { | |||
| self.bounds = CGRect{CGPointZero, frame.size}; | |||
| [self.superview layoutIfNeeded]; | |||
There was a problem hiding this comment.
Okay, so we now trigger layout in the native superview (this is one of system views, beyond control of react) every time we receive a new frame from react.
The solution itself is ok & if it helps I'm for it, but do we have understanding why does it not work without this change? Especially that we do not have such code on Paper & I believe it works fine there?
There was a problem hiding this comment.
Yeah, it does work fine on paper. It looks like the bounds of the subview are set properly but then the frame is given the old values unless we trigger a re-layout of the superview.
There was a problem hiding this comment.
Did you check what happens if you disable view recycling on header subviews in this PR? Im speaking with context of this issue on which we debated earlier today: #2232
There was a problem hiding this comment.
This might be the same issue of the views being swapped due to recycling pool working in stack manner.
There was a problem hiding this comment.
Turning off recycling does not fix this issue
kkafar
approved these changes
Jul 19, 2024
2 tasks
This was referenced Aug 27, 2024
This was referenced Sep 2, 2024
This was referenced Sep 24, 2024
alduzy
added a commit
that referenced
this pull request
Sep 27, 2024
## Description This PR fixes the incorrect position of the custom header items when updating more than one option at the same time. The proposed solution let us remove the previous fix for a similar problem: #2248 which only fixed the issue on fabric. Fixes #432, #2231. ## Changes - forced re-layout of the navigation controller when subviews are updated - removed previous fix - updated `Test432` repro <!-- ## Screenshots / GIFs Here you can add screenshots / GIFs documenting your change. You can add before / after section if you're changing some behavior. ### Before ### After --> ## Test code and steps to reproduce - use `Test432` and `Test2231` to test this fix on both architectures. ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
This was referenced Sep 28, 2024
2 tasks
alduzy
added a commit
that referenced
this pull request
Oct 7, 2024
## Description This PR intents to fix header subviews incorrect layout when changing tabs. The previous solution did layout the subviews correctly in the test cases, but triggered an undesirable `layoutIfNeeded` when going back from tab to tab. In such case the navigation layout happened without updating subview's layout metrics. Moving the logic to subview resolves the issue as the re-layout is now triggered only when subview's layout metrics are updated. Related fixes from the past: #2316, #2248 ## Changes - combined `Test2231.tsx` with `Test432.tsx` to create comprehensive test case - moved re-layout logic to subview ## Screenshots / GIFs ### Before  ### After  ## Test code and steps to reproduce - Use `Test432.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
## Description This PR fixes the header layout mismatch when updating both `headerLeft` and `headerRight` options simultaneously using `navigation.setOptions`. Fixes software-mansion#2231 . ## Changes - Forced subview to re-layout when updating layoutMetrics - added `Test2231.tsx` repro ## Screenshots / GIFs ### Before  ### After  ## Test code and steps to reproduce - Use `Test2231.tsx` repro ## Checklist - [x] Ensured that CI passes
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
## Description This PR fixes the incorrect position of the custom header items when updating more than one option at the same time. The proposed solution let us remove the previous fix for a similar problem: software-mansion#2248 which only fixed the issue on fabric. Fixes software-mansion#432, software-mansion#2231. ## Changes - forced re-layout of the navigation controller when subviews are updated - removed previous fix - updated `Test432` repro <!-- ## Screenshots / GIFs Here you can add screenshots / GIFs documenting your change. You can add before / after section if you're changing some behavior. ### Before ### After --> ## Test code and steps to reproduce - use `Test432` and `Test2231` to test this fix on both architectures. ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
## Description This PR intents to fix header subviews incorrect layout when changing tabs. The previous solution did layout the subviews correctly in the test cases, but triggered an undesirable `layoutIfNeeded` when going back from tab to tab. In such case the navigation layout happened without updating subview's layout metrics. Moving the logic to subview resolves the issue as the re-layout is now triggered only when subview's layout metrics are updated. Related fixes from the past: software-mansion#2316, software-mansion#2248 ## Changes - combined `Test2231.tsx` with `Test432.tsx` to create comprehensive test case - moved re-layout logic to subview ## Screenshots / GIFs ### Before  ### After  ## Test code and steps to reproduce - Use `Test432.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
kkafar
pushed a commit
that referenced
this pull request
Oct 25, 2024
## Description This PR fixes the incorrect position of the custom header items when updating more than one option at the same time. The proposed solution let us remove the previous fix for a similar problem: #2248 which only fixed the issue on fabric. Fixes #432, #2231. ## Changes - forced re-layout of the navigation controller when subviews are updated - removed previous fix - updated `Test432` repro <!-- ## Screenshots / GIFs Here you can add screenshots / GIFs documenting your change. You can add before / after section if you're changing some behavior. ### Before ### After --> ## Test code and steps to reproduce - use `Test432` and `Test2231` to test this fix on both architectures. ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes (cherry picked from commit bc95fa4)
kkafar
pushed a commit
that referenced
this pull request
Oct 25, 2024
## Description This PR intents to fix header subviews incorrect layout when changing tabs. The previous solution did layout the subviews correctly in the test cases, but triggered an undesirable `layoutIfNeeded` when going back from tab to tab. In such case the navigation layout happened without updating subview's layout metrics. Moving the logic to subview resolves the issue as the re-layout is now triggered only when subview's layout metrics are updated. Related fixes from the past: #2316, #2248 ## Changes - combined `Test2231.tsx` with `Test432.tsx` to create comprehensive test case - moved re-layout logic to subview ## Screenshots / GIFs ### Before  ### After  ## Test code and steps to reproduce - Use `Test432.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes (cherry picked from commit 91d89c4)
8 tasks
kkafar
added a commit
that referenced
this pull request
Dec 6, 2024
…2552) ## Description > [!note] > This issue seems to concern only old architecture. See below for description of Fabric situation 👇🏻 This PR aims to fix a bug described below 👇🏻 and at the same time balancing on the thin edge of not introducing regressions regarding: #2316, #2248, #2385. ### Bug description See `Test2552`. The flow is as follows: 1. we have tab navigator with nested stack navigators on each tab (A & B), 2. In `useLayoutEffect` we schedule a timer which callback changing the subview elements, 3. before the timer fires we change the tab from A to B, 4. wait few seconds fot timer to fire, 5. go back to A, 6. notice that the subviews are laid out incorrectly (video below 👇🏻) https://github.com/user-attachments/assets/2bf621a7-efd4-44cf-95e1-45a46e425f07 Basically what happens is we're sending `layoutIfNeeded` to navigation bar before subviews are mounted under navigation bar view hierarchy. This causes "isLayoutDirty" flags to be cleaned up and subsequent `layoutIfNeeded` messages have no effect. ## Changes We now wait with triggering layout for the subview to be attached to window. > [!note] > TODO: possibly we should call the layout from `didMoveToWindow` but I haven't found the case it does not work without the call, so I'm not adding it for now. > [!note] > Calling layout on whole navigation bar for every subview update seems wrong, however the change is subview change can have effect on other neighbouring views (e.g. long title which need to be truncated) & it seems that we need to do this. Maybe we could get away will requesting it only from UINavigationBarContentView skipping few levels, but this is left for consideration in the future. > [!important] > I do not understand why we need to send `layoutIfNeeded` and `setNeedsLayout` is not enough, but not sending the former results in regressions in Test432. Leaving it for future considerations. ### Fabric The strategy with setting screen options inside timer nested in useLayoutEffect seems to not work at all on new architecture. My impression is that the timer gets cancelled every time the screen loses focus (tab is changed). I do not know whether this is a bug on our side, or maybe it should work this way or it is Fabric bug. This should be debugged in future. ## Test code and steps to reproduce Test2552 - Follows the steps described above ☝🏻 Test432 - Follow the steps from issues described in mentioned issues :point_up: ## 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
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
This PR fixes the header layout mismatch when updating both
headerLeftandheaderRightoptions simultaneously usingnavigation.setOptions.Fixes #2231 .
Changes
Test2231.tsxreproScreenshots / GIFs
Before
After
Test code and steps to reproduce
Test2231.tsxreproChecklist