[Navigation] sheet destination not popped when rapidly dismissing sheet

[ind1go]

Description

I have a layout where a FAB triggers the bottom sheet, and then disappears so as not to cover it. When the sheet is dismissed, the button reappears (see the video under 'expected behaviour').

This works most of the time, however if the sheet is dismissed at a particular moment shortly after it's started, it seems that the the currentBackStackEntryAsState state isn't updated back to the prior destination, and therefore the action to show the button isn't run. There appears to be a race condition.

Video of the failing case:

https://user-images.githubusercontent.com/1038350/154358665-7326cd6e-6a35-485c-b0c3-65fbf9313a2b.mp4

Steps to reproduce

1. Press the button to switch to the sheet and hide the button.
2. Press the scrim, just about at the time when the sheet animation stops.
3. Observe that sheet is dismissed, but the button has not reappeared.

Also, in terms of a bit of troubleshooting...

4. If I add a println as follows:

   val navBackStackEntry by navController.currentBackStackEntryAsState() // existing line
   println("Route: ${navBackStackEntry?.destination?.route}") // new println

   within my MainLayout, then in the erroneous case it ends on SHEET. The HOME navigation does not appear to be triggered.

Full code below.

import android.os.Bundle
import androidx.activity.ComponentActivity
import androidx.activity.compose.setContent
import androidx.compose.material.icons.Icons
import androidx.compose.material.icons.filled.Star
import androidx.compose.material3.*
import androidx.compose.runtime.*
import androidx.compose.ui.graphics.vector.rememberVectorPainter
import androidx.compose.ui.unit.sp
import androidx.navigation.NavHostController
import androidx.navigation.compose.NavHost
import androidx.navigation.compose.composable
import androidx.navigation.compose.currentBackStackEntryAsState
import androidx.navigation.compose.rememberNavController
import com.google.accompanist.navigation.material.*

class TestActivity2 : ComponentActivity() {

    @OptIn(ExperimentalMaterialNavigationApi::class, ExperimentalMaterial3Api::class)
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            val bottomSheetNavigator = rememberBottomSheetNavigator()
            val navController = rememberNavController(bottomSheetNavigator)
            var fabVisible by remember { mutableStateOf(true) }

            Scaffold(
                floatingActionButton = {
                    if (fabVisible) {
                        FloatingActionButton(onClick = {
                            navController.navigate(Destinations.SHEET.name)
                        }) {
                            Icon(
                                painter = rememberVectorPainter(image = Icons.Default.Star),
                                contentDescription = null
                            )
                        }
                    }
                },
            ) {
                MainLayout(
                    bottomSheetNavigator = bottomSheetNavigator,
                    navController = navController,
                    onShowSheet = { fabVisible = false },
                    onHideSheet = { fabVisible = true },
                )
            }
        }
    }

    @OptIn(ExperimentalMaterialNavigationApi::class)
    @Composable
    fun MainLayout(
        bottomSheetNavigator: BottomSheetNavigator,
        navController: NavHostController,
        onShowSheet: () -> Unit = {},
        onHideSheet: () -> Unit = {}
    ) {
        val navBackStackEntry by navController.currentBackStackEntryAsState()
        if (navBackStackEntry?.destination?.route == Destinations.SHEET.name) {
            onShowSheet()
        } else {
            onHideSheet()
        }
        ModalBottomSheetLayout(bottomSheetNavigator) {
            NavHost(navController, Destinations.HOME.name) {
                composable(route = Destinations.HOME.name) {
                    Text("This is the home.", fontSize = 16.sp)
                }
                bottomSheet(route = Destinations.SHEET.name) {
                    Text("This is the bottom sheet.", fontSize = 16.sp)
                }
            }
        }
    }

    private enum class Destinations {
        HOME, SHEET
    }
}

Expected behavior

The logic to re-enable the button is triggered, regardless of how quickly I dismiss the sheet.

https://user-images.githubusercontent.com/1038350/154358541-1af846e4-96d0-4d17-9bff-bbab977ae450.mp4

Additional context

Compose version 1.2.0-alpha03 Accompanist version 0.24.2-alpha Android S (Pixel 3a) and Android S (Nexus 5 emulator)

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[jossiwolf]

Hi @ind1go, thank you! Yes, Swipeable and ModalBottomSheetLayout which we use have a 1-frame delay, resulting in a race condition. We are doing some work upstream but this is dependent on a bigger redesign of the underlying APIs. For now, we can't offer a good solution for this issue, but I will provide any updates when I can.

[ind1go]

