Bottom sheet scrolling by dnfield · Pull Request #21896 · flutter/flutter

dnfield · 2018-09-14T23:44:35Z

This is still a bit rough, but wanted to start getting some eyes on it.

Fixes #497

Here's what I know it still needs

Tests! Existing tests need to be updated
Write new tests for some of the new functionality
~~Persistent sheets (e.g. setting the Scaffold.bottomSheet) need some work. Right now they're getting dismissed if another sheet gets dismissed.~~ This is resolved by tightening up an existing restriction to ensure that people can't show Standard Bottom Sheets if a persistent Bottom Sheet is already visible (and vice versa). Existing code had it covered the other way around, this just extends that. I suspect someone out there might want to do this, but I'm not sure it makes much sense design wise and it makes the code a lot more complicated.
Settle on how to handle when a child shouldn't scroll to the top of the screen. Right now, a developer has to pass a few parameters (set initialTop to something valid, and set clampTop to true). This is the best I've been able to come up with so far - any attempts to automate that for developers result in some kind of bad result somewhere else. I think the refactor to an initialHeightPercentage solves a lot of this.
Determine whether everything I'm breaking absolutely needs to be broken. I'm changing some parameters and behavior here, because the existing widget is pretty badly broken in a few ways.
Make sure this all works with a11y

Ways existing widget is broken:

Can't go full screen
Can't be dragged if it has a list/scrolling content/gesturedetector in its child
Doesn't hide the FAB if it's given a tall height
Doesn't differentiate very well between "Standard Bottom Sheets" and "Persistent Bottom Sheets"

This is still WIP, but I think enough is done that it's worth getting some review/feedback.

rami-a · 2019-04-22T20:43:46Z

Hey @dnfield I'm currently working on some overlapping BottomSheet changes in #31318

I just wanted you to be aware of that. Looks like we both had a similar idea (and implementation) regarding exposing color.

HansMuller

I apologize if a few of my comments are out of date; I started this review a week ago and just resumed now.

I've looked through everything except the tests

HansMuller · 2019-04-10T21:54:12Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+  ///
+  /// In most cases, this should be true. However, in the case of a parent
+  /// widget that will position this one based on its desired size (such as a
+  /// [MultiChildLayoutDelegate]), this should be set to false.


This seems like a somewhat obscure class to reference here. Maybe Center instead?

HansMuller · 2019-04-10T21:55:16Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// related changes. When the extent of the sheet changes via a drag,
+/// this notification bubbles up through the tree, which means a given
+/// [NotificationListener] will recieve notifications for all descendant
+/// [DraggableScrollableSheet] widgets. To focus on notifications form the


form => from

HansMuller · 2019-04-10T21:55:38Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

  _DraggableScrollableSheetState createState() => _DraggableScrollableSheetState();
 }

+/// A [Notification] related to the position of the [DraggableScrollableSheet].


maybe "size and scroll offset" instead of "position"? Ideally this one-line summary would make the meaning of "extent" clear.

HansMuller · 2019-04-10T21:56:21Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// A [Notification] related to the position of the [DraggableScrollableSheet].
+///
+/// [DraggableScrollableSheet] widgets notify their ancestors about layout
+/// related changes. When the extent of the sheet changes via a drag,


Can we say something more specific than "layout related"?

HansMuller · 2019-04-10T21:57:40Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// nearest [DraggableScorllableSheet] descendant, check that the [depth]
+/// property of the notification is zero.
+///
+/// When a extent notification is received by a [NotificationListener], the


HansMuller · 2019-04-23T23:40:30Z

packages/flutter/lib/src/material/scaffold.dart

+  Future<void> close() {
+    assert(widget.animationController != null);
    widget.animationController.reverse();
+    widget.onClosing?.call();


We usually write:

if (widget.onClosing != null) widget.onClosing();

HansMuller · 2019-04-23T23:42:15Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// By default, the widget will expand its non-occupied area to fill availble
+/// space in the parent. If this is not desired, e.g. because the parent wants
+/// to position sheet based on the space it is taking, the [expand] property
+/// may be set ot false.


HansMuller · 2019-04-23T23:47:36Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// A widget that notifies a descendent [DraggableScrollableSheet] that it
+/// should reset its position to the initial state.
+///
+/// The [Scaffold] uses this widget to notify a persistententbottom sheet that


"persistent bottom sheet" or refer to a class/method

HansMuller · 2019-04-23T23:49:20Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// the user has tapped back if the sheet has started to cover more of the body
+/// than when at its initial position. This is important for users of assistive
+/// technology, where dragging may be difficult to communicate.
+class DraggableScrollableSheetResetter extends StatelessWidget {


We should chat about this name :-) On the up side it literally communicates its singular function. On the other hand ....

HansMuller · 2019-04-23T23:51:25Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+    @required this.child
+  }) : super(key: key);
+
+  /// The child to build that may contain a [DraggableScrollableSheet].


