adrielcafe / voyager

🛸 A pragmatic navigation library for Jetpack Compose
https://voyager.adriel.cafe
MIT License
2.48k stars 124 forks source link

BottomSheet closes when recomposing #330

Open jordond opened 6 months ago

jordond commented 6 months ago

I have a BottomSheet that when launched, allows the user to choose a color to base the app theme on. When a color is selected a new theme is generated and the colors in the app change.

When this happens the BottomSheet gets dismissed.

Here is some relevant code:

val screenModel = getScreenModel<SettingsModel>()
val state by screenModel.collectAsState()

val navigator = LocalNavigator.currentOrThrow

BottomSheetNavigator(
    sheetShape = MaterialTheme.shapes.large,
) { bottomSheetNavigator ->
    SettingsScreenContent(
        settings = state.settings,
        updateSettings = screenModel::update,
        onAction = { action ->
            when (action) {
                is Close -> navigator.pop()
                is ChangeTheme -> bottomSheetNavigator.show(DynamicThemeSheet())
            }
    )
}

And a video of it happening:

https://github.com/adrielcafe/voyager/assets/528792/328b683c-8aa4-4909-9c77-f129be137801

Kashifmasood07 commented 6 months ago

@jordond I am also facing the similar issue with textfield, were you able to solve it?

Kashif-E commented 6 months ago

for me its only happening on ios not on android

Kashif-E commented 6 months ago

@DevSrSouza can you please have a look at this? for me it happens only in ios and after updating to compose 1.6

hm-tamim commented 5 months ago

Same here, only in iOS. It closes when TextField is focused and keyboard opens up. Compose 1.6

hm-tamim commented 5 months ago

It seems the culprit is confirmValueChange in rememberModalBottomSheetState, it's using rememberSaveable where confirmValueChange is used as a key. When this method changes in iOS somehow when you open keyboard or any other thing that causes recomposition, it recreates the sheet state, which hides the bottom sheet modal.

Removing confirmValueChange and using LaunchedEffect to observe sheetState.currentValue solves the issue.

Let me raise a PR for this.

DevSrSouza commented 4 months ago

The PR from @hm-tamim was released on 1.0.0-alpha04. Try out and let us know if the issue is fixed.

Qw4z1 commented 4 months ago

@DevSrSouza I had the same issue after updating compose, and can confirm that the issue is fixed in 1.1.0-alpha04. Big thanks to @hm-tamim!

Kashif-E commented 4 months ago

@DevSrSouza it's fixed with @hm-tamim's contribution but there is a case, open any bottom sheet with a text field, keep the text field in focus and navigate to another screen pop the screen and in some cases backstack is cleared