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.

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

Fix bottom sheet dismiss on click outside issue #224

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

DevNatan
Copy link
Collaborator

@DevNatan DevNatan commented Sep 29, 2023

Fixes #203
Fixes #221

isVisible is false when we click outside the bottom sheet so it is not being removed from the stack.

This happens because we are NOT vetoing bottom sheet state change so when it arrives in Voyager's hide() is already hiding. This is the solution for now, before the release I will look into this veto issue.

To reproduce

  1. Run samples:android and click "Bottom Sheet Navigation" button;
  2. Open logs, BasicNavigationScreen has a LifecycleEffect that logs onStarted/onDisposed.
  3. Click "Show BottomSheet"
    3.1. Swipe down: hidden, disposed and replaced by HiddenBottomSheet
    3.2. Add a button inside BasicNavigationScreen that calls bottomSheetNavigator.hide(), click on it: hidden, disposed and replaced by HiddenBottomSheet
    3.2. Click outside the bottom sheet: hidden, not disposed and still on stack (this should work now).

Don't add if (isVisible || sheetState.targetValue == ModalBottomSheetValue.Hidden) because hide() tries again (again because hide was already requested internally by outside click) to close the bottom sheet but outer CoroutineScope is already in a "near to cancel" state, only waiting animation to end so hide() throws a CancellationException and replaceAll do not get called at all

@DevNatan DevNatan added the bug Something isn't working label Sep 29, 2023
@DevNatan DevNatan self-assigned this Sep 29, 2023
@DevNatan DevNatan changed the title Fix bottom sheet click outside issue Fix bottom sheet dismiss on click outside issue Sep 29, 2023
@programadorthi
Copy link
Collaborator

Have you tried to remove the check for isVisible and always call sheetState.hide()?
Because it is a suspend function and calling it when animation is running or finished should do nothing than return immediatelly.

@DevNatan
Copy link
Collaborator Author

Have you tried to remove the check for isVisible and always call sheetState.hide()? Because it is a suspend function and calling it when animation is running or finished should do nothing than return immediatelly.

@programadorthi
Visibility check is here because if confirmValueChange optional parameter is present (and returns true) in rememberModalBottomSheetState it understands that Voyager holds its own "is visible" bottom sheet state (that's not the case) so when we call sheetState.hide() it ignores its own internal sheetState visibility state and calls confirmValueChange infinitely with ModalBottomSheetValue.Hidden expecting that Voyager will take care of the whole mechanism, and we get a voyagerBottomSheet.hide() -> sheetState.hide() -> confirmValueChange -> voyagerBottomSheet.hide() -> sheetState.hide() stack overflow.

ModalBottomSheetState says that sheetState.hide() throws a exception if animation gets interrupted while running :/ I tried ignoring it but bottom sheet stopped working at all 🤡

@DevNatan DevNatan merged commit 1667c0b into main Oct 4, 2023
1 check passed
@DevNatan DevNatan deleted the fix/hidden-bottom-sheet-replacement branch October 4, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bottomsheet dismiss issue Bottomsheet dismiss listener
2 participants