Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(iOS, Paper): fix header layout when updating non focued screen #2552

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Dec 6, 2024

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 👇🏻)
Screen.Recording.2024-12-06.at.17.15.13.mov

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 ☝️

Checklist

@kkafar kkafar changed the title fix(iOS): fix header layout when updating non focued screen fix(iOS, Paper): fix header layout when updating non focued screen Dec 6, 2024
@kkafar kkafar merged commit 8c43de6 into main Dec 6, 2024
5 checks passed
@kkafar kkafar deleted the @kkafar/spf-header-issue-repro branch December 6, 2024 22:56
kkafar added a commit that referenced this pull request Dec 6, 2024
#2553)

## Description

I was just writing #2552 and got confused that the header config
subviews are added as subviews to the header config view
in host tree, before they are attached to the navigation bar view
hierarchy. This transient state does not make any sense,
and we do not do similar thing on Fabric.

## Changes

`self.superview` returns now `nil` until the subview is mounted in
navigation bar view hierarchy.

Also moved the function to Paper specific section of file, because it is
not called on Fabric on any codepath.

## Test code and steps to reproduce

We haven't used this behaviour. There should be no regression in text
examples.

## 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant