fix(Android, Stack): don't add top inset to measured header height when it's disabled#4357
Open
phazei wants to merge 1 commit into
Open
Conversation
…consumeTopInset is false The C++ dummy-layout measurement (findHeaderHeight -> ScreenDummyLayoutHelper. computeDummyLayout) unconditionally added the decor view top inset to the reported header height. The native CustomToolbar, however, only pads itself with the top inset when `legacyTopInsetBehavior || consumeTopInset` holds (introduced in software-mansion#4220). As a result, any header that opts out of the top inset via `disableTopInsetApplication` (consumeTopInset == false) has its measured height over-reported by the status-bar / cutout inset, offsetting screen content by that amount on the first Fabric layout. Mirror the toolbar's condition in the measurement path: - pass `applyTopInset = legacyTopInsetBehavior || consumeTopInset` through JNI (computeDummyLayout signature (IZ)F -> (IZZ)F) - zero the top inset in the dummy measurement when applyTopInset is false - include applyTopInset in the measurement cache key so mixed-mode headers do not collide on a stale cached height Verify with Test4220: toggling `disableTopInsetApplication` should no longer offset the screen content below the header.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
When a header opts out of the top inset on Android (
disableTopInsetApplication, which setsconsumeTopInset = false), the measured header height still includes the status bar / cutout inset, so it comes back too tall.The native
CustomToolbaronly pads itself with the top inset whenlegacyTopInsetBehavior || consumeTopInsetis true. But the dummy layout used to report the header height back to the shadow tree always adds the decor view top inset, regardless of that prop. So the height it reports doesn't match what the toolbar actually renders, and the screen content ends up pushed down by roughly the status bar height on the first Fabric layout.This looks like it was just missed when the
disable*InsetApplicationprops landed in #4220 — the toolbar learned aboutconsumeTopInsetbut the measurement path didn't.Changes
RNSScreenShadowNode.cpp: read the same header props the toolbar uses (legacyTopInsetBehavior || consumeTopInset) and pass the result down to the measurement asapplyTopInset. The JNI signature changes from(IZ)Fto(IZZ)F.ScreenDummyLayoutHelper.kt: only add the decor view top inset whenapplyTopInsetis true, and include the flag in the measurement cache key so screens with different settings don't share a stale cached height.Test plan
Reproducible with the existing
Test4220scenario:Test4220on Android.disableTopInsetApplicationon.Before this change the screen content sits about a status bar's height too low (the header reserves space it isn't actually using). After, the content sits directly under the header.
Checklist
Test4220)