Refactor Scrollbar/CupertinoScrollbar to RawScrollbar [Scrollbars Part 1] #71242

Piinks · 2020-11-25T23:09:35Z

Description

This is the first of several changes planned for #70866
Originally plotted out in #71181, that change is being broken up into multiple smaller changes. (Hopefully more palatable that way 🙂 )

This change will refactor the Scrollbar and CupertinoScrollbar widgets to use RawScrollbar as a base class.
I wanted to complete this refactoring first since these widgets have so many common lines of code.

The RawScrollbar widget will provide the basic fade in-out scrollbar thumb the framework currently provides, with the current options to customize (to be expanded in follow up changes). Right now this change is sticking to existing functionality to validate the refactor and keep things simple.

This refactoring will enable dragging on the thumb for RawScrollbar, Scrollbar, and CupertinoScrollbar since everyone is sharing. (Previously only CupertinoScrollbar supported dragging the thumb).

This will also remove the adaptive nature of the Scrollbar widget. It looks very simple now, but there are a lot of MD updates coming to the Scrollbar widget in follow-up changes, including new animations. (See #71181)

I want to separate the CupertinoScrollbar from Scrollbar since as we expand on these widgets and look forward to additional platforms, having special handling in Scrollbar for CupertinoScrollbar was going to get even more complicated. Adaptive widgets like these do not scale well, and are easier to maintain and debug when separated. The new Material Design Scrollbar is very similar to the CupertinoScrollbar, so I am hoping the change will have a minimal effect.

Planned Changes for Scrollbars

(Order may change as these are reviewed and land)

1. Refactor to RawScrollbar
2. Update Scrollbar to match Material Design spec
3. Add track gestures
4. Add code samples, etc.
5. Add ScrollbarTheme
6. Create Scrollbars by default for relevant scrollables on desktop and web

Related Issues

Part of #70866
Fixes #70105

Tests

Existing tests pass after refactoring. :)
Added tests for the now shared thumb gestures between cupertino and material.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

Piinks · 2020-11-25T23:13:42Z

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

+///  * [CupertinoScrollbar], an iOS style scrollbar.
+///  * [ListView], which display a linear, scrollable list of children.
+///  * [GridView], which display a 2 dimensional, scrollable array of children.
+class RawScrollbar extends StatefulWidget {


Originally I had this as an abstract class, but after our last brainstorm session, we discussed how Scrollbars will be used by default for scrollables on desktop and web.
We can use this scrollbar for the widgets layer scroll configuration, while the other two scroll configurations (material and cupertino) have scrollbars to match.

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

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

gspencergoog · 2020-12-01T21:18:31Z

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

+  /// {@end-tool}
+  final bool isAlwaysShown;
+
+  /// The radius of the corners of the scrollbar.


You should add doc comments about what happens if these are set to null (this property, and the other nullable properties).

Also, why not make these non-nullable? Yes, you'd have to make them required in the constructor, but that seems reasonable for a raw scrollbar.

I think I left them nullable since the current material/Scrollbar allows for these to be null. They are non-nullable for CupertinoScrollbar.
If thickness is null, material/Scrollbar just uses the default thickness, so I think we can handle that there so it can be non-nullable here.
Radius can be null, since ScrollbarPainter null checks to determine if it needs to paint a rect or a rrect.

I will add more docs too for all of this. :)

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

…ollbar-part-1

…rt-1

HansMuller

Mostly API doc comments

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

HansMuller · 2020-12-02T20:17:21Z

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

-
-  /// The radius of the corners of the scrollbar.
+  /// If the [controller] is null, the default behavior is to automatically
+  /// enable scrollbar dragging on the nearest [ScrollController] using


We're not really enabling dragging "on a ScrollController"?

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

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

HansMuller · 2020-12-02T21:23:44Z

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

+  }
+
+  /// Returns the [Axis] of the child scroll view, or null if one is not
+  /// determined.


What situation would lead to a null return value? In other words do callers have to unconditionally guard against a null return value or are there circumstances where it's safe to assume that the return value is non-null?

HansMuller · 2020-12-02T21:25:47Z

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

+    return false;
+  }
+
+  /// Get the GestureRecognizerFactories used to detect gestures on the scrollbar


This needs to say more about what gestures we're handling and what methods (that subclasses can override) are called when those gestures occur.

If we decide to remove all of the protected methods, then subclasses won't be able to override these. Actually, in the changes we discussed today, I think all subclasses will have to implement their own gestures.

HansMuller · 2020-12-02T21:28:05Z

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

      return;
    }
-    _fadeoutTimer?.cancel();
+    super.handleLongPress();


If subclasses are allowed to opt out of the super call, the base class will have trouble reasoning about how the handleFoo methods update its state.

I should include a @mustCallSuper

Piinks · 2020-12-03T00:55:41Z

I am going to close this for now while we revisit the larger picture. I split this up into multiple smaller changes to make reviewing easier, but I think it has lost some of the context, making reviewing actually harder.. I'll re-open another PR that covers the whole thing with the suggestions made here applied so we can discuss further with everything together.

Piinks added 5 commits November 24, 2020 17:46

Rework scrollbars to base RawScrollbar

3d948de

Test work

a4829a6

Remove all but RawScrollbar refactoring

f2da010

Test updates

52b0ba2

Remove duped tests

2242ce0

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Nov 25, 2020

google-cla bot added the cla: yes label Nov 25, 2020

Piinks commented Nov 25, 2020

View reviewed changes

Fix tests

025050e

Piinks requested review from HansMuller and gspencergoog November 25, 2020 23:45

Piinks commented Dec 1, 2020

View reviewed changes

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

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

ec21f6b

gspencergoog reviewed Dec 1, 2020

View reviewed changes

Piinks added 4 commits December 2, 2020 12:35

Review feedback

ebb5094

Merge branch 'scrollbar-part-1' of github.com:Piinks/flutter into scr…

1ae57ad

…ollbar-part-1

++

b4a85fe

Merge branch 'master' of github.com:flutter/flutter into scrollbar-pa…

a7ab8aa

…rt-1

HansMuller reviewed Dec 2, 2020

View reviewed changes

Piinks closed this Dec 3, 2020

Piinks mentioned this pull request Dec 3, 2020

Updated Interactive Scrollbars #71664

Merged

9 tasks

Refactor Scrollbar/CupertinoScrollbar to RawScrollbar [Scrollbars Part 1] #71242

Refactor Scrollbar/CupertinoScrollbar to RawScrollbar [Scrollbars Part 1] #71242

Uh oh!

Conversation

Piinks commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Planned Changes for Scrollbars

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Piinks commented Dec 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Piinks commented Nov 25, 2020 •

edited

Loading