This child's [DraggableScrollSheet] descendant will be reset when the static [reset] method is applied to a to context that includes it.

packages/flutter/lib/src/material/bottom_sheet.dart

packages/flutter/lib/src/material/scaffold.dart

packages/flutter/lib/src/material/bottom_sheet.dart

HansMuller

LGTM with a few small potatoes comments

Hooray!

HansMuller · 2019-04-26T19:39:15Z

packages/flutter/lib/src/material/bottom_sheet.dart

    this.animationController,
    this.enableDrag = true,
    this.elevation = 0.0,
+    this.color,


I hate to bring this up but, since showBottomSheet now has a backgroundColor parameter, maybe this should be called backgroundColor as well.

HansMuller · 2019-04-26T19:41:27Z

packages/flutter/lib/src/material/bottom_sheet.dart

+
+  @override
+  Widget build(BuildContext context) {
+    final MediaQueryData mediaQuery = MediaQuery.of(context);


Should assert here:

assert(debugCheckHasMediaQuery(context)); assert(debugCheckHasMaterialLocalizations(context));

HansMuller · 2019-04-26T19:52:25Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// [DraggableScrollableSheet].
+///
+/// [DraggableScrollableSheet] widgets notify their ancestors when the size of
+/// the sheet changes. When the extent of the sheet changes via a drag,


this is a little tricky because we're talking about "size" and then "extent" in the next sentence. I think we need to defined what "extent" means at the outset of all of this.

HansMuller · 2019-04-26T19:52:49Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// nearest [DraggableScorllableSheet] descendant, check that the [depth]
+/// property of the notification is zero.
+///
+/// When a extent notification is received by an [NotificationListener], the


a extent => an extent
an [NotificationListener] => a [NotificationListener]

HansMuller · 2019-04-26T19:54:54Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+/// build or layout based on an extent notification would result in a layout
+/// that lagged one frame behind, which is a poor user experience. Extent
+/// notifications are used primarily to drive animations. The [Scaffold] widget
+/// is an example of driving animations for the [FloatingActionButton] as the


maybe you mean that the Scaffold listens for extent notifications and responds by driving animations ...

HansMuller · 2019-04-26T19:56:52Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+  /// changed.
+  ///
+  /// All parameters are required. The [minExtent] must be >= 0.  The [maxExtent]
+  /// must be <= 1.0.  The [extent] must be between [minExtent] and [maxExtent].


Doesn't initialExtent need to be between min/maxExtent

HansMuller · 2019-04-26T19:59:49Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

  }
 }
+
+/// A widget that notifies a descendent [DraggableScrollableSheet] that it


that notifies => that can notify

HansMuller · 2019-04-26T20:02:16Z

packages/flutter/test/material/modal_bottom_sheet_test.dart

    expect(find.text('BottomSheet'), findsOneWidget);

-    await tester.pump(); // bottom sheet dismiss animation starts
+     await tester.pump(); // bottom sheet dismiss animation starts


extra space here

ThinkDigitalSoftware · 2019-04-26T21:45:53Z

Thanks, @dnfield for sticking with this! Are any of the issues mentioned in the original post or other comments still existing?

dnfield · 2019-04-26T22:22:29Z

If there are, please file a bug. This should be good though.

ThinkDigitalSoftware · 2019-05-24T22:45:32Z

@dnfield Shouldn't this be DraggableScrollableBottomSheet? and shouldn't the name of the parameter be initialChildSizeRatio, not size? since 1 isn't a size, but a ratio. Same with the other parameters. Also, could we match the params of the other bottom sheets? Like the shape. Does this need to be a new issue?

dnfield · 2019-05-24T23:02:11Z

