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 broken layout for header subviews in iOS #902

Merged
merged 3 commits into from
May 7, 2021

Conversation

abram
Copy link
Contributor

@abram abram commented Apr 30, 2021

Description

This addresses bugs such as #528 where header content is incorrectly positioned on initial load or after rotation. I was experiencing these issues in my current app, and when I looked at the XCode view debugger I could see that RNSScreenStackHeaderConfig subviews had their .frame.origin.x property set to a non-zero value that moved them out of place.

I'm fairly new to React Native and very new to this codebase, so I haven't totally got to the bottom of what's happening here. My guess/theory is that RN layout code is setting the position of these views as if they're part of of the regular view hierarchy, even though they're meant to be managed by a UINavigationBar. Rather than trying to fix this at a deeper level and risk introducing side effects, I've added a small workaround.

Fixes #528.

Changes

This PR adds a few lines of code inside RNSScreen.viewDidLayoutSubviews which manually set the .frame.origin.x and .y of each RNSScreenStackHeaderConfig to zero. I believe it's accomplishing the same thing as PR #668 (which was my starting point for this work) but in a simpler and more direct way.

)

This addresses bugs like software-mansion#528 where header content gets incorrectly
positioned on initial load or after rotation. I was experiencing
these issues in my current app, and when I looked at the XCode
view debugger I could see that RNSScreenStackHeaderConfig.subviews
were having their .frame.origin.x property set to a non-zero
value that moved them out of place.

I'm fairly new to React Native and very new to this codebase, so
I haven't totally got to the bottom of what's happening here. My
guess/theory is that RN layout code is setting the position of
these views as if they're part of of the regular view hierarchy,
even though they're meant to be managed by a UINavigationBar. Rather
than trying to fix this at a deeper level and risk introducing side
effects, I've added this small workaround.
@WoLewicki
Copy link
Member

Thanks for the PR! Yeah looks like the layouts passed by yoga layout are somehow trying to apply their calculations instead of leaving the origin to 0. I will test it and let other users do it and if it works, I think we can merge it 🎉

@WoLewicki
Copy link
Member

Maybe instead of changing it in RNScreen.m, we could do it directly in RNSScreenStackHeaderSubview.m. It would look something like this added here: https://github.com/software-mansion/react-native-screens/blob/master/ios/RNSScreenStackHeaderConfig.m#L43.

- (void)reactSetFrame:(CGRect)frame
{
  [self setFrame:CGRectMake(0, 0, frame.size.width, frame.size.height)];
}

Could you test it and if it works the same, maybe add it like this instead?

Per @WoLewicki's suggestion in software-mansion#902, move this code down to
a lower level where it makes more sense.
@abram
Copy link
Contributor Author

abram commented May 4, 2021

Brilliant! That's definitely a better place for this change. I updated the PR as you suggested and the fix still works as it should, at least in my testing.

Co-authored-by: Wojciech Lewicki <wojciech.lewicki@swmansion.com>
@WoLewicki WoLewicki merged commit 5105c40 into software-mansion:master May 7, 2021
WoLewicki pushed a commit that referenced this pull request May 20, 2021
This PR overrides `reactSetFrame` inside `RNSScreenStackHeaderSubview` which manually set the `frame.origin.x` and `.y` of each RNSScreenStackHeaderConfig to zero because they're meant to be managed by a `UINavigationBar`, and instead the RN layout gives them wrong origin e.g. on rotation.
@hirbod
Copy link
Contributor

hirbod commented May 29, 2022

@WoLewicki this is still happening randomly when I use headerRight. Sometimes, my buttons are totally off screen.

@WoLewicki
Copy link
Member

@hirbod I haven't spotted this issue since this change iirc. Can you provide a repro where it can be spotted?

@hirbod
Copy link
Contributor

hirbod commented May 30, 2022

It is nearly impossible to reproduce. 50 calls work, 1 does not. @WoLewicki

@WoLewicki
Copy link
Member

Seems really weird since due to this PR it should be controlled by the native side and work just fine. I cannot say much without a repro unfortunately :/

@hirbod
Copy link
Contributor

hirbod commented May 31, 2022

Just had 4 reports today, that it happened to them (currently in a private beta and rolled out our soon to be released app to around 30 people)

The ONLY thing I am doing is:

  useEffect(() => {
    navigation.setOptions({
      headerRight: () => (
        <Text sx={{ color: '$accent', opacity: 0.3 }} variants={['semibold']}>
          {t('general.save')}
        </Text>
      ),
    });
  }, []);

That's it. I just navigated to that screen 30-40 times, it happen once and all the other times its just fine.
My simple Text was like 30px offscreen to the right.

@WoLewicki
Copy link
Member

Does this code gets triggered when you navigate to a screen? Maybe there is some race condition between setting headerRight and the start of navigation animation, during which the system does not accept any layout changes on the header iirc. Does it happen anytime if you set headerRight in options of the stack's screen?

@hirbod
Copy link
Contributor

hirbod commented Jun 1, 2022

Yes it gets triggered when I navigate to the screen. I did not encounter this when set directly in screens options, but I need dynamic function since I pass an onPress function there (toggling active state of my save button).

@WoLewicki
Copy link
Member

Did you try using useLayoutEffect instead? I have no other idea how to solve it unfortunately if it is some kind of race condition between JS and native side.

@hirbod
Copy link
Contributor

hirbod commented Jun 1, 2022

No I did not try useLayoutEffect, I could give it a try.

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.

Header right elements disappear on rotate
3 participants