Update the scrollbar #82687

xu-baolin · 2021-05-17T11:06:49Z

I try to introduce a new ScrollContentMetricsNotification event to perceive the metric changes of the scrollable widget.

TODO: need more tests.

@Piinks Hi, What do you think of this?

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

Piinks · 2021-05-17T20:31:00Z

Ooooo This looks very interesting! Let me test it out with a few of the use cases folks have been reporting. I'll fire off the G tests as well to check for potential breaks. Thanks again @xu-baolin! You are a consistently excellent contributor! 🎉

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

Piinks · 2021-06-03T19:23:52Z

Hey @xu-baolin I've been away for a bit, I've just fired off the Google tests to see if this is breaking there so we can see about moving forward. :)

xu-baolin · 2021-06-05T07:47:09Z

Hey, @Piinks Could you please kick off the G-test again? Thanks ~

Piinks · 2021-06-10T19:58:01Z

Sorry @xu-baolin it looks like I created some merge conflicts here. 😳
I am forking this today to test out against a couple of the other use cases we discussed. :)

Piinks

This is really great. I have tested it against a bunch of the various reports we have gotten and it is a very nice solution. Thank you @xu-baolin! Let me know what you think about the options for keeping or eliminating the type cast you mentioned.

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

Piinks · 2021-06-15T22:40:28Z

packages/flutter/test/widgets/scrollbar_test.dart

@@ -1374,6 +1374,37 @@ void main() {
    );
  });

