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

[Navigation Material] Fix Bottom Sheet Fully Expanding #742

Merged
merged 2 commits into from
Sep 30, 2021

Conversation

jossiwolf
Copy link
Collaborator

Kinda similar to #741 - we were calling show too early. That meant that the SwipeableState/ModalBottomSheetState's anchors wouldn't have been calculated properly yet. By default, a sheet will have two anchors: Hidden and Expanded. If sheetHeight > containerHeight / 2, the sheet should have a HalfExpanded state. For that, ModalBottomSheetLayout needs to know the sheet's height, making it two-pass. The effect we use to show the sheet got called on the first pass though, when the sheet content is composed to measure it, but the anchors didn't have the correct sheet height yet, causing the sheet to fully expand despite it being tall enough to have a HalfExpanded state.

This isn't a nice solution but in agreement with Matvei, it seems to be the most reasonable fix for the moment.

Our `show` calls are getting cancelled due to a race condition (https://issuetracker.google.com/issues/200980998) so we can't rely on their completion as a signal that the sheet is shown and we can mark the sheet's back stack entry's transition as complete. Using `withFrameNanos`, we try to wait until the sheet is visible.

Test: Existing tests still pass (as they did despite the race condition...). Will try to create tests that fail before the fix. Manual testing apart from that.
Fixes: google#725, google#720, google#700
@google-cla google-cla bot added the cla: yes label Sep 28, 2021
Calling `show` too early means that the SwipeableState's anchors won't have been properly calculated yet so we wait for a few frames.

Test: SheetContentHostTest#testSheetHalfExpanded_onBackStackEntryEnter_shortSheet, SheetContentHostTest#testSheetHalfExpanded_onBackStackEntryEnter_tallSheet
Fixes: google#717
Copy link
Collaborator

@ianhanniballake ianhanniballake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gross.

@chrisbanes chrisbanes merged commit 142d6c4 into google:main Sep 30, 2021
@jossiwolf jossiwolf deleted the sheet-fully-expands branch September 30, 2021 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants