accompanist icon indicating copy to clipboard operation
accompanist copied to clipboard

BottomSheet route is not displayed

Open nglauber opened this issue 2 years ago • 9 comments

Describe the bug

Bottom sheet route is not displayed using a combination of LaunchedEffect, flow.collectAsState() and Text.

To Reproduce

Steps to reproduce the behavior:

  1. Declare a bottomSheet route inside of the AnimatedNavHost
ModalBottomSheetLayout(bottomSheetNavigator) {
    AnimatedNavHost(...) {
        bottomSheet("bottomSheetNav") { BottomSheetViaRoute(it) }
    }
}
  1. Navigate to this route using:
navController.navigate("bottomSheetNav")
  1. The composable function BottomSheetViaRoute should be like this:
@Composable
fun BottomSheetViaRoute(
    navBackStackEntry: NavBackStackEntry
) {
    val viewModel: MyViewModel = viewModel(navBackStackEntry)
    val list by viewModel.listFlow.collectAsState(emptyList())
    LaunchedEffect(Unit) {
        viewModel.loadList()
    }
    Column(Modifier.verticalScroll(rememberScrollState())) {
        //Text(text = "BottomSheet via route", style = MaterialTheme.typography.h3)
        list.forEach {
            Text(
                text = it,
                Modifier
                    .padding(16.dp)
                    .fillMaxWidth()
            )
        }
    }
}
  1. Notice the Text is commented...
  2. Run this code and it should work as expected.
  3. Now, uncomment the call to the Text function.

Expected behavior

The bottom sheet should be displayed with the Text on top of the items.

Current behavior

Nothing happens... :/

Screenshots?

N/A

Environment:

  • Android OS version: 8.0
  • Device: Motorola moto Z2 Play
  • Accompanist version: 0.19.0

Additional context

The full code of the BottomSheetViaRoute function can be found here. The full code of the MyViewModel class can be found here.

I also tried comment the list.forEach call. And it worked... I mean, only Text and only list.forEach work, but not both...

nglauber avatar Oct 05 '21 02:10 nglauber

Thanks for the report! This is due to an underlying synchronization issue with the Material APIs that's not trivial to fix. The issue here lies in the sheet content changing while the sheet is still animating. The really dirty workaround, for now, is to add something like a delay(40) to your launched effect... :/ We're looking into ways to re-write this asap.

jossiwolf avatar Oct 05 '21 09:10 jossiwolf

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.

github-actions[bot] avatar Nov 06 '21 03:11 github-actions[bot]

I believe I've hit this too, except in my case I'm loading images like so:

bottomSheet(...) {
  Image(
    painter = rememberImagePainter(...), // from Coil
  )
}

If I change this code to provide a Modifier.height(), then the problem goes away:

bottomSheet(...) {
  Image(
    painter = rememberImagePainter(...),
    modifier = Modifier.height(400.dp),
  )
}

Some other observations:

  • Accompanist 0.18.0 doesn't have this problem.
  • Even though navigation "fails" and the bottom sheet never opens, the bottom sheet destination is added to the back stack.
  • Sometimes the bottom sheet DOES load. When this occurs, and if there was a previous failed attempt to navigate (as in the above point) dismissing the bottom sheet just pops you to the same bottom sheet. You need to dismiss perhaps several times to return to where you want to go.

I have code that reproduces it here:

https://github.com/chrisfillmore/accompanist_coil_bottom_sheet_test/blob/3a1e8d16cd12ba8ccbca7fa158634272ad33ea26/app/src/main/java/ca/chrisfillmore/accompanistbottomsheettest/MainActivity.kt#L39-L117

chrisfillmore avatar Nov 16 '21 20:11 chrisfillmore

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.

github-actions[bot] avatar Feb 02 '22 03:02 github-actions[bot]

After some debugging, what's going on is that ModalBottomSheetState.show() is throwing in SwipeableState.animateTo():

val targetOffset = anchors.getOffset(targetValue)
requireNotNull(targetOffset) {
  "The target value must have an associated anchor."
}

The error is being swallowed because it is thrown in a Flow.collect() block, which cancels the containing CoroutineScope without passing the error through.

This happens unless you set a fixed height on the content with Modifier.height(). heightIn() does not work, which makes me suspect that the SwipeableState anchors are still not computed by the time ModalBottomSheetState.show() is called. There may be some dependency between the content measurement and ModalBottomSheetState updating sheetHeightState through onGloballyPositioned.

tadfisher avatar May 25 '22 21:05 tadfisher

See https://issuetracker.google.com/issues/167966118 for the root cause. Feel free to track the progress there. We're addressing this in Swipeable itself.

jossiwolf avatar Sep 23 '22 16:09 jossiwolf

One more issues related to bottom sheet navigation using accompanist, we are unable to pass data back to parent composable (from where bottom sheet is routed to). We know its not a good way but still its one way to do, so by using previousBackStackEntry and putting arguments in its savedStateHandle, when bottom sheet is dismissed parent composable is not recomposing, thus unable to read the arguments.

navController.previousBackStackEntry?.savedStateHandle?.apply { set(DestinationArgs.otpStatus, PaymentStatus.SUCCESS) } navController.navigateUp()

hsaddique avatar Sep 24 '22 09:09 hsaddique

@jossiwolf

Thanks for the report! This is due to an underlying synchronization issue with the Material APIs that's not trivial to fix. The issue here lies in the sheet content changing while the sheet is still animating. The really dirty workaround, for now, is to add something like a delay(40) to your launched effect... :/ We're looking into ways to re-write this asap.

Having a fixed size on the sheet content fixes the issue with the bottomsheet not appearing. Unfortunately if I simultaneously have implemented @leinardi solution from #657 and setting skipHalfExpanded = true, the issue with the bottomsheet not appearing comes back again.

lilemma avatar Oct 06 '22 12:10 lilemma

Yes, having a fixed size will work around this :) For the skipHalfExpanded flag, see my comment on #657 on why this doesn't work right now.

jossiwolf avatar Oct 06 '22 14:10 jossiwolf

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

jossiwolf avatar Jan 12 '23 07:01 jossiwolf

Hi, I also have this problem on a slightly different setup. But I wanted to try to update to 0.28.1 to see if it fixed it but it does not seem to be available or published to maven central? Was there any issue publishing 0.28.1? When could we have a release including this fix? Thanks!

jrgonzalezg avatar Feb 24 '23 15:02 jrgonzalezg

Sorry, we renamed the version before publishing. It's v0.29.0-alpha.

jossiwolf avatar Feb 24 '23 17:02 jossiwolf