Fix ScrollbarPainter thumbExtent calculation and add padding #31763

LongCatIsLooong · 2019-04-29T00:41:17Z

Description

Fixed extentInside calculation in ScrollMetrics
Added asserts to extentInside getter, as well as ScrollPosition.applyContentDimensions to enforce minScrollExtent <= maxScrollExtent
Added padding to ScrollbarPainter, updated implementation. Took care of some edge cases.
Changed some scroll bar constants on Cupertino side.

Related Issues

Fixes #31430, partially fixes #13253 and #25802

Tests

I added the following tests:

Scrollbar is not smaller than minLength with large scroll views, if minLength is small
When scrolling normally, the size of the scrollbar stays the same, and it scrolls evenly
mainAxisMargin is respected
crossAxisMargin & text direction are respected
Padding works for all scroll directions
Should scroll towards the right direction
Handles MediaQuery well

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

Appendix: Screenshots from iPhone XR running iOS 12.1

Both horizontal and vertical scrollbars are visible and overscrolling
Only the horizontal scrollbars is visible and overscrolling
Only the vertical scrollbars is visible and overscrolling

…ctions

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

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

…flutter into scrollable-obstructions

xster · 2019-04-30T00:56:43Z

Can you rebase to head to solve the merge conflict?

…flutter into scrollable-obstructions

LongCatIsLooong · 2019-05-09T01:49:47Z

@xster @HansMuller removed some randomly made-up terms in documentation. Could you take a look?

xster

LGTM

xster · 2019-05-09T20:53:04Z

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

 const double _kMinThumbExtent = 18.0;

-/// A [CustomPainter] for painting scrollbars.
+/// A [CustomPainter] for painting scrollbars. The size of the scrollbar along


Let the dartdoc's first sentence be a one line summary paragraph.

xster · 2019-05-09T20:56:22Z

packages/flutter/test/cupertino/scrollbar_paint_test.dart