Thanks for the update!

[NG-Gaurav]

Any update on this issue.

[NG-Gaurav]

??

[jossiwolf]

Upstream issue: https://issuetracker.google.com/issues/167966118

[gregkorossy]

Since this is marked fixed in the upstream issue, is this getting fixed here too?

[jossiwolf]

Yep, we will work on these fixes when the changes are released in January :)

[jossiwolf]

v0.28.1 addresses this issue. Please let us know if you can still reproduce it with this version.

[gregkorossy]

There's no v0.28.1 release and v0.29.0-alpha doesn't contain this fix (or if it does, it's not working).

[jossiwolf]

Apologies, the fix is in 0.29.0-alpha. Can you please provide a repro? Is it OP's repro?

[gregkorossy]

Created a sample project that demonstrates the issue still being present: https://github.com/Gericop/BottomSheetBugSample

To reproduce it, tap on the button and as soon as the bottom sheet starts opening, tap it again (so dismiss the bottom sheet while opening). You'll notice how the text below the button will say Back stack last: sheet while the sheet is not visible anymore. Tapping the back button on your device will pop the bottom sheet and the displayed text will be Back stack last: home.

Recorded a video to show it:

https://user-images.githubusercontent.com/5181621/212745673-d0466437-6354-4801-8911-b9b542179353.mp4

[jossiwolf]

Thanks, I can repro with this, although very inconsistently. Have you seen any steps that make it more likely to repro this issue?

[gregkorossy]

@jossiwolf You can replace the

val bottomSheetNavigator = rememberBottomSheetNavigator()
val navController = rememberNavController(bottomSheetNavigator)

with

val sheetState = rememberModalBottomSheetState(
    initialValue = ModalBottomSheetValue.Hidden,
    animationSpec = SwipeableDefaults.AnimationSpec
)

val bottomSheetNavigator = remember { BottomSheetNavigator(sheetState) }
val navController = rememberNavController(bottomSheetNavigator)

LaunchedEffect(Unit) {
    snapshotFlow {
        bottomSheetNavigator.navigatorSheetState.targetValue
    }
        .filter { it != ModalBottomSheetValue.Hidden }
        .collectLatest {
            // the delay could be removed and it will still reproduce the issue
            // it's just nice to see the animation happening
            delay(100)

            sheetState.hide()
        }
}

This way the bottom sheet will be dismissed 100ms after the opening animation starts and it will reproduce the issue all the time. (You can actually remove the delay as it will still end up in a bad state, but it's nice to see the animation happening.)

P.S.: Updated the repo as well to make it easier to reproduce the issue.

[jossiwolf]

Thanks!

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[gregkorossy]

@jossiwolf Any updates on this bug?

[spun]

I've started noticing this issue after changing the "Animator duration scale" in developer options to test some animations.

Using "Animation scale 10x", I am able to reproduce the bug if I click the scrim before the bottomSheet enter animation completes. If inconsistency was still a problem, maybe this could help.

https://user-images.githubusercontent.com/1004332/228764522-abc51b16-49f3-442d-8840-b7c3a00adac6.mp4

The FAB doesn't reappear until I do the back gesture

Compose bom: 2023.03.00
Accompanist version: 0.30.0

---

After a bit of research, I've noticed that SheetContentHost uses isVisible from ModalBottomSheetState to "notify" BottomSheetNavigator when the bottomSheet is shown or dismissed.

isVisible uses swipeableState.currentValue, and this value (unlike targetValue) doesn't change until the animations are completed.

If we click the scrim before the "show" animation ends, currentValue and isVisible will never change from their initial values and the content of onSheetShown/onSheetDismissed (with the pop call) will not be executed.

Sorry if this is incorrect or doesn't add any new info.

[jossiwolf]

Thanks, that is very helpful!

[gabrielittner]

I've seen something similar to what @spun described. onSheetShown isn't called which means the transition never gets marked as completed through markTransitionComplete. Then onSheetDismissed is called and will mark the transition as comleted instead of popping the bottom sheet destination. However for me this behavior happens even after the animation is finished when the sheet is dismissed by clicking the scrim or by swiping it down.

[ind1go]

At this point, is the issue requiring an upstream fix, or is the bug in accompanist?

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[gregkorossy]

Any update on this issue?

[ind1go]

Is this really closed as completed? I don't see any related changes. I feel like the bot just ignored someone commenting.

[ind1go]

@jossiwolf please could you reopen this? It appears to have been closed in error.

[ivanosh]

@jossiwolf issue still exists