Skip to content

fix(Android, Stack v4): Add heuristic for applying inset only on top-level header#3793

Merged
t0maboro merged 17 commits intomainfrom
@t0maboro/header-inset-consumption
Apr 9, 2026
Merged

fix(Android, Stack v4): Add heuristic for applying inset only on top-level header#3793
t0maboro merged 17 commits intomainfrom
@t0maboro/header-inset-consumption

Conversation

@t0maboro
Copy link
Copy Markdown
Contributor

@t0maboro t0maboro commented Mar 24, 2026

Description

This PR fixes multiple applications of top insets when stack navigators are nested, leading to excessive whitespace. We introduced a TopInsetConsumptionContext to coordinate top inset consumption across nested navigators. The topmost visible, non-hidden header consumes the inset and notifies nested screens that the inset has already been handled.
Additionally, we implemented a pre-layout inset handling mechanism using DecorView to prevent layout jumps before the window insets are dispatched - this is an updated approach relative to: #3442. Additionally, all workarounds that were related to manual frame corrections from that PR were removed, because now we can apply proper layout to the Toolbar earlier, and we don't need to make any manual corrections before the insets come.
We're adding an option to opt out of this behavior with an experimental feature flag androidLegacyTopInsetBehavior.

Changes

  • added TopInsetConsumptionContext to track whether the inset was already consumed higher up in the hierarchy
  • ScreenStackItem now calculates whether its nested children should consider the top inset consumed
  • ScreenStackHeaderConfig reads the context and determines if it should consume the inset.
  • androidLegacyTopInsetBehavior was added to allow opt-out from this approach
  • added a logic to apply pre-layout padding, reading insets from DecorView - with additional logic to clean up applied paddings and insets when the component was detached
  • added new native props
  • removed old workarounds related to manual layout calculations on Screen and AppBarLayout level

Before & after - visual documentation

Test3793

Before After
test3793-before.mov
test3793-after.mov

Test3006
Note: issues with jumping content in Tabs->Stack(with header)->Stack(with header) combo are still present, don't see an option to resolve that now, because the DummyLayoutHelper would need to be aware which header is top-level, for now I'm treating that specific setup as uncommon.

