update the scrollbar that support always show the track even not on hover #90178

xu-baolin · 2021-09-16T08:47:18Z

I am currently working on that let the scrollbar support leading and trailing arrow buttons(reference: #80370) like chrome on the web:

First, we should support that always show the track like the web.

My arrow buttons solution (Next work):

1, Add a property like showControlButtons to control whether show the buttons.
2, Add a class like ArrowScrollbarButtonShape(works like RectangularSliderTrackShape) to paint the buttons. it can be custom by the developers.

Look forward to your thoughts @Piinks

flutter-dashboard · 2021-09-16T08:47:21Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

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

xu-baolin · 2021-09-18T06:15:16Z

Ready for review now

Piinks

This is awesome! Thank you @xu-baolin! I am looking forward to all the changes you have laid out here and for the future. :)

I think we should avoid enumerating configs like showTrack and showTrackOnHover.

This sounds like a perfect use case for using a MaterialStateProperty like ScrollbarThemeData.thumbColor. We could simplify it to something like aMaterialStateProperty<bool?>? for ScrollbarThemeData.trackVisibility, that way you can evaluate showing the track for any state, or just return true for always visible etc..

In this case we would probably deprecate showTrackOnHover and point folks to using the new property.

What do you think @xu-baolin? @HansMuller may also be interested. :)

Piinks

Let me know your thoughts on this. You have inspired me to see if I can refactor thickness/hoverThickness into a single MaterialStateProperty. :)

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

xu-baolin · 2021-10-09T10:51:24Z

It’s weird to have a hoverThickness API, it works only hover state and showTrackOnHover, is there a special reason?

Maybe we can deprecate this after we have trackVisibility.

Piinks · 2021-10-11T17:25:11Z

It’s weird to have a hoverThickness API, it works only hover state and showTrackOnHover, is there a special reason?

Maybe we can deprecate this after we have trackVisibility.

Yeah, I think when we did that large refactor last year adding RawScrollbar as the base class + new material updates, we wanted to try to make all of those changes non breaking. Since thickness already existed as a double, we just added hoverThickness.

Piinks

Thanks for the updates! This is a bit tricky to refactor, so thanks for sticking with it! A cleaner API will be so much better for everyone. :)
I am happy to write dart fixes as a follow up as well.

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

Piinks · 2021-10-11T17:36:35Z

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

@@ -51,8 +56,16 @@ class ScrollbarThemeData with Diagnosticable {
  ///  * [MaterialState.hovered] on web and desktop platforms.
  final MaterialStateProperty<double?>? thickness;

+  /// Controls if the track will show in all descendant [Scrollbar] widgets.
+  ///
+  final MaterialStateProperty<bool?>? trackVisibility;


This should probably list the default behavior, this thickness just above here.

This value defaults to null, did you mean to add some default behaviors for some platforms?

By default the scrollbar will show this on hover right?

Long time no see :)
I think the windows platform should show track by default.

xu-baolin · 2021-10-12T02:55:17Z

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