+    await tester.pump(const Duration(milliseconds: 500));
+
+    expect(find.byType(CupertinoScrollbar), paints..rrect(
+        color: _kScrollbarColor,


xster · 2019-05-09T20:57:07Z

packages/flutter/test/material/scrollbar_paint_test.dart

 import '../rendering/mock_canvas.dart';

+Widget _buildSingleChildScrollViewWithScrollbar({
+    TextDirection textDirection = TextDirection.ltr,


HansMuller

This looks good, although I'm not claiming to have combed through all of the new tests. Just some minor feedback.

HansMuller · 2019-05-28T21:20:38Z

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

-  /// This value can be negative infinity, if the scroll is unbounded.
+  /// This value must not be null and must be less than or equal to [maxScrollExtent].
+  /// It can be negative infinity, if the scroll is unbounded.
  double get minScrollExtent;


FixedScrollMetrics should assert that these constraints are true.

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

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

HansMuller · 2019-05-28T21:42:07Z

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

-      );
+    // Thumb extent reflects fraction of content visible, as long as this
+    // isn't less than the absolute minimum size.
+    // contentExtent >= viewportDimension, so (contentExtent - mainAxisPadding) > 0


packages/flutter/test/widgets/slivers_evil_test.dart

HansMuller · 2019-05-28T21:51:11Z

packages/flutter/test/widgets/slivers_evil_test.dart

-            viewportBuilder: (BuildContext context, ViewportOffset offset) {
-              return Viewport(
+    await tester.pumpWidget(
+      MediaQuery(


I'm hoping that the only change to this test is just adding the MediaQuery

I think that's the case. Should I make the MediaQuery dependency optional?

It's fine as is; just difficult to read github diffs sometimes.

HansMuller

LGTM

LongCatIsLooong added 14 commits April 24, 2019 19:02

WIP

c46cec9

page calculation

42bf521

WIP

ef0a54e

WIP

a2cec76

scrollbar x position

4432ff3

fix tests

134df49

WIP

7792e4d

add tests

0386126

Merge remote-tracking branch 'upstream/master' into scrollable-obstru…

71edbc8

…ctions

Merge remote-tracking branch 'upstream/master' into scrollable-obstru…

4e4c640

…ctions

fix tests

c06299f

more tests

4433a94

remove newline

ffddf9c

typo

5833e34

googlebot added the cla: yes label Apr 29, 2019

LongCatIsLooong added d: api docs Issues with https://api.flutter.dev/ f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Apr 29, 2019

LongCatIsLooong commented Apr 29, 2019

View reviewed changes

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

comments

96924ec

LongCatIsLooong force-pushed the scrollable-obstructions branch from 030f24b to 96924ec Compare April 29, 2019 00:52

LongCatIsLooong commented Apr 29, 2019

View reviewed changes

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

LongCatIsLooong commented Apr 29, 2019

View reviewed changes

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

LongCatIsLooong requested a review from xster April 29, 2019 01:36

LongCatIsLooong added 2 commits April 28, 2019 19:16

replace new syntax

74c79a8

Merge branch 'scrollable-obstructions' of github.com:LongCatIsLooong/…

6a62e9c

…flutter into scrollable-obstructions

Piinks added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Apr 29, 2019

Merge branch 'master' into scrollable-obstructions

2532fdb

LongCatIsLooong force-pushed the scrollable-obstructions branch from dda788c to b76d326 Compare May 7, 2019 00:27

review, WIP

4d3e39e

LongCatIsLooong force-pushed the scrollable-obstructions branch from b76d326 to 4d3e39e Compare May 7, 2019 00:30

LongCatIsLooong added 4 commits May 7, 2019 11:09

add new test for material

2357997

Update scrollbar.dart

4271b67

update comments and tests

387c5cd

Merge branch 'scrollable-obstructions' of github.com:LongCatIsLooong/…

5578ad2

…flutter into scrollable-obstructions

LongCatIsLooong force-pushed the scrollable-obstructions branch 3 times, most recently from aa0a16f to 21dda78 Compare May 7, 2019 19:02

indentation

128c12b

LongCatIsLooong force-pushed the scrollable-obstructions branch from 21dda78 to 128c12b Compare May 7, 2019 19:04

change wording

535d25c

xster reviewed May 9, 2019

View reviewed changes

LongCatIsLooong added 2 commits May 9, 2019 14:59

review

be616d3

remove whitespace

d6c9632

HansMuller reviewed May 28, 2019

View reviewed changes

trailing comma

67d9e58

LongCatIsLooong force-pushed the scrollable-obstructions branch from 9184687 to 67d9e58 Compare May 29, 2019 16:54

LongCatIsLooong added 2 commits May 29, 2019 09:55

pixel -> logical pixel

2bea4cf

must -> should

54d37d1

HansMuller approved these changes May 29, 2019

View reviewed changes

Merge branch 'master' into scrollable-obstructions

ff2fff9

LongCatIsLooong merged commit 22ea031 into flutter:master May 29, 2019

LongCatIsLooong deleted the scrollable-obstructions branch May 29, 2019 23:52

justinmc mentioned this pull request Jul 24, 2019

Fix thumb size calculation #36887

Merged

Piinks mentioned this pull request Feb 27, 2020

Scrollbar should have option to specify effective depth for nested scrollable #36474

Closed

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

Fix ScrollbarPainter thumbExtent calculation and add padding #31763

Fix ScrollbarPainter thumbExtent calculation and add padding #31763

Uh oh!

Conversation

LongCatIsLooong commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Appendix: Screenshots from iPhone XR running iOS 12.1

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xster commented Apr 30, 2019

Uh oh!

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

Uh oh!

Uh oh!

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

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LongCatIsLooong commented Apr 29, 2019 •

edited

Loading

LongCatIsLooong commented May 9, 2019 •

edited

Loading