Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Navigation Material] Fix Bottom Sheet Fully Expanding #742

Merged
merged 2 commits into from Sep 30, 2021
Merged

[Navigation Material] Fix Bottom Sheet Fully Expanding #742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
acae2b1
Reliably mark showing sheet's back stack entry as complete
jossiwolf Sep 28, 2021
2eb346e
Fix tall sheet fully expanding when navigating
jossiwolf Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
94 changes: 85 additions & 9 deletions ...l/src/androidTest/java/com/google/accompanist/navigation.material/SheetContentHostTest.kt
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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<NavBackStackEntry?>(null)
val shownBackStackEntries = mutableListOf<NavBackStackEntry>()

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()
Expand All @@ -157,6 +170,62 @@ internal class SheetContentHostTest {
}
}

@Test
fun testSheetHalfExpanded_onBackStackEntryEnter_shortSheet(): Unit = runBlocking {
val sheetState = ModalBottomSheetState(ModalBottomSheetValue.Hidden)
val backStackEntryState = mutableStateOf<NavBackStackEntry?>(null)
val shownBackStackEntries = mutableListOf<NavBackStackEntry>()

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<NavBackStackEntry?>(null)
val shownBackStackEntries = mutableListOf<NavBackStackEntry>()

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<NavBackStackEntry?>,
sheetState: ModalBottomSheetState,
Expand All @@ -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
Expand Down
73 changes: 62 additions & 11 deletions ...ion-material/src/main/java/com/google/accompanist/navigation/material/SheetContentHost.kt
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -58,15 +64,15 @@ import kotlinx.coroutines.launch
* pop the back stack here.
*/
@ExperimentalMaterialNavigationApi
@OptIn(ExperimentalMaterialApi::class)
@OptIn(ExperimentalMaterialApi::class, ExperimentalCoroutinesApi::class)
@Composable
internal fun SheetContentHost(
columnHost: ColumnScope,
backStackEntry: NavBackStackEntry?,
sheetState: ModalBottomSheetState,
saveableStateHolder: SaveableStateHolder,
onSheetShown: (entry: NavBackStackEntry) -> Unit,
onSheetDismissed: (entry: NavBackStackEntry) -> Unit
onSheetDismissed: (entry: NavBackStackEntry) -> Unit,
) {
val scope = rememberCoroutineScope()
if (backStackEntry != null) {
Expand All @@ -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<Unit?>()
}

// 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)
}
}
Expand All @@ -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()
Expand All @@ -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