This initially was implemented to support horizontal scrolling as well, but was removed due to complexity and lack of solid use cases. It would be possible to reimplement

dnfield · 2019-05-24T23:03:32Z

And in general it's probably a bad idea to specify absolute heights for this wide, since it will show up on different screens and form factors.

E.g. what if the phone is in landscape mode, or what if you're on a desktop or larger tablet

ThinkDigitalSoftware · 2019-05-24T23:07:59Z

Wouldn't that be our business to control? For example, I want a widget that works exactly like a bottomsheet that will cover most of my screen except the appbar.

dnfield · 2019-05-25T00:21:53Z

You can just specify it as a percentage of the available space

ThinkDigitalSoftware · 2019-05-25T00:55:49Z

I think it's because I thought this was a bottomsheet

dnfield and others added 30 commits September 6, 2018 16:23

start bottom sheet refactoring

8d7eddc

Teach ScaffoldState to hide and show FAB

7483039

move stack to proper location

e5b8a78

Make sure to dispose animationController, add test

db4d804

fix indentation, remove unused call

75c4b1f

start work on adding to theme

d194c25

Start moving controllers up to scaffold

b285fba

revert unneeded changes to theme

96d4022

cleanup

970c071

test for interaction between show/hide and move

8723748

add onPressed to tests

8f2a1c5

merge hide_fab

3e29681

Expose value instead of controller

df9d20a

Merge branch 'hide_fab' into bottom_sheet

071a8d9

Merge branch 'master' into hide_fab

73cf248

scrollTopThenContent widget

e047a5d

merge

675203d

remove new

64fd139

typo

22adf2e

typo

9958c15

remove new

5f4ee2f

merge with hide_fab changes

7acedd4

Use positioned

b68cec0

Scrolling mechanics and FAB hiding

c2a3c20

Dismissed sheets, scrim

4c64e74

Scrim touchup, formatting

158d43f

merge

ca3eb6e

Merge remote-tracking branch 'upstream/master' into bottom_sheet

051348a

rename, minor improvements

874e87b

cleanup, propagate initialtop

009cc58

dnfield added 3 commits April 10, 2019 23:26

Merge remote-tracking branch 'upstream/master' into bottom_sheet

9a9affe

refactor notifier, improve test

7a2810d

const

2aa19fa

HansMuller reviewed Apr 24, 2019

View reviewed changes

dnfield and others added 4 commits April 23, 2019 18:21

Feedback

bfe7462

save before committing

ad02a15

Merge branch 'master' into bottom_sheet

f7adb72

Update scaffold.dart

60c2e93

rami-a reviewed Apr 25, 2019

View reviewed changes

packages/flutter/lib/src/material/bottom_sheet.dart Outdated Show resolved Hide resolved

rami-a reviewed Apr 25, 2019

View reviewed changes

packages/flutter/lib/src/material/scaffold.dart Outdated Show resolved Hide resolved

bottomSheetColor -> backgroundColor

10202a8

rami-a reviewed Apr 25, 2019

View reviewed changes

packages/flutter/lib/src/material/bottom_sheet.dart Outdated Show resolved Hide resolved

Typo

647b6f5

HansMuller approved these changes Apr 26, 2019

View reviewed changes

dnfield added 2 commits April 26, 2019 13:19

DONE

b5be75c

last param

46095c0

dnfield merged commit 6a48e66 into flutter:master Apr 26, 2019

dnfield deleted the bottom_sheet branch April 26, 2019 21:12

HansMuller mentioned this pull request Apr 29, 2019

Add backgroundColor option to bottom sheet #31248

Closed

9 tasks

HansMuller mentioned this pull request May 15, 2019

Bottom sheet with configurable height #32747

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

Conversation

dnfield commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rami-a commented Apr 22, 2019

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThinkDigitalSoftware commented Apr 26, 2019

Uh oh!

dnfield commented Apr 26, 2019

Uh oh!

ThinkDigitalSoftware commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented May 24, 2019

Uh oh!

dnfield commented May 24, 2019

Uh oh!

ThinkDigitalSoftware commented May 24, 2019

Uh oh!

dnfield commented May 25, 2019

Uh oh!

ThinkDigitalSoftware commented May 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

dnfield commented Sep 14, 2018 •

edited

Loading

ThinkDigitalSoftware commented May 24, 2019 •

edited

Loading