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.

Sign up for GitHub

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

Jump to bottom

Wrong headerTitle layout when pushing screen with keyboard on new arch #2232

Closed
janicduplessis opened this issue Jul 5, 2024 · 2 comments · Fixed by #2263
Closed

Wrong headerTitle layout when pushing screen with keyboard on new arch #2232

janicduplessis opened this issue Jul 5, 2024 · 2 comments · Fixed by #2263
Assignees
@kkafar @alduzy
Labels
NewArch Issues related only to new architecture Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@janicduplessis
Copy link
Contributor

janicduplessis commented Jul 5, 2024 •
edited
Loading

Description

When pushing a new screen that has headerTitle, headerRight and an autoFocus TextInput the layout of the headerTitle is incorrect the 2nd time the screen is opened.

Workaround:

Disable view recycling in RNSScreenStackHeaderSubview

// RNSScreenStackHeaderSubview.mm

+ (BOOL)shouldBeRecycled
{
  return NO;
}

Not sure if it would be an acceptable fix, but disabling view recycling does fix the problem.

Steps to reproduce

Using this code in FabricExample app HeaderOptions.tsx

Note: If using iOS simulator make sure to toggle the software keyboard.

  • Go to "Header Options" example
  • Click the "Header large item" button
  • Click "Go to next screen"
  • See that header title layout is correct
  • Click on the navbar back icon
  • Click "Go to next screen"
  • See that the header title layout is not incorrect and offset to the right

First open:

image

2nd open:

image

Weirdly this doesn't repro if no headerRight is set or if the keyboard is not opened.

Snack or a link to a repository

https://github.com/janicduplessis/react-native-screens/tree/%40janic/header-bug-2

Screens version

main@e5a6220

React Native version

0.74

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native (without Expo)

Architecture

Fabric (New Architecture)

Build type

Debug mode

Device

iOS simulator

Device model

No response

Acknowledgements

Yes
@github-actions github-actions bot added Repro provided A reproduction with a snack or repo is provided Platform: iOS This issue is specific to iOS labels Jul 5, 2024
@kkafar kkafar added the NewArch Issues related only to new architecture label Jul 8, 2024
@tboba
Copy link
Member

tboba commented Jul 17, 2024

Hi @janicduplessis, thanks for creating this issue!
@alduzy Could you check if PR #2248 that you've created fixes this issue? I believe that these layout things might be similar.

@alduzy alduzy self-assigned this Jul 17, 2024
@alduzy
Copy link
Member

alduzy commented Jul 17, 2024

@janicduplessis I can confirm it is reproducible even without opening the keyboard. @tboba Unfortunately the PR you mentioned does not fix this issue. I'll take a look at it

@kkafar kkafar mentioned this issue Jul 18, 2024
Merged
1 task
@kkafar kkafar self-assigned this Jul 25, 2024
@kkafar kkafar mentioned this issue Jul 26, 2024
Merged
4 tasks
kkafar added a commit that referenced this issue Jul 29, 2024
@kkafar
fix(iOS, Fabric): wrong headerCenter layout after native dismissal (#…
c5a36b3 
…2263)

## Description

This PR fixes #2232 by disabling view recycling in
`RNSScreenStackHeaderSubview` view component, as no better solution was
found.

The following sequence leads to the bug:

1. User clicks back button
2. The header subviews are properly removed and put in recycle pool in
the same order they were mounted (recycle pool is a stack).
3. User pushes again just-popped-screen.
4. React Native reuses view from recycle pool, but `headerCenter` is to
be mounted first, so RN reuses the view that was previously
`headerRight` (because it is on top of the recycle pool stack).
Similarly the second view in recycle pool, which was previously
`headerCenter` is now repurposed as `headerRight`.
5. RN sets correct frames for both subviews.
6. System sets wrong frame for the new `headerCenter` due to layout
triggered by `setNavigationBarHidden` in `updateViewController` of
`RNSScreenStackHeaderConfig`. The `setNavigationBarHidden` is called,
because there is difference in value of the prop between the screens
(see issue for repro). The only fact that matters is the one, that
native layout is triggered & it computes the `width` incorrectly <- it
looks like UIKit does hold some cache for layout computations & when
seeing the recycled view it just bonks it with it's previous frame, for
some reason not observing that deeper in view hierarchy contents have
changed.

I've tried to invalidate layout of the subview in various places that
could make sense, e.g. `prepareForRecycle`, just before computing native
layout in `updateViewController`, etc. for no result.

To avoid putting more debugging hours into this particular issue I went
with disabling view recycling for header subviews, originally suggested
by @janicduplessis in the issue description.


### Open questions

We would want to answer those at some time in the future:

1. [ ] What's the difference between JS-initiated dismissal and native
one, in this very case? Where does the difference in behaviour come
from? (JS-initiated works properly)
2. [ ] In what other way can we solve this w/o disabling view recycling?

Fixes #2232 

## Changes

Disable view recycling in `RNSScreenStackHeaderSubview` view component.

## Test code and steps to reproduce

`Test2232`

## 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 issue Oct 9, 2024
@kkafar @ja1ns
fix(iOS, Fabric): wrong headerCenter layout after native dismissal (s…
bee5cae 
…oftware-mansion#2263)

## Description

This PR fixes software-mansion#2232 by disabling view recycling in
`RNSScreenStackHeaderSubview` view component, as no better solution was
found.

The following sequence leads to the bug:

1. User clicks back button
2. The header subviews are properly removed and put in recycle pool in
the same order they were mounted (recycle pool is a stack).
3. User pushes again just-popped-screen.
4. React Native reuses view from recycle pool, but `headerCenter` is to
be mounted first, so RN reuses the view that was previously
`headerRight` (because it is on top of the recycle pool stack).
Similarly the second view in recycle pool, which was previously
`headerCenter` is now repurposed as `headerRight`.
5. RN sets correct frames for both subviews.
6. System sets wrong frame for the new `headerCenter` due to layout
triggered by `setNavigationBarHidden` in `updateViewController` of
`RNSScreenStackHeaderConfig`. The `setNavigationBarHidden` is called,
because there is difference in value of the prop between the screens
(see issue for repro). The only fact that matters is the one, that
native layout is triggered & it computes the `width` incorrectly <- it
looks like UIKit does hold some cache for layout computations & when
seeing the recycled view it just bonks it with it's previous frame, for
some reason not observing that deeper in view hierarchy contents have
changed.

I've tried to invalidate layout of the subview in various places that
could make sense, e.g. `prepareForRecycle`, just before computing native
layout in `updateViewController`, etc. for no result.

To avoid putting more debugging hours into this particular issue I went
with disabling view recycling for header subviews, originally suggested
by @janicduplessis in the issue description.


### Open questions

We would want to answer those at some time in the future:

1. [ ] What's the difference between JS-initiated dismissal and native
one, in this very case? Where does the difference in behaviour come
from? (JS-initiated works properly)
2. [ ] In what other way can we solve this w/o disabling view recycling?

Fixes software-mansion#2232 

## Changes

Disable view recycling in `RNSScreenStackHeaderSubview` view component.

## Test code and steps to reproduce

`Test2232`

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kkafar kkafar

@alduzy alduzy

Labels
NewArch Issues related only to new architecture Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Milestone
No milestone
4 participants
@janicduplessis @tboba @kkafar @alduzy