+  testWidgets('The bar can show or hide when the viewport size change', (WidgetTester tester) async {


Can you add another test like this, where rather than changing the content (inside of SingleChildScrollView), it changes the size of the window around it just to verify it works both ways? I think you can do this easiest by just wrapping it with a media query and altering the media query data.

OK, I will add more tests for the new events.

Is this change still break the G-test or internal test?

I'll run the test suite with the latest commit. It only runs automatically after something has been approved.

xu-baolin · 2021-06-16T03:25:17Z

haha, so interesting, this patch can also fix #84201
The new events trigger the remote Material widget to re-layout!

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

Line 379 in 57a565c

onNotification: (LayoutChangedNotification notification) {

xu-baolin · 2021-06-16T13:07:21Z

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

@@ -898,6 +914,15 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
    UserScrollNotification(metrics: copyWith(), context: context.notificationContext!, direction: direction).dispatch(context.notificationContext);
  }

+  /// Dispatches a notification that the contents metrics has changed.
+  ///
+  /// This notification is dispatched when the dimensions of the contents have


I am not sure if I have expressed it clearly,
it is likely that need your help to reorganize the language, thank you!

Piinks · 2021-06-17T21:54:22Z

haha, so interesting, this patch can also fix #84201
The new events trigger the remote Material widget to re-layout!

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

Line 379 in 57a565c

onNotification: (LayoutChangedNotification notification) {

Awesome!! 🥳

Piinks · 2021-06-17T21:55:51Z

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

@@ -942,3 +967,27 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
    description.add('viewport: ${_viewportDimension?.toStringAsFixed(1)}');
  }
 }
+
+/// A notification that a [Scrollable] widget's contents metrics has changed.
+class ScrollContentMetricsNotification extends LayoutChangedNotification with ViewportNotificationMixin {


Suggested change

class ScrollContentMetricsNotification extends LayoutChangedNotification with ViewportNotificationMixin {

class ScrollMetricsNotification extends LayoutChangedNotification with ViewportNotificationMixin {

Let's remove reference to 'content' since it may not be the content that has changed, but something else like the window resizing.

Some sample code would be good too. I think in the dartpad embed, you can change the window size, so folks could see this in action.

Piinks · 2021-06-17T21:56:30Z

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

@@ -508,6 +509,17 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
  ScrollMetrics? _lastMetrics;
  Axis? _lastAxis;

+  bool _isContentMetricsChanged() {


Suggested change

bool _isContentMetricsChanged() {

bool _isMetricsChanged() {

Same here and elsewhere, removing reference to content.

Piinks · 2021-06-17T21:56:50Z

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

@@ -898,6 +914,15 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
    UserScrollNotification(metrics: copyWith(), context: context.notificationContext!, direction: direction).dispatch(context.notificationContext);
  }

+  /// Dispatches a notification that the contents metrics has changed.


Suggested change

/// Dispatches a notification that the contents metrics has changed.

/// Dispatches a notification that the [ScrollMetrics] have changed.

Piinks · 2021-06-17T22:00:24Z

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

+  /// This notification is dispatched when the dimensions of the contents have
+  /// changed. For example, the size of the child changed or new children were
+  /// load when scrolling a long list.


Suggested change

/// This notification is dispatched when the dimensions of the contents have

/// changed. For example, the size of the child changed or new children were

/// load when scrolling a long list.

/// For example, when the content of a scrollable is altered, making it larger or smaller,

/// this notification will be dispatched. Similarly, if the size of the window or parent

/// changes, the scrollable can notify of these changes in dimensions.

How does that sound? We should probably also document how this is different from ScrollNotification.

Piinks · 2021-06-17T22:01:13Z

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

@@ -942,3 +967,27 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
    description.add('viewport: ${_viewportDimension?.toStringAsFixed(1)}');
  }
 }
+
+/// A notification that a [Scrollable] widget's contents metrics has changed.


Similar updates to the docs here. Also, discuss how this is different from the other ScrollNotifications.

Piinks · 2021-06-17T22:03:11Z

packages/flutter/test/widgets/scrollbar_test.dart

@@ -1374,6 +1374,37 @@ void main() {
    );
  });

+  testWidgets('The bar can show or hide when the viewport size change', (WidgetTester tester) async {


I'll run the test suite with the latest commit. It only runs automatically after something has been approved.

xu-baolin · 2021-06-21T13:15:01Z

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

+
+    print('call me.');
+    if (_isMetricsChanged()) {
+      // It isn't safe to trigger the ScrollMetricsNotification if we are in


Currently in the layout stage, and we can not fire an event to the developer.

Piinks · 2021-06-23T18:58:25Z

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

+///
+/// The above behaviors usually do not trigger [ScrollNotification] events,
+/// so this is useful for listening to [ScrollMetrics] changes that are not
+/// caused by the user scrolls.


Suggested change

/// caused by the user scrolls.

/// caused by the user scrolling.

Piinks

This is looking really great. Thank you so much!
Can we split it in to two changes though? I think we should land the new ScrollMetricsNotification first (here) and then implement it in the scrollbar in a follow up change. Especially since this can fix more than one issue, we should keep them separate in case of potential reverts.

Piinks · 2021-06-23T18:59:16Z

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

+///
+/// {@tool dartpad --template=freeform}
+/// This sample shows that the [ScrollMetricsNotification] dispatched when the
+/// window size changed. Press the floating action button to increase the


Just to avoid confusion aroudn the term window

Suggested change

/// window size changed. Press the floating action button to increase the

/// `windowSize` is changed. Press the floating action button to increase the

This is an excellent example by the way.

Piinks · 2021-06-23T18:59:29Z

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

+/// caused by the user scrolls.
+///
+/// {@tool dartpad --template=freeform}
+/// This sample shows that the [ScrollMetricsNotification] dispatched when the


Suggested change

/// This sample shows that the [ScrollMetricsNotification] dispatched when the

/// This sample shows that how a [ScrollMetricsNotification] is dispatched when the

Piinks · 2021-06-23T19:00:06Z

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

+/// ```
+/// {@end-tool}
+class ScrollMetricsNotification extends LayoutChangedNotification with ViewportNotificationMixin {
+  /// Creates a notification that the scrollable widget's [ScrollMetrics] has


Suggested change

/// Creates a notification that the scrollable widget's [ScrollMetrics] has

/// Creates a notification that the scrollable widget's [ScrollMetrics] have

xu-baolin · 2021-06-24T13:12:19Z

@Piinks hey, I have posted a new PR #85221 to land the new event.

Piinks · 2021-06-30T22:12:57Z

(triage update) This is waiting on the reland of #85221, it was reverted.

Piinks · 2021-07-28T21:58:48Z

This can be updated now that the change has landed. 🎉
I am waiting on it to roll to dev to ensure it is not reverted as well.

Piinks

LGTM! This can land now. Thank you very much for sticking with this! You came up with a great solution and then did a lot of work to get it landed. Your contributions are always very much appreciated!

elliette · 2021-08-12T01:15:47Z

Hi! I’m adding a horizontal scrollbar to the source file view in the Flutter devtools’ debugger: flutter/devtools#3262

I’m seeing very different behavior when using the stable Flutter SDK (which doesn’t include this change), and my local fork of Flutter (which includes this change).

Without this change: If the viewport starts out small enough that a horizontal scrollbar is required, and then is resized so that the scrollbar is no longer needed, the scrollbar will still be visible but it won’t do anything.
With this change: If the viewport starts out big enough that a horizontal scrollbar is not required, and then is resized smaller so that a scrollbar is now needed, no scrollbar will show up.

It seems like the scrollbar is being removed in cases where it shouldn't? Thanks!

xu-baolin · 2021-08-12T01:24:40Z

Hi! I’m adding a horizontal scrollbar to the source file view in the Flutter devtools’ debugger: flutter/devtools#3262

I’m seeing very different behavior when using the stable Flutter SDK (which doesn’t include this change), and my local fork of Flutter (which includes this change).

Without this change: If the viewport starts out small enough that a horizontal scrollbar is required, and then is resized so that the scrollbar is no longer needed, the scrollbar will still be visible but it won’t do anything.

With this change: If the viewport starts out big enough that a horizontal scrollbar is not required, and then is resized smaller so that a scrollbar is now needed, no scrollbar will show up.

It seems like the scrollbar is being removed in cases where it shouldn't? Thanks!

@elliette Hey, it would be better if you could provide a minimal sample code. I will investigate as soon as possible.

xu-baolin · 2021-08-12T01:26:04Z

Without this change: If the viewport starts out small enough that a horizontal scrollbar is required, and then is resized so that the scrollbar is no longer needed, the scrollbar will still be visible but it won’t do anything.

This behavior is also not expected, right?

xu-baolin · 2021-08-12T02:08:37Z

@elliette hi, I have left some comments at flutter/devtools#3262, which may help you.

efraimbart · 2021-10-03T08:45:54Z

@xu-baolin Should this notification go out for a list view when items are added below the viewport? I can't seem to get it to work in that case.

xu-baolin · 2021-10-03T09:18:26Z

@xu-baolin Should this notification go out for a list view when items are added below the viewport? I can't seem to get it to work in that case.

It should work, can you provide a sample code to reproduce the case?

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 17, 2021

google-cla bot added the cla: yes label May 17, 2021

xu-baolin mentioned this pull request May 17, 2021

Reland "improve the scrollbar behavior when viewport size changed" #80643

Closed

Piinks reviewed May 17, 2021

View reviewed changes

packages/flutter/lib/src/widgets/scroll_position.dart Outdated Show resolved Hide resolved

This was referenced May 17, 2021

ScrollController.position listeners aren't called when position.pixels i.e., offset is changed by item dismissed #67690

Closed

The Scrollbar() Widget in Flutter Web is not working as expected #75613

Closed

xu-baolin commented May 18, 2021

View reviewed changes

packages/flutter/lib/src/widgets/scrollbar.dart Show resolved Hide resolved

xu-baolin commented May 18, 2021

View reviewed changes

packages/flutter/lib/src/widgets/scrollbar.dart Show resolved Hide resolved

goderbauer requested a review from Piinks May 19, 2021 21:44

xu-baolin force-pushed the 051701bar branch from ea1ca9b to 6e0616a Compare June 5, 2021 06:53

Piinks added the f: scrolling Viewports, list views, slivers, etc. label Jun 10, 2021

Piinks mentioned this pull request Jun 14, 2021

Tree table scroll bar doesn't resize until scroll area touched flutter/devtools#2788

Closed

Piinks reviewed Jun 15, 2021

View reviewed changes

packages/flutter/lib/src/widgets/scrollbar.dart Show resolved Hide resolved

packages/flutter/lib/src/widgets/scrollbar.dart Show resolved Hide resolved

packages/flutter/lib/src/widgets/scrollbar.dart Show resolved Hide resolved

Piinks added a: desktop Running on desktop platform-web Web applications specifically labels Jun 15, 2021

Piinks reviewed Jun 15, 2021

View reviewed changes

xu-baolin commented Jun 16, 2021

View reviewed changes

Piinks added the c: new feature Nothing broken; request for a new capability label Jun 17, 2021

Piinks reviewed Jun 17, 2021

View reviewed changes

xu-baolin commented Jun 21, 2021

View reviewed changes

Piinks reviewed Jun 23, 2021

View reviewed changes

xu-baolin mentioned this pull request Jun 24, 2021

[new feature]introduce ScrollMetricsNotification #85221

Merged

Piinks mentioned this pull request Jul 7, 2021

[Bug web] scrollbar isAlwaysShown not disabling when the scroll is no more needed (on window resize) #85852

Closed

xu-baolin mentioned this pull request Jul 8, 2021

Scrollbar should hide when content is completely displayed #86035

Closed

update the scrollbar

5495299

xu-baolin force-pushed the 051701bar branch from 48065b4 to 5495299 Compare July 29, 2021 03:08

Piinks approved these changes Jul 30, 2021

View reviewed changes

Piinks added the waiting for tree to go green label Jul 30, 2021

fluttergithubbot merged commit 46f160d into flutter:master Jul 30, 2021

flar mentioned this pull request Jul 31, 2021

Lock the first row/col (labels) of the build dashboard flutter/cocoon#1309

Merged

elliette mentioned this pull request Aug 11, 2021

Add a scrollbar for horizontal scrolling of source files in the debugger flutter/devtools#3262

Merged

	class ScrollContentMetricsNotification extends LayoutChangedNotification with ViewportNotificationMixin {
	class ScrollMetricsNotification extends LayoutChangedNotification with ViewportNotificationMixin {

	/// Dispatches a notification that the contents metrics has changed.
	/// Dispatches a notification that the [ScrollMetrics] have changed.

	/// caused by the user scrolls.
	/// caused by the user scrolling.

	/// window size changed. Press the floating action button to increase the
	/// `windowSize` is changed. Press the floating action button to increase the

	/// This sample shows that the [ScrollMetricsNotification] dispatched when the
	/// This sample shows that how a [ScrollMetricsNotification] is dispatched when the

	/// Creates a notification that the scrollable widget's [ScrollMetrics] has
	/// Creates a notification that the scrollable widget's [ScrollMetrics] have

Update the scrollbar #82687

Update the scrollbar #82687

Uh oh!

Conversation

xu-baolin commented May 17, 2021 • edited by Piinks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Piinks commented May 17, 2021

Uh oh!

Uh oh!

Uh oh!

Piinks commented Jun 3, 2021

Uh oh!

xu-baolin commented Jun 5, 2021

Uh oh!

Piinks commented Jun 10, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

xu-baolin commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks commented Jun 17, 2021

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!

Piinks 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!

xu-baolin commented Jun 24, 2021

Uh oh!

Piinks commented Jun 30, 2021

Uh oh!

Piinks commented Jul 28, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

elliette commented Aug 12, 2021

Uh oh!

xu-baolin commented Aug 12, 2021

xu-baolin commented May 17, 2021 •

edited by Piinks

Loading

xu-baolin commented Jun 16, 2021 •

edited

Loading

efraimbart commented Oct 3, 2021 •

edited

Loading