Skip to content

Wrap a screen content to a Box for the iOS back animation blackout #2146

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

Merged
merged 1 commit into from
Jun 3, 2025

Conversation

terrakok
Copy link
Member

@terrakok terrakok commented May 28, 2025

It fixes a crash when NavHost is located inside a scrollable container and there is no available max size.

The reason is an additional iOS blackout layer.
Simple reproducer:

Column(Modifier.fillMaxSize().verticalScroll(rememberScrollState())) {
  NavHost(...)
}

Inside the verticalScroll it is not possible to measure constrains:

.layout { m, c ->
  val placeable = m.measure(Constraints.fixed(c.maxWidth, c.maxHeight)
  layout(c.minWidth, c.minHeight) { placeable.place(0, 0) }
}

So, I wrapped the screen content in an additional Box and use matchParentSize() modifier now.

Fixes https://youtrack.jetbrains.com/issue/CMP-8233

Release Notes

Fixes - Navigation

  • Fix a crash on iOS when a NavHost is located in a scrollable container

@terrakok terrakok requested a review from MatkovIvan May 28, 2025 18:26
@MatkovIvan MatkovIvan changed the title Wrap a screen content to a Box for the iOS back animation blackout. Wrap a screen content to a Box for the iOS back animation blackout Jun 3, 2025
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

LGTM in general, just a few nitpicks

@@ -719,28 +721,33 @@ internal fun NavHost(
// while in the scope of the composable, we provide the navBackStackEntry as the
// ViewModelStoreOwner and LifecycleOwner
currentEntry?.LocalOwnersProvider(saveableStateHolder) {
(currentEntry.destination as ComposeNavigator.Destination).content(
Copy link
Member

Choose a reason for hiding this comment

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

It probably makes sense to extract our extra drawing into a separate function to simplify the diff with AOSP (+ it's already a huge function)

Copy link
Member Author

Choose a reason for hiding this comment

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

there is a lot of the local context to pass to the new function. readability will be worse

It fixes a crash when NavHost is located inside a scrollable container and there is no available max size.
@terrakok terrakok requested a review from MatkovIvan June 3, 2025 12:22
@terrakok terrakok merged commit c8ddf88 into jb-main Jun 3, 2025
10 checks passed
@terrakok terrakok deleted the k.tskh/CMP-8233 branch June 3, 2025 12:57
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.

2 participants