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.

Sign up for GitHub

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

Jump to bottom

fix a draggableScrollableSheet's LocalHistoryEntry leaking #110576

Merged
merged 1 commit into from Sep 16, 2022
Merged

fix a draggableScrollableSheet's LocalHistoryEntry leaking #110576

merged 1 commit into from Sep 16, 2022

Conversation

xu-baolin
Copy link
Member

@xu-baolin xu-baolin commented Aug 30, 2022 •
edited

If the persistent bottom sheet is a draggableScrollableSheet and the user expand it, there will be a back button for a11y.
There are two situations that the local history entry may be leaked,
1, When user shrink the sheet we can not remove the entry due to the double calculations precision.
2, We do not remove the route entry when unmount the draggableScrollableSheet widget.

Fixes #110123

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 30, 2022
xu-baolin
xu-baolin commented Aug 30, 2022
View reviewed changes
packages/flutter/lib/src/material/scaffold.dart
if (notification.extent > notification.initialExtent) {
if (persistentSheetHistoryEntry == null) {
persistentSheetHistoryEntry = LocalHistoryEntry(onRemove: () {
if (notification.extent > notification.initialExtent) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This condition is always true, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would seem so.. I am out today and will take a closer look a little later. FYI @thkim1011, who is joining us in scrolling land. Tae has been studying the draggable scrollable sheet. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@thkim1011 Hi, nice to meet you : )

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, wouldn't this be true or false based on the direction (expanding/contracting) the sheet was going in?

if (notification.extent > notification.initialExtent)

Copy link
Member Author

Choose a reason for hiding this comment

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

This statement in the scope of onRemove callback and it should be never changed until it actually called, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Line #2189 already ensures that this condition is true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh silly me, I missed that.

@absar
Copy link

absar commented Sep 14, 2022

Isn't it true that, by definition persistent bottom sheets on Scaffold.bottomSheet should not create a LocalHistoryEntry as the sheet is considered part of the main UI as per material guidelines, from Scaffold.bottomSheet documentation: Unlike the persistent bottom sheet displayed by showBottomSheet this bottom sheet is not a LocalHistoryEntry and cannot be dismissed with the scaffold appbar's back button. If so why is it even creating a LocalHistoryEntry

@xu-baolin
Copy link
Member Author

Isn't it true that, by definition persistent bottom sheets on Scaffold.bottomSheet should not create a LocalHistoryEntry as the sheet is considered part of the main UI as per material guidelines, from Scaffold.bottomSheet documentation: Unlike the persistent bottom sheet displayed by showBottomSheet this bottom sheet is not a LocalHistoryEntry and cannot be dismissed with the scaffold appbar's back button. If so why is it even creating a LocalHistoryEntry

This is a special case where if the persistent sheet has a draggable widget we should give it a back button for a11y.
Maybe we can update the docs for more information.

@absar
Copy link

absar commented Sep 14, 2022

Ok understood. Can the reviewers prioritize it, the issue is giving a bad UX for the end users

Piinks
Piinks approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 16, 2022
@auto-submit auto-submit bot merged commit eae9bba into flutter:master Sep 16, 2022
This was referenced Sep 17, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 17, 2022
@engine-flutter-autoroll
eae9bba77 fix a draggableScrollableSheet's LocalHistoryEntry leaking (f…
36d9bde 
…lutter/flutter#110576)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 17, 2022
@engine-flutter-autoroll
eae9bba fix a draggableScrollableSheet's LocalHistoryEntry leaking (f…
bdb0575 
…lutter/flutter#110576)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piinks Piinks Piinks approved these changes

@dnfield dnfield Awaiting requested review from dnfield

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
3 participants
@xu-baolin @absar @Piinks