@@ -51,8 +56,16 @@ class ScrollbarThemeData with Diagnosticable {
  ///  * [MaterialState.hovered] on web and desktop platforms.


I can not found this default behavior on web or desktop platforms, am I miss something?

Oh I think this hover behavior came from a design specification I was given by the material design team. If you hover over the material scrollbar on desktop the thickness changes.

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

Piinks

This is looking good. It may be helpful to include this in the design doc I mentioned in the arrow button change. That way we can reason about a follow up change to deprecate some of the other properties mentioned here.

Thanks as always for your patience. I was away. This is really good work, and a lot of it too! I know it takes a while to get through it all, that's why I think a design doc may help get us on the same page. :)

Piinks · 2021-11-11T22:24:31Z

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

@@ -51,8 +56,16 @@ class ScrollbarThemeData with Diagnosticable {
  ///  * [MaterialState.hovered] on web and desktop platforms.


Oh I think this hover behavior came from a design specification I was given by the material design team. If you hover over the material scrollbar on desktop the thickness changes.

Piinks · 2021-11-11T22:26:47Z

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

@@ -51,8 +56,16 @@ class ScrollbarThemeData with Diagnosticable {
  ///  * [MaterialState.hovered] on web and desktop platforms.
  final MaterialStateProperty<double?>? thickness;

+  /// Controls if the track will show in all descendant [Scrollbar] widgets.
+  ///
+  final MaterialStateProperty<bool?>? trackVisibility;


By default the scrollbar will show this on hover right?

Piinks · 2021-11-24T21:44:49Z

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

+  /// If this property is null, then [ScrollbarThemeData.trackVisibility] of
+  /// [ThemeData.scrollbarTheme] is used. If that is also null, the track will
+  /// not show by default.
+  final MaterialStateProperty<bool?>? trackVisibility;


Can you document how this relates (or doesn't) to showTrackOnHover? I know that we are going to follow up with deprecating the later, but in the meantime it may be difficult to understand which one I should use and how they affect each other, or if one overrides the other.

Piinks · 2021-11-24T21:55:12Z

Another thought, the CupertinoScrollbar may someday want to incorporate a track as well (it does not match native Mac desktop yet either), would making the Scrollbar.trackVisibility a bool? that is also in the super class as RawScrollbar.trackVisibility, make that easier in the future?
trackVisibility could still be a MaterialStateProperty<bool?> in the ScrollbarTheme. Just like:

ScrollbarTheme.thickness is MaterialSateProperty<double?>
Scrollbar.thickness, CupertinoScrollbar.thickness and RawScrollbar.thickness is double

I don't know if one or the other is better necessarily, but if one sets us up better for future changes it will likely be worth it. I am curious to know what you think. Thanks @xu-baolin!

goderbauer · 2021-11-29T17:36:52Z

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

@@ -95,6 +96,13 @@ class Scrollbar extends StatelessWidget {
  /// {@macro flutter.widgets.Scrollbar.isAlwaysShown}
  final bool? isAlwaysShown;

+  /// Controls if the track will show in all descendant [Scrollbar] widgets.


Why descendant Scrollbar widgets? Does this affect more than just the scrollbar widget it is set on?

Oh, this is a typo.

goderbauer · 2021-11-29T17:38:37Z

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

@@ -51,6 +52,11 @@ class ScrollbarThemeData with Diagnosticable {
  ///  * [MaterialState.hovered] on web and desktop platforms.
  final MaterialStateProperty<double?>? thickness;

+  /// Overrides the default value of [Scrollbar.trackVisibility] in all
+  /// descendant [Scrollbar] widgets.
+  ///


nit: remove blank line?

skia-gold · 2021-12-02T11:47:47Z

Gold has detected about 4 new digest(s) on patchset 9.
View them at https://flutter-gold.skia.org/cl/github/90178

xu-baolin · 2021-12-02T11:57:22Z

Another thought, the CupertinoScrollbar may someday want to incorporate a track as well (it does not match native Mac desktop yet either), would making the Scrollbar.trackVisibility a bool? that is also in the super class as RawScrollbar.trackVisibility, make that easier in the future? trackVisibility could still be a MaterialStateProperty<bool?> in the ScrollbarTheme. Just like:

ScrollbarTheme.thickness is MaterialSateProperty<double?>

Scrollbar.thickness, CupertinoScrollbar.thickness and RawScrollbar.thickness is double

I don't know if one or the other is better necessarily, but if one sets us up better for future changes it will likely be worth it. I am curious to know what you think. Thanks @xu-baolin!

I think what you said makes sense. For future updates, it is not suitable to introduce the Material state into Scrollbar widget. At the same time, I added some documents.

Piinks

LGTM thank you!
Would you like me to do the follow-up deprecations? Or were you planning on doing that?
I am happy to do it and write the dart fix migrations if you'd prefer. :)

xu-baolin · 2021-12-04T03:50:48Z

I would really appreciate it if you can do this for me,
I am willing to review it.

Piinks · 2021-12-06T19:32:45Z

I would really appreciate it if you can do this for me, I am willing to review it.

Happy to! I will cc you for review when it is ready. Thanks! :)

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Sep 16, 2021

google-cla bot added the cla: yes label Sep 16, 2021

xu-baolin requested a review from Piinks September 16, 2021 08:54

xu-baolin commented Sep 16, 2021

View reviewed changes

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

xu-baolin force-pushed the 0915showtrack branch from 1a79f49 to 2390ff0 Compare September 18, 2021 06:03

xu-baolin changed the title ~~update scrollbar widget[1/2]~~ update the scrollbar that support always show the track even not on hover Sep 18, 2021

Piinks reviewed Sep 22, 2021

View reviewed changes

Piinks reviewed Oct 1, 2021

View reviewed changes

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

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

Piinks reviewed Oct 11, 2021

View reviewed changes

xu-baolin commented Oct 12, 2021

View reviewed changes

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

xu-baolin requested a review from Piinks October 12, 2021 07:01

xu-baolin mentioned this pull request Oct 15, 2021

[Ready for review] update scrollbar to support the leading and trailing buttons #91877

Closed

Piinks reviewed Nov 11, 2021

View reviewed changes

Piinks reviewed Nov 24, 2021

View reviewed changes

goderbauer reviewed Nov 29, 2021

View reviewed changes

feedback

c29158b

xu-baolin force-pushed the 0915showtrack branch from 8fffc99 to c29158b Compare December 2, 2021 11:44

++

b8172a2

xu-baolin requested review from goderbauer and Piinks December 2, 2021 12:22

Piinks approved these changes Dec 3, 2021

View reviewed changes

xu-baolin added the waiting for tree to go green label Dec 4, 2021

fluttergithubbot merged commit 313b5f3 into flutter:master Dec 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update the scrollbar that support always show the track even not on hover #90178

update the scrollbar that support always show the track even not on hover #90178

xu-baolin commented Sep 16, 2021 •

edited by Piinks

flutter-dashboard bot commented Sep 16, 2021

xu-baolin commented Sep 18, 2021

Piinks left a comment

Piinks left a comment

xu-baolin commented Oct 9, 2021

Piinks commented Oct 11, 2021

Piinks left a comment

Piinks Oct 11, 2021

xu-baolin Oct 12, 2021

Piinks Nov 11, 2021

xu-baolin Nov 12, 2021

xu-baolin Oct 12, 2021

Piinks Nov 11, 2021

Piinks left a comment

Piinks Nov 11, 2021

Piinks Nov 11, 2021

Piinks Nov 24, 2021

Piinks commented Nov 24, 2021

goderbauer Nov 29, 2021

xu-baolin Dec 2, 2021

goderbauer Nov 29, 2021

xu-baolin Dec 2, 2021

skia-gold commented Dec 2, 2021

xu-baolin commented Dec 2, 2021

Piinks left a comment

xu-baolin commented Dec 4, 2021

Piinks commented Dec 6, 2021

		@@ -51,8 +56,16 @@ class ScrollbarThemeData with Diagnosticable {
		/// * [MaterialState.hovered] on web and desktop platforms.

update the scrollbar that support always show the track even not on hover #90178

update the scrollbar that support always show the track even not on hover #90178

Conversation

xu-baolin commented Sep 16, 2021 • edited by Piinks

My arrow buttons solution (Next work):

flutter-dashboard bot commented Sep 16, 2021

xu-baolin commented Sep 18, 2021

Piinks left a comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

xu-baolin commented Oct 9, 2021

Piinks commented Oct 11, 2021

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skia-gold commented Dec 2, 2021

xu-baolin commented Dec 2, 2021

Piinks left a comment

Choose a reason for hiding this comment

xu-baolin commented Dec 4, 2021

Piinks commented Dec 6, 2021

xu-baolin commented Sep 16, 2021 •

edited by Piinks