diff --git a/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/SheetContentHostTest.kt b/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/SheetContentHostTest.kt index 5b6952476..abd012d45 100644 --- a/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/SheetContentHostTest.kt +++ b/navigation-material/src/androidTest/java/com/google/accompanist/navigation.material/SheetContentHostTest.kt @@ -17,12 +17,15 @@ package com.google.accompanist.navigation.material import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.ColumnScope import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.height import androidx.compose.material.ExperimentalMaterialApi import androidx.compose.material.ModalBottomSheetLayout import androidx.compose.material.ModalBottomSheetState import androidx.compose.material.ModalBottomSheetValue import androidx.compose.material.Text +import androidx.compose.runtime.Composable import androidx.compose.runtime.State import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.saveable.rememberSaveableStateHolder @@ -33,6 +36,7 @@ import androidx.compose.ui.test.junit4.ComposeContentTestRule import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.performClick +import androidx.compose.ui.unit.dp import androidx.navigation.NavBackStackEntry import androidx.navigation.testing.TestNavigatorState import androidx.test.ext.junit.runners.AndroidJUnit4 @@ -41,6 +45,7 @@ import com.google.common.truth.Truth.assertThat import com.google.common.truth.Truth.assertWithMessage import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.TestCoroutineDispatcher import kotlinx.coroutines.test.runBlockingTest import org.junit.Rule @@ -49,7 +54,11 @@ import org.junit.runner.RunWith @LargeTest @RunWith(AndroidJUnit4::class) -@OptIn(ExperimentalCoroutinesApi::class, ExperimentalMaterialApi::class, ExperimentalMaterialNavigationApi::class) +@OptIn( + ExperimentalCoroutinesApi::class, + ExperimentalMaterialApi::class, + ExperimentalMaterialNavigationApi::class +) internal class SheetContentHostTest { private val testDispatcher = TestCoroutineDispatcher() @@ -137,17 +146,21 @@ internal class SheetContentHostTest { @Test fun testOnSheetShownCalled_onBackStackEntryEnter() = runBlockingTest(testClock) { val sheetState = ModalBottomSheetState(ModalBottomSheetValue.Hidden) - val backStackEntry = createBackStackEntry(sheetState) - + val backStackEntryState = mutableStateOf(null) val shownBackStackEntries = mutableListOf() composeTestRule.setBottomSheetContent( - backStackEntry = mutableStateOf(backStackEntry), + backStackEntry = backStackEntryState, sheetState = sheetState, onSheetShown = { entry -> shownBackStackEntries.add(entry) }, onSheetDismissed = { } ) + val backStackEntry = createBackStackEntry(sheetState) { + Box(Modifier.height(50.dp)) + } + backStackEntryState.value = backStackEntry + composeTestRule.runOnIdle { assertWithMessage("Sheet is visible") .that(sheetState.isVisible).isTrue() @@ -157,6 +170,62 @@ internal class SheetContentHostTest { } } + @Test + fun testSheetHalfExpanded_onBackStackEntryEnter_shortSheet(): Unit = runBlocking { + val sheetState = ModalBottomSheetState(ModalBottomSheetValue.Hidden) + val backStackEntryState = mutableStateOf(null) + val shownBackStackEntries = mutableListOf() + + composeTestRule.setBottomSheetContent( + backStackEntry = backStackEntryState, + sheetState = sheetState, + onSheetShown = { entry -> shownBackStackEntries.add(entry) }, + onSheetDismissed = { } + ) + + val backStackEntry = createBackStackEntry(sheetState) { + Box(Modifier.height(100.dp)) + } + backStackEntryState.value = backStackEntry + + composeTestRule.runOnIdle { + assertWithMessage("Sheet is fully expanded") + .that(sheetState.currentValue) + .isEqualTo(ModalBottomSheetValue.Expanded) + assertWithMessage("Back stack entry should be in the shown entries list") + .that(shownBackStackEntries) + .containsExactly(backStackEntry) + } + } + + @Test + fun testSheetHalfExpanded_onBackStackEntryEnter_tallSheet(): Unit = runBlocking { + val sheetState = ModalBottomSheetState(ModalBottomSheetValue.Hidden) + val backStackEntryState = mutableStateOf(null) + val shownBackStackEntries = mutableListOf() + + composeTestRule.setBottomSheetContent( + backStackEntry = backStackEntryState, + sheetState = sheetState, + onSheetShown = { entry -> shownBackStackEntries.add(entry) }, + onSheetDismissed = { } + ) + + val backStackEntry = createBackStackEntry(sheetState) { + Box(Modifier.fillMaxSize()) + } + backStackEntryState.value = backStackEntry + + composeTestRule.runOnIdle { + assertWithMessage("Tall sheet is half-expanded") + .that(sheetState.currentValue) + .isEqualTo(ModalBottomSheetValue.HalfExpanded) + assertWithMessage("Back stack entry should be in the shown entries list") + .that(shownBackStackEntries) + .containsExactly(backStackEntry) + } + } + private fun ComposeContentTestRule.setBottomSheetContent( backStackEntry: State, sheetState: ModalBottomSheetState, @@ -177,19 +246,26 @@ internal class SheetContentHostTest { ) }, sheetState = sheetState, - content = { Box(Modifier.fillMaxSize().testTag(bodyContentTag)) } + content = { + Box( + Modifier + .fillMaxSize() + .testTag(bodyContentTag) + ) + } ) } } - private fun createBackStackEntry(sheetState: ModalBottomSheetState): NavBackStackEntry { + private fun createBackStackEntry( + sheetState: ModalBottomSheetState, + sheetContent: @Composable ColumnScope.(NavBackStackEntry) -> Unit = { Text("Fake Sheet Content") } + ): NavBackStackEntry { val navigatorState = TestNavigatorState() val navigator = BottomSheetNavigator(sheetState) navigator.onAttach(navigatorState) - val destination = BottomSheetNavigator.Destination(navigator) { - Text("Fake Sheet Content") - } + val destination = BottomSheetNavigator.Destination(navigator, sheetContent) val backStackEntry = navigatorState.createBackStackEntry(destination, null) navigator.navigate(listOf(backStackEntry), null, null) return backStackEntry diff --git a/navigation-material/src/main/java/com/google/accompanist/navigation/material/SheetContentHost.kt b/navigation-material/src/main/java/com/google/accompanist/navigation/material/SheetContentHost.kt index 57a06f74c..5e8fafa09 100644 --- a/navigation-material/src/main/java/com/google/accompanist/navigation/material/SheetContentHost.kt +++ b/navigation-material/src/main/java/com/google/accompanist/navigation/material/SheetContentHost.kt @@ -33,15 +33,21 @@ import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.saveable.SaveableStateHolder import androidx.compose.runtime.setValue import androidx.compose.runtime.snapshotFlow +import androidx.compose.runtime.withFrameNanos import androidx.compose.ui.Modifier +import androidx.compose.ui.layout.onGloballyPositioned import androidx.compose.ui.unit.dp import androidx.navigation.NavBackStackEntry import androidx.navigation.compose.LocalOwnersProvider +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.drop import kotlinx.coroutines.flow.filter import kotlinx.coroutines.launch +import kotlinx.coroutines.withTimeout /** * Hosts a [BottomSheetNavigator.Destination]'s [NavBackStackEntry] and its @@ -58,7 +64,7 @@ import kotlinx.coroutines.launch * pop the back stack here. */ @ExperimentalMaterialNavigationApi -@OptIn(ExperimentalMaterialApi::class) +@OptIn(ExperimentalMaterialApi::class, ExperimentalCoroutinesApi::class) @Composable internal fun SheetContentHost( columnHost: ColumnScope, @@ -66,7 +72,7 @@ internal fun SheetContentHost( sheetState: ModalBottomSheetState, saveableStateHolder: SaveableStateHolder, onSheetShown: (entry: NavBackStackEntry) -> Unit, - onSheetDismissed: (entry: NavBackStackEntry) -> Unit + onSheetDismissed: (entry: NavBackStackEntry) -> Unit, ) { val scope = rememberCoroutineScope() if (backStackEntry != null) { @@ -86,20 +92,48 @@ internal fun SheetContentHost( .collect { if (!hideCalled) currentOnSheetDismissed(backStackEntry) } } - // Whenever the composable associated with the latestEntry enters the composition, we + // We use this signal to know when its (almost) safe to `show` the bottom sheet + // It will be set after the sheet's content has been `onGloballyPositioned` + val contentPositionedSignal = remember(backStackEntry) { + CompletableDeferred() + } + + // Whenever the composable associated with the backStackEntry enters the composition, we // want to show the sheet, and hide it when this composable leaves the composition DisposableEffect(backStackEntry) { scope.launch { - // Our show call can get cancelled in which case Swipeable will move to the closest - // anchor + contentPositionedSignal.await() try { + // If we don't wait for a few frames before calling `show`, we will be too early + // and the sheet content won't have been laid out yet (even with our content + // positioned signal). If a sheet is tall enough to have a HALF_EXPANDED state, + // we might be here before the SwipeableState's anchors have been properly + // calculated, resulting in the sheet animating to the EXPANDED state when + // calling `show`. As a workaround, we wait for a magic number of frames. + // https://issuetracker.google.com/issues/200980998 + repeat(AWAIT_FRAMES_BEFORE_SHOW) { awaitFrame() } sheetState.show() + } catch (sheetShowCancelled: CancellationException) { + // There is a race condition in ModalBottomSheetLayout that happens when the + // sheet content changes due to the anchors being re-calculated. This causes an + // animation to run for a short time, cancelling any currently running animation + // such as the one triggered by our `show` call. + // The sheet will still snap to the EXPANDED or HALF_EXPANDED state. + // In that case we want to wait until the sheet is visible. For safety, we only + // wait for 800 milliseconds - if the sheet is not visible until then, something + // has gone horribly wrong. + // https://issuetracker.google.com/issues/200980998 + withTimeout(800) { + while (!sheetState.isVisible) { + awaitFrame() + } + } } finally { - // If the target state is a visible state, it's fairly safe to assume that - // Swipeable will end up settling in that state - if (sheetState.targetValue == ModalBottomSheetValue.Expanded || - sheetState.targetValue == ModalBottomSheetValue.HalfExpanded - ) { + // If, for some reason, the sheet is in a state where the animation is still + // running, there is a chance that it is already targeting the EXPANDED or + // HALF_EXPANDED state and will snap to that. In that case we can be fairly + // certain that the sheet will actually end up in that state. + if (sheetState.isVisible || sheetState.willBeVisible) { currentOnSheetShown(backStackEntry) } } @@ -118,7 +152,9 @@ internal fun SheetContentHost( val content = (backStackEntry.destination as BottomSheetNavigator.Destination).content backStackEntry.LocalOwnersProvider(saveableStateHolder) { - columnHost.content(backStackEntry) + Box(Modifier.onGloballyPositioned { contentPositionedSignal.complete(Unit) }) { + columnHost.content(backStackEntry) + } } } else { EmptySheet() @@ -135,8 +171,23 @@ private fun EmptySheet() { Box(Modifier.height(1.dp)) } +private suspend fun awaitFrame() = withFrameNanos(onFrame = {}) + +/** + * This magic number has been chosen through painful experiments. + * - Waiting for 1 frame still results in the sheet fully expanding, which we don't want + * - Waiting for 2 frames results in the `show` call getting cancelled + * - Waiting for 3+ frames results in the sheet expanding to the correct state. Success! + * We wait for a few frames more just to be sure. + */ +private const val AWAIT_FRAMES_BEFORE_SHOW = 3 + // We have the same issue when we are hiding the sheet, but snapTo works better @OptIn(ExperimentalMaterialApi::class) private suspend fun ModalBottomSheetState.internalHide() { snapTo(ModalBottomSheetValue.Hidden) } + +@OptIn(ExperimentalMaterialApi::class) +private val ModalBottomSheetState.willBeVisible: Boolean + get() = targetValue == ModalBottomSheetValue.HalfExpanded || targetValue == ModalBottomSheetValue.Expanded