diff --git a/ui/navigation/build.gradle.kts b/ui/navigation/build.gradle.kts index 27266d9bf8..08015a2ea7 100644 --- a/ui/navigation/build.gradle.kts +++ b/ui/navigation/build.gradle.kts @@ -1,3 +1,5 @@ +import java.time.Duration + plugins { alias(libs.plugins.flipcash.kmp.app.library) alias(libs.plugins.compose.compiler) @@ -59,3 +61,9 @@ dependencies { add("kspJvm", libs.dagger.compiler) add("jvmMainImplementation", libs.dagger) } + +// WrapContentSheetLayoutLoopTest fails by hanging: a sheet stuck in a measure loop never returns +// to the test. Cap the run so that shows up as a failure instead of eating the CI job. +tasks.withType().configureEach { + timeout.set(Duration.ofMinutes(10)) +} diff --git a/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt b/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt new file mode 100644 index 0000000000..c370240495 --- /dev/null +++ b/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt @@ -0,0 +1,105 @@ +package com.getcode.navigation.scenes + +import androidx.compose.animation.AnimatedContent +import androidx.compose.animation.SharedTransitionLayout +import androidx.compose.animation.SizeTransform +import androidx.compose.animation.core.tween +import androidx.compose.animation.fadeIn +import androidx.compose.animation.fadeOut +import androidx.compose.animation.togetherWith +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import androidx.compose.ui.Modifier +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.unit.dp +import androidx.navigation3.runtime.NavEntry +import androidx.navigation3.runtime.NavKey +import com.getcode.navigation.AppHome +import com.getcode.navigation.DemoSheet +import com.getcode.navigation.NavMetadataKeys +import com.getcode.navigation.core.EmptyCodeNavigator +import com.getcode.navigation.core.LocalCodeNavigator +import com.getcode.navigation.scrim.LocalScrimController +import com.getcode.navigation.scrim.ScrimController +import com.getcode.navigation.testNavigator +import com.getcode.theme.DesignSystem +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * A wrap-content sheet whose content animates its height while the sheet is still opening. + * + * The app hosts every scene in a `SharedTransitionLayout`, a lookahead scope, so the sheet is + * measured twice a frame. `AnimatedContent` with a [SizeTransform] reports its target height to + * the lookahead pass and the animated height to the main pass, and compose-unstyled's `Sheet` + * writes each into its state during measure. While the sheet moves, the two writes keep + * invalidating each other and the frame never ends: the host Invite sheet's ANR, when a group's + * Approve New Members loads as on just after the sheet appears. + * + * A regression hangs rather than failing an assertion: the loop spins inside the sheet's own + * measure and never returns here. The module's test task timeout turns that into a failure. + */ +@RunWith(RobolectricTestRunner::class) +class WrapContentSheetLayoutLoopTest { + + @get:Rule + val composeTestRule = createComposeRule() + + @Test + fun `content that animates its height while the sheet opens does not spin layout`() { + var tall by mutableStateOf(false) + val sheetEntry = NavEntry(key = DemoSheet) { + AnimatedContent( + targetState = tall, + modifier = Modifier.fillMaxWidth(), + transitionSpec = { + fadeIn(tween(300)) togetherWith fadeOut(tween(300)) using + SizeTransform(clip = false) { _, _ -> tween(300) } + }, + label = "test", + ) { isTall -> + Box(Modifier.fillMaxWidth().height(if (isTall) 240.dp else 160.dp)) + } + } + val baseEntry = NavEntry(key = AppHome) { Box(Modifier.fillMaxSize()) } + val scene = ModalBottomSheetScene( + key = DemoSheet, + previousEntries = listOf(baseEntry), + overlaidEntries = listOf(baseEntry), + entry = sheetEntry, + sheetProperties = BottomSheetProperties(), + onBack = {}, + metadata = mapOf(NavMetadataKeys.IsWrapContentSheet.key to true), + navResultStore = EmptyCodeNavigator.resultStore, + lastNavKey = { AppHome }, + ) + + composeTestRule.mainClock.autoAdvance = false + composeTestRule.setContent { + DesignSystem { + CompositionLocalProvider( + LocalCodeNavigator provides testNavigator(AppHome, DemoSheet), + LocalScrimController provides ScrimController(), + ) { + SharedTransitionLayout { scene.content() } + } + } + } + + // Partway up, the content learns it is taller, as the host sheet does when the group's + // Approve New Members setting arrives. + composeTestRule.mainClock.advanceTimeBy(64) + tall = true + repeat(60) { composeTestRule.mainClock.advanceTimeByFrame() } + composeTestRule.mainClock.autoAdvance = true + composeTestRule.waitForIdle() + } +} diff --git a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt index 7234527a52..ca673a410a 100644 --- a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt +++ b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt @@ -44,6 +44,8 @@ class InWindowSheetState internal constructor( ) { internal lateinit var sheet: BottomSheetState + internal val wrapContent = WrapContentDetent() + /** True once [hide] has been asked for: the caller closed it, so no dismiss request follows. */ internal var hidingByCaller by mutableStateOf(false) private set @@ -61,7 +63,7 @@ fun rememberInWindowSheetState(isLocked: () -> Boolean = { false }): InWindowShe val state = remember { InWindowSheetState { locked.value() } } state.sheet = rememberBottomSheetState( initialDetent = SheetDetent.Hidden, - detents = listOf(SheetDetent.Hidden, WrapContent), + detents = listOf(SheetDetent.Hidden, state.wrapContent.detent), confirmDetentChange = { detent -> detent != SheetDetent.Hidden || state.hidingByCaller || !state.isLocked() }, @@ -95,7 +97,8 @@ fun InWindowBottomSheet( val scope = rememberCoroutineScope() val currentOnDismiss by rememberUpdatedState(onDismissRequest) - LaunchedEffect(sheet) { sheet.animateTo(WrapContent) } + LaunchedEffect(sheet) { sheet.animateTo(state.wrapContent.detent) } + state.wrapContent.Track(sheet) // [UnstyledBottomSheet] has no dismissal callback: watch for it settling Hidden after it was // shown, the way the navigation sheets do, and ignore a close the caller asked for itself. @@ -119,7 +122,7 @@ fun InWindowBottomSheet( modifier = Modifier .fillMaxSize() .drawBehind { - val progress = sheet.progress(SheetDetent.Hidden, WrapContent).coerceIn(0f, 1f) + val progress = sheet.progress(SheetDetent.Hidden, state.wrapContent.detent).coerceIn(0f, 1f) drawRect(scrimColor.copy(alpha = scrimColor.alpha * progress)) } .then(if (dismissOnScrimTap) Modifier.noRippleClickable(onClick = requestClose) else { @@ -138,7 +141,9 @@ fun InWindowBottomSheet( .clip(RoundedCornerShape(topStart = 28.dp, topEnd = 28.dp)) .background(containerColor), ) { - content() + Box(with(state.wrapContent) { Modifier.fillMaxWidth().content() }) { + content() + } } } } diff --git a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt index d50c80772c..ffb3b8a741 100644 --- a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt +++ b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt @@ -71,18 +71,6 @@ private val Expanded = SheetDetent("expanded") { containerHeight, _ -> containerHeight * 0.925f } -/** - * Resting detent for a [com.getcode.navigation.WrapContentSheet]: the height its content asked for, - * never more than [Expanded] would have given it. - * - * The detent is what sizes the sheet — compose-unstyled lays the sheet out to the detent height, not - * to the content — so dropping `fillMaxHeight()` from the sheet body is not enough on its own. This - * is the half of wrap-content that makes the sheet hug three lines instead of claiming the screen. - */ -internal val WrapContent = SheetDetent("wrap-content") { containerHeight, sheetHeight -> - sheetHeight.coerceAtMost(containerHeight * 0.925f) -} - /** * Resting — and, for short content, only — detent for a [com.getcode.navigation.HalfSheet]. * @@ -176,10 +164,13 @@ internal class ModalBottomSheetScene constructor( val isHalfSheet = metadata[NavMetadataKeys.IsHalfSheet.key] as? Boolean ?: false val isFullscreen = metadata[NavMetadataKeys.IsFullscreenSheet.key] as? Boolean ?: false + // Sized from the content's recorded height, never from the sheet's own measure; + // see [WrapContentDetent]. + val wrapContent = rememberWrapContentDetent() val restingDetent = when { isHalfSheet -> Half isFullscreen -> Fullscreen - isWrapContent -> WrapContent + isWrapContent -> wrapContent.detent else -> Expanded } @@ -205,6 +196,8 @@ internal class ModalBottomSheetScene constructor( }, ) + if (isWrapContent && !isHalfSheet) wrapContent.Track(sheetState) + // Offer the expanded detent only while there is something below the fold to expand into, // so a sheet that already fits can't be dragged up into empty space. Never withdraw it // while the sheet is sitting at (or heading toward) it. @@ -388,6 +381,8 @@ internal class ModalBottomSheetScene constructor( .then( if (!isWrapContent) { Modifier.fillMaxHeight() + } else if (!isHalfSheet) { + with(wrapContent) { Modifier.content() } } else Modifier ) // Every other sheet rests below the status bar and so never had diff --git a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt new file mode 100644 index 0000000000..71ed4b3e1d --- /dev/null +++ b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt @@ -0,0 +1,89 @@ +package com.getcode.navigation.scenes + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.Stable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.runtime.snapshotFlow +import androidx.compose.ui.Modifier +import androidx.compose.ui.layout.layout +import androidx.compose.ui.layout.onSizeChanged +import androidx.compose.ui.platform.LocalDensity +import androidx.compose.ui.platform.LocalWindowInfo +import androidx.compose.ui.unit.Dp +import androidx.compose.ui.unit.constrainHeight +import androidx.compose.ui.unit.dp +import androidx.compose.ui.unit.isSpecified +import com.composeunstyled.BottomSheetState +import com.composeunstyled.SheetDetent +import kotlinx.coroutines.flow.drop +import kotlin.math.roundToInt + +/** The most of its container a wrap-content sheet may take: the same as [Expanded]. */ +internal const val WrapContentMaxFraction = 0.925f + +/** + * Resting detent for a sheet that hugs its content: the height the content asked for, never more + * than [WrapContentMaxFraction] of the container. + * + * compose-unstyled lays a sheet out to its detent height, not to the content. A detent that + * returns the `sheetHeight` the library hands it puts the sheet in the library's content-height + * mode, where the sheet's measure writes the content's height into its state and reads it back in + * the same pass. Every scene sits in a `SharedTransitionLayout`, so the sheet is measured twice a + * frame; content whose height animates (an `AnimatedContent` with a `SizeTransform`) reports one + * height to the lookahead pass and another to the main pass, each write invalidates the other, + * and the frame never finishes. That was the host Invite sheet's ANR. + * + * This detent never reads `sheetHeight`, so the library treats it as fixed. [content] records the + * content's own height after layout, and [Track] re-resolves the detents when it changes, a frame + * later rather than inside the measure pass. + */ +@Stable +internal class WrapContentDetent { + private var contentHeight by mutableStateOf(Dp.Unspecified) + + val detent = SheetDetent("wrap-content") { containerHeight, _ -> + val max = containerHeight * WrapContentMaxFraction + // Unmeasured (or measured against a window that had no size yet), it opens toward the + // cap; the first real layout pulls it in to the content. Zero would sit on Hidden. + if (contentHeight.isSpecified && contentHeight > 0.dp) contentHeight.coerceAtMost(max) else max + } + + /** + * Measures the sheet's content at its natural height, up to the cap, and records it. The + * content is laid out at the top of whatever height the sheet currently has, so it is clipped + * while the sheet catches up with it rather than squeezed. + */ + @Composable + fun Modifier.content(): Modifier { + val density = LocalDensity.current + val maxHeightPx = (LocalWindowInfo.current.containerSize.height * WrapContentMaxFraction) + .roundToInt() + return layout { measurable, constraints -> + val placeable = measurable.measure( + constraints.copy(minHeight = 0, maxHeight = maxHeightPx.coerceAtLeast(0)) + ) + layout(placeable.width, constraints.constrainHeight(placeable.height)) { + placeable.place(0, 0) + } + }.onSizeChanged { contentHeight = with(density) { it.height.toDp() } } + } + + /** Moves [state]'s anchors whenever the content's recorded height changes. */ + @Composable + fun Track(state: BottomSheetState) { + LaunchedEffect(state) { + // The first value is whatever the sheet's opening measure already resolved; only + // later changes need the anchors moved. + snapshotFlow { contentHeight } + .drop(1) + .collect { state.invalidateDetents() } + } + } +} + +@Composable +internal fun rememberWrapContentDetent(): WrapContentDetent = remember { WrapContentDetent() }