Before After
(shouldn't change)
test3006-after.mov

Test plan

Added Test3793, performed regression testing on Test3006.

Checklist

  • Included code example that can be used to test this change.
  • Updated / created local changelog entries in relevant test files.
  • For visual changes, included screenshots / GIFs / recordings documenting the change.
  • For API changes, updated relevant public types.
  • Ensured that CI passes

@t0maboro t0maboro marked this pull request as ready for review March 24, 2026 11:50
@t0maboro t0maboro requested a review from Copilot March 24, 2026 11:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses excessive whitespace on Android when stack navigators are nested by ensuring the top inset is consumed only by the topmost visible header. It coordinates inset consumption across nested stacks via a React context, adds pre-layout inset handling using DecorView to reduce layout jumps, and introduces an experimental flag to opt out.

Changes:

  • Add TopInsetConsumptionContext and wire it through ScreenStackItem / ScreenStackHeaderConfig to decide which header consumes the top inset.
  • Introduce androidLegacyTopInsetBehavior experimental feature flag to opt out and retain legacy behavior.
  • Update Android native header config/toolbar to support consumeTopInset + pre-layout DecorView inset application; remove older AppBarLayout workaround; add a new regression test (Test3793).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/flags.ts Adds experimental flag androidLegacyTopInsetBehavior.
src/fabric/ScreenStackHeaderConfigNativeComponent.ts Extends native props with consumeTopInset / legacyTopInsetBehavior.
src/components/contexts/TopInsetConsumptionContext.ts Introduces context to track top inset consumption across nested stacks.
src/components/ScreenStackItem.tsx Provides inset-consumption context to nested children based on header visibility/consumption.
src/components/ScreenStackHeaderConfig.tsx Computes and forwards inset-consumption props to native header config.
android/.../ScreenStackHeaderConfigViewManager.kt Wires new props to ScreenStackHeaderConfig on Android.
android/.../ScreenStackHeaderConfig.kt Stores new props and triggers inset re-application when they change.
android/.../CustomToolbar.kt Adds DecorView pre-layout inset application and consumption-aware inset handling.
android/.../ScreenStackFragment.kt Removes usage of CustomAppBarLayout and falls back to AppBarLayout.
android/.../Screen.kt Removes prior manual inset-based shadow state correction.
android/.../CustomAppBarLayout.kt Deletes the old AppBarLayout correction workaround.
apps/src/tests/issue-tests/Test3793.tsx Adds regression test for nested stacks and toggling header visibility.
apps/src/tests/issue-tests/index.ts Exports the new test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread android/src/main/java/com/swmansion/rnscreens/CustomToolbar.kt
Comment thread src/components/ScreenStackItem.tsx Outdated
Comment thread apps/src/tests/issue-tests/Test3793.tsx Outdated
@phazei
Copy link
Copy Markdown

phazei commented Mar 26, 2026

This is really needed, currently the inset on the header intermittently disappears, and is gone in landscape but back in portrait, there's no consistency so there's no way to manually set anything when it's not there. This seems like it should make all the cases less fragile.

Comment thread android/src/main/java/com/swmansion/rnscreens/CustomToolbar.kt
Comment thread android/src/main/java/com/swmansion/rnscreens/ScreenStackHeaderConfig.kt Outdated
@t0maboro t0maboro force-pushed the @t0maboro/header-inset-consumption branch from 5ac64dd to f0eaaeb Compare April 1, 2026 18:10
Comment thread android/src/main/java/com/swmansion/rnscreens/CustomToolbar.kt Outdated
Comment thread android/src/main/java/com/swmansion/rnscreens/CustomToolbar.kt Outdated
Comment thread android/src/main/java/com/swmansion/rnscreens/CustomToolbar.kt
Comment thread src/components/contexts/TopInsetConsumptionContext.ts Outdated

const nextContextValue = React.useMemo(
() => ({
isTopInsetConsumed: insetContext.isTopInsetConsumed || consumesTopInset,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see BottomTabsAndStack example. The inset is repeated due to JS header in tabs.

Image

This seems like a good example where we should allow the user to disable the top inset maunally, right? This functionality is missing in this PR but I think that we discussed it. Is this going to be handled in separate PR?

But if we're going to add it, we need to think about propagating that information.

In regular case, if the outer header is hidden, then the inner header should handle the inset as is right now in this PR.

Another situation: imagine if we had 2 native stacks in BottomTabsAndStack. If the user manually disables outer native header inset, then the inner header shouldn't apply the padding as well, right? The user override indicates that we applied inset where we shouldn't and any header below it shouldn't apply the inset as well? On the other hand, setting the same prop to all stacks isn't that hard and it would simplify our approach.

Just something to think about.

Copy link
Copy Markdown
Contributor Author

@t0maboro t0maboro Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make that in the follow-up PR, ticket for that: https://github.com/software-mansion/react-native-screens-labs/issues/1096

I think that just exposing a prop is fine in that case, because the prop will be living per screen instance, so the downstream might propagate that information as well

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that initially - especially in stack v4, we should settle with allowing for manual control.

Describe somewhere how does the heuristic work and when it needs assistance (cases like described above).

Let's keep as simple as possible for stack v4.

Copy link
Copy Markdown
Contributor Author

@t0maboro t0maboro Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be followed-up in: #3835

@t0maboro t0maboro requested a review from kligarski April 2, 2026 08:12
Copy link
Copy Markdown
Contributor

@kligarski kligarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a regression in this PR or does it happen on main as well (look at the navigation bar inset for bottom navigation view)?

Screen_recording_20260407_083008.mp4

@t0maboro
Copy link
Copy Markdown
Contributor Author

t0maboro commented Apr 7, 2026

Is this a regression in this PR or does it happen on main as well (look at the navigation bar inset for bottom navigation view)?

Screen_recording_20260407_083008.mp4

Known issue with delayed insets when native tabs are nested in stack: #3747 (comment)

@phazei
Copy link
Copy Markdown

phazei commented Apr 8, 2026

Any reason this hasn't been merged yet?

@phazei
Copy link
Copy Markdown

phazei commented Apr 8, 2026

Oh, another use case that needs accounting for, likely an option for manual control of disabling the header top inset entirely:

If the user wants a top banner: Right now when adding a or at the very top of the app, since the header always applies the margin no matter what, that extra margin is added under the top banner.

Has this been tested in landscape? Currently the header doesn't add any inset in landscape mode, and only does in portrait.

Also, wouldn't have never had adding it to the header at all have been the easiest, then it's left to the dev to handle it and it could be managed, having it auto added is what made it unmanagable.

@t0maboro
Copy link
Copy Markdown
Contributor Author

t0maboro commented Apr 9, 2026

Oh, another use case that needs accounting for, likely an option for manual control of disabling the header top inset entirely:

If the user wants a top banner: Right now when adding a or at the very top of the app, since the header always applies the margin no matter what, that extra margin is added under the top banner.

will be realized in a follow-up PR: #3835

Has this been tested in landscape? Currently the header doesn't add any inset in landscape mode, and only does in portrait.

landscape:

without content overflow: Screenshot 2026-04-09 at 09 02 49

with content overflow: Screenshot 2026-04-09 at 09 02 23

@t0maboro t0maboro merged commit e74d69f into main Apr 9, 2026
7 checks passed
@t0maboro t0maboro deleted the @t0maboro/header-inset-consumption branch April 9, 2026 07:04
@isaacfilarski
Copy link
Copy Markdown

Hi, when is this going to be released?

@t0maboro
Copy link
Copy Markdown
Contributor Author

We still have a few important changes to merge for the native tabs, native stack memory leak fixes and a follow-up PR for the header changes before we release 4.25. If this update is important for you right now, I recommend using the nightly release.

@isaacfilarski
Copy link
Copy Markdown

We still have a few important changes to merge for the native tabs, native stack memory leak fixes and a follow-up PR for the header changes before we release 4.25. If this update is important for you right now, I recommend using the nightly release.

Thank you, could you tell me which nightly release has this specific change? I see that the last one was 8 days ago.

@t0maboro
Copy link
Copy Markdown
Contributor Author

Sorry, we didn't notice that the nightly releases started failing last week - we're fixing it now #3878

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.

6 participants