google / accompanist

A collection of extension libraries for Jetpack Compose
https://google.github.io/accompanist
Apache License 2.0
7.38k stars 596 forks source link

[Navigation Material] bottomSheet() onSheetShown and onSheetDismissed issues #1644

Closed newmanw closed 11 months ago

newmanw commented 1 year ago

Description Noticed after upgrade from 0.28.0 to 0.30.1 navigation stack was not being popped correctly causing issues with state,

Steps to reproduce open a few bottom sheets and click on scrim to close

Expected behavior Digging into the code I think this is what is happening, but not an expert here.

onSheetShown should be called here for every sheet, in some cases this is not called

https://github.com/google/accompanist/blob/main/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt#L227

When onSheetShown is not called, the transitionInProgressEntries still contains an entry and marks the transition as complete without pop'ing the navigation stack.

https://github.com/google/accompanist/blob/main/navigation-material/src/main/java/com/google/accompanist/navigation/material/BottomSheetNavigator.kt#L232

Additional context I need to know when the bottom sheet is dismissed on scrim click, as such I am relying on the navigation stack to change to detect this. When I click on the scrim the navigation stack is no longer changed to route/content that was under the scrim.

spectrl commented 1 year ago

I think this may be an issue with ModalBottomSheetLayout in Compose Material 1.5.0-beta01 because I've noticed some issues with the scrim staying displayed (but the sheet dismissing), and also weird back button behaviour as well since updating...

newmanw commented 1 year ago

I'm seeing this in 1.4.x. Was working in 1.3.x, trying to downgrade now to confirm.

newmanw commented 1 year ago

If it helps, this is how I am verifying.

val navController = rememberNavController(bottomSheetNavigator)
navController.addOnDestinationChangedListener { _: NavController, destination: NavDestination, _: Bundle? ->
   Log.i("Debug", "navigation destination changed $destination")
}

I am not seeing the navigation destination change on scrim tap. The bottom sheet does dismiss.

newmanw commented 1 year ago

Downgrading to compose 1.3.3 and accompanist 0.28.0 fixes the issue. This also requires a downgrade of kotlin and compose material3. Which is not optimal.

If it helps upgrading to compose 1.5 beta and accompanist 0.31.3 beta does not help either.

newmanw commented 1 year ago

This seems to be an issue with no bottom sheet content, 0 height until content is ready. I have forked the repo and modified the bottomsheet sample that shows the issue with the above logging I mentioned. Simply just added some live data to delay loading the bottom sheet content.

Latest commit on the main branch.

https://github.com/newmanw/accompanist/commits/main

tymen-schiphol commented 1 year ago

I've encountered this same issue.

github-actions[bot] commented 1 year ago

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.

Mwhite69 commented 1 year ago

What can i do now?

On Thu, Aug 10, 2023, 11:19 PM github-actions[bot] @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/google/accompanist/issues/1644#issuecomment-1674165626, or unsubscribe https://github.com/notifications/unsubscribe-auth/A6VPCDUSZ53IBRIKMPB7V6LXUWQEHANCNFSM6AAAAAAYTGHRLM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

github-actions[bot] commented 11 months ago

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.

paktalin commented 7 months ago

still encountering the same issue with accompanist 0.32.0 and compose material 1.5.4. It is indeed caused by the bottom sheet having 0 height at first

chriswiesner commented 7 months ago

issue is still happening - any way to workaround that?

newmanw commented 7 months ago

As a workaround you wrap the dynamic content in something that has a height. Not optimal if you want your height to be dynamic based on content. You could resize after data is loaded.

Instead of:

@Composable
fun BottomSheet() {
    val viewModel: BottomSheetViewModel = viewModel()
    val isLoading by viewModel.isLoading.collectAsState()
    viewModel.load()

    isLoading?.let {
        Column(Modifier.fillMaxWidth()) {
         ...
        }
    }
}
@Composable
fun BottomSheet() {
    val viewModel: BottomSheetViewModel = viewModel()
    val isLoading by viewModel.isLoading.collectAsState()
    viewModel.load()

    Column(Modifier.height(400.dp)) {
        isLoading?.let {
           // Your content here
        }
     }
}