-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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: fix useHeaderHeight in native-stack #10827
fix: fix useHeaderHeight in native-stack #10827
Conversation
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
The Expo app for the example from this branch is ready! expo.dev/@react-navigation/react-navigation-example?release-channel=pr-10827 |
Truly appreciate this fix. Thank you! |
Hey! This issue is closed and isn't watched by the core team. You are welcome to discuss the issue with others in this thread, but if you think this issue is still valid and needs to be tracked, please open a new issue with a repro. |
…`statusBarTranslucent` to prevent header content jump (#12014) **Motivation** *copied & slightly redacted message from discord* 😄 Currently we set `headerTopInsetEnabled: true` when `safe-area-context` reports any non-zero top inset. This prop is intended to handle missing inset when `statusBarTranslucent` is set to `true` (so whole header is moved up, and headerTitle is partially obfuscated by the status bar, just like on the first screenshot, so in the place I've sent you it seems that it is a bit misused? I'm opening this PR, because I'm handling all header-related issues right now and: when `statusBarTranslucent: false` & `headerTopInsetEnabled == topInset !== 0` we get buggy behaviour on native stack, because `safe-area-context` does report `topInset > 0` only during the initial render & on every consecutive render `topInset === 0`, thus `headerTopInsetEnabled === false` <- this results in jumping header content (sometimes even whole screen contents do jump), because during first render we apply the headerTopInset (which basically means adding topPadding to header with the value of status bar's height), and on every consecutive render this padding is set to 0. See the attached recording (best to download and go frame by frame). What's also intriguing, `safe-area-context` does report non-zero top inset during initial JS render only because we pass `initialWindowMetrics` as `initialMetrics` [prop to SafeAreaProvider](https://github.com/react-navigation/react-navigation/blob/25e834b4f330fb05b4ecdbe81ef7f2f065fd26c8/packages/elements/src/SafeAreaProviderCompat.tsx#L55), and these (`initialWindowMetrics`) do have non zero top padding (thus inconsistency with what we get in first render. I digged into `safe-area-context` implementation & I won't be really able to fix these initial window metrics there, because they are "true", and I mean by that they are reporting real window dimensions with existing status bar / notch / navigation bar offsets, but the insets that come after first render are from perspective of the SafeAreaProvider view that resides in native tree, and not from window anymore, thus it reports 0, 0, 0, 0 for insets <- and this is also technically correct. | statusBarTranslucent: true, headerTopInsetEnabled: false | Jumping header title video | | ---- | ---- | | <img src=https://github.com/react-navigation/react-navigation/assets/50801299/45207a46-9260-4b4e-b614-c86c77e46cf8 width="270" height="555" /> | <video src="https://github.com/react-navigation/react-navigation/assets/50801299/70c0aa1f-b49a-4500-8d81-88f4ea778e6a" controls width="270" height="555" /> | ### Proposed solution For me, the easiest way to go forward would be to set `headerTopInsetEnabled` to `false` everytime `statusBarTranslucent == false`. `headerTopInsetEnabled` is not exposed as a part of public API, so that's feasible without introducing breaking changes if we come to conclusion that suggested fix is ok. ### Ideal solution `react-native-safe-area-context` frame reported in the very first render (so the constant we use for `initialMetrics`) should be the same as in consecutive renders. Action points: dig into whether we can use some different initial metrics. **Test plan** I've tested on the example prepared for #10827 by @kacperkapusciak: [link](https://snack.expo.dev/@kacperkapusciak/native-stack-header-height). As `headerTopInsetEnabled` prop impacts only Android, I've tested only Android. **I do see an regression here**, on fifth screen the reported header height is *negative*, **however this happens also on main** so it is not the impact of this PR. Other behaviour seems ok & it also fixes my problem. Video with regression: https://github.com/react-navigation/react-navigation/assets/50801299/84f43a56-b6a0-4d0b-baad-39ed72847d6d The change must pass lint, typescript and tests.
Motivation
useHeaderHeight
should return the height of the closest header but in native-stack but it returns incorrect values.Fixes #10333
iOS
Before
before-ios.mov
After
after-ios.mov
iPad
Before
before-ipad.mov
After
after-ipad.mov
Android
Before
before-android.mp4
After
after-android.mp4
Web
Same behavior before and after.
Screen.Recording.2022-09-08.at.16.04.35.mov
Test code
https://snack.expo.dev/@kacperkapusciak/native-stack-header-height
Code example:
Added tests to accommodate this change.