New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ScrollbarPainter thumbExtent calculation and add padding #31763
Fix ScrollbarPainter thumbExtent calculation and add padding #31763
Conversation
030f24b
to
96924ec
Compare
…flutter into scrollable-obstructions
Can you rebase to head to solve the merge conflict? |
b76d326
to
4d3e39e
Compare
…flutter into scrollable-obstructions
aa0a16f
to
21dda78
Compare
21dda78
to
128c12b
Compare
@xster @HansMuller removed some randomly made-up terms in documentation. Could you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -12,7 +12,10 @@ import 'scroll_metrics.dart'; | |||
|
|||
const double _kMinThumbExtent = 18.0; | |||
|
|||
/// A [CustomPainter] for painting scrollbars. | |||
/// A [CustomPainter] for painting scrollbars. The size of the scrollbar along |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let the dartdoc's first sentence be a one line summary paragraph.
await tester.pump(const Duration(milliseconds: 500)); | ||
|
||
expect(find.byType(CupertinoScrollbar), paints..rrect( | ||
color: _kScrollbarColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 indents
@@ -7,32 +7,74 @@ import 'package:flutter/material.dart'; | |||
|
|||
import '../rendering/mock_canvas.dart'; | |||
|
|||
Widget _buildSingleChildScrollViewWithScrollbar({ | |||
TextDirection textDirection = TextDirection.ltr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 indents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, although I'm not claiming to have combed through all of the new tests. Just some minor feedback.
@@ -60,14 +60,16 @@ abstract class ScrollMetrics { | |||
/// | |||
/// The actual [pixels] value might be [outOfRange]. | |||
/// | |||
/// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FixedScrollMetrics should assert that these constraints are true.
); | ||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICE
viewportBuilder: (BuildContext context, ViewportOffset offset) { | ||
return Viewport( | ||
await tester.pumpWidget( | ||
MediaQuery( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping that the only change to this test is just adding the MediaQuery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the case. Should I make the MediaQuery
dependency optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine as is; just difficult to read github diffs sometimes.
9184687
to
67d9e58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…#31763) - 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.
Description
extentInside
calculation inScrollMetrics
extentInside
getter, as well asScrollPosition.applyContentDimensions
to enforceminScrollExtent <= maxScrollExtent
ScrollbarPainter
, updated implementation. Took care of some edge cases.Related Issues
Fixes #31430, partially fixes #13253 and #25802
Tests
I added the following tests:
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.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your 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