Fixed Scroll bars incorrectly rendering with view padding #150685

D-extremity · 2024-06-24T07:53:40Z

Replacing ..padding = MediaQuery.paddingOf(context) with ..padding = widget.padding??EdgeInsets.zero to fix incorrect rendering of scrollbar.

closes #150544

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on [Discord].

Piinks

Hey @D-extremity thanks for contributing! There are some failing tests here, can you take a look?

D-extremity · 2024-06-27T14:47:29Z

Hey @D-extremity thanks for contributing! There are some failing tests here, can you take a look?

yeah! actually When padding is removed from RawScrollbar, Material Scrollbar and Cupertino, i wrote test cases they are passed, but previous tests started failing because when they were written, they were written with the idea of padding in the scrollbar, i fixed some.

I am trying to solve this test case error, it would be helpful if you have any tip or idea to solve it

flutter/packages/flutter/test/cupertino/scrollbar_paint_test.dart

Line 16 in 9a67317

testWidgets('Paints iOS spec', (WidgetTester tester) async {

Piinks · 2024-07-02T17:47:48Z

That one looks like a precision error. The values of the rect are not precisely matching the expectation, so I would update the test to check the individual values of the rect using the moreOrLessEquals.

D-extremity · 2024-07-22T17:23:19Z

@Piinks please review it in your available time

packages/flutter/test/cupertino/scrollbar_paint_test.dart

packages/flutter/test/material/scrollbar_test.dart

packages/flutter/test/widgets/scrollbar_test.dart

packages/flutter/test/material/scrollbar_paint_test.dart

packages/flutter/test/material/scrollbar_test.dart

dkwingsmt · 2024-07-22T23:35:14Z

Mostly ok except for the removed expect

D-extremity · 2024-07-22T23:37:58Z

Mostly ok except for the removed expect

Thank you sir,
I tried implementing that test from many ways but it didn't work
At last I was stuck at
Which: threw the following exception: It called drawRRect with RRect, RRect.fromLTRBR(794.0, 3.0, 797.0, 90.0, 1.5), which was not exactly the expected RRect (RRect.fromLTRBR(794.0, 3.0, 797.0, 90.0, 1.5)).

Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>

fixed formatting Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>

Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>

dkwingsmt · 2024-07-23T21:23:58Z

I did some more investigation on the failed test (https://github.com/flutter/flutter/pull/150685/files#diff-c470a64409840da3df5f8b241a8b984964dd26290c65839c8ebeddcc16691155L118) and tried to get the exact number and how it is made up.

The correct height is 68.06544372830938, which is fractionVisible * _traversableTrackExtent, where

fractionVisible = 0.12989588497768964
_traversableTrackExtent = 524.0

The fractionVisible is a result of (_lastMetrics!.extentInside - _totalTrackMainAxisOffsets) / (_totalContentExtent - _totalTrackMainAxisOffsets), where

_lastMetrics!.extentInside = 524
_totalContentExtent = 4034
_totalTrackMainAxisOffsets = 0

Compare with before the PR, where

fractionVisible = 0.1225
_traversableTrackExtent = 490
_lastMetrics!.extentInside = 524
_totalContentExtent = 4034
_totalTrackMainAxisOffsets = 34

To my understanding, what we want to do is merely to change how the scrollbar is painted, not the content, therefore I'm surprised to find that fractionVisible changed at all. I also discovered that there is another property mainAxisMargin that seems to be similar but slightly different, but unfortunately I don't completely understand padding's doc. I'm not sure if clearing padding is the way or should we make use of mainAxisMargin somehow.

@Piinks May I have your opinion?

D-extremity · 2024-07-23T23:12:48Z

I did some more investigation on the failed test (https://github.com/flutter/flutter/pull/150685/files#diff-c470a64409840da3df5f8b241a8b984964dd26290c65839c8ebeddcc16691155L118) and tried to get the exact number and how it is made up.

The correct height is 68.06544372830938, which is fractionVisible * _traversableTrackExtent, where

fractionVisible = 0.12989588497768964

_traversableTrackExtent = 524.0

The fractionVisible is a result of (_lastMetrics!.extentInside - _totalTrackMainAxisOffsets) / (_totalContentExtent - _totalTrackMainAxisOffsets), where

_lastMetrics!.extentInside = 524

_totalContentExtent = 4034

_totalTrackMainAxisOffsets = 0

Compare with before the PR, where

fractionVisible = 0.1225

_traversableTrackExtent = 490

_lastMetrics!.extentInside = 524

_totalContentExtent = 4034

_totalTrackMainAxisOffsets = 34

To my understanding, what we want to do is merely to change how the scrollbar is painted, not the content, therefore I'm surprised to find that fractionVisible changed at all. I also discovered that there is another property mainAxisMargin that seems to be similar but slightly different, but unfortunately I don't completely understand padding's doc. I'm not sure if clearing padding is the way or should we make use of mainAxisMargin somehow.

@Piinks May I have your opinion?

Woo!! It worked ❤️
I think when i removed padding from scrollbar that changed fractionVisible, according to your calculation sir, _totalTrackMainAxisOffsets = 34 became 0, and that 34 got added to _traversableTrackExtend = 524 (490+34).

dkwingsmt · 2024-07-23T23:19:33Z

Yeah, but I'm still concerned whether it's the correct way to go. Let's get her opinion first (she's probably OOO for a few days.)

Piinks · 2024-07-26T20:23:13Z

packages/flutter/test/cupertino/scrollbar_paint_test.dart

-      ),
-    ));
+    expect(tester.getRect(find.byType(CupertinoScrollbar)),
+        rectMoreOrLessEquals(const Rect.fromLTWH(0, 0, 800, 600), epsilon: 1)); // (800, 600) is viewport size


This does not look right, does the scrollbar overlap the navigation bar now?

What iOS does is that, if the scrollbar is full screen, then the scrollbar follows the safe area. If the scrollbar is attached to a non-fullscreen widget, such as a card, then the scrollbar shouldn't have any paddings.

So the problem is, either we have a way for the scrollbar to automatically detect what it is attached to, or its parent should be responsible to decide whether the scrollbar should use the view padding.

For the first option, if we decide that it's the parent's responsibility to warp itself in SafeArea, then we don't need this PR at all. However I don't think this is a good way, because we have way more "non-fullscreen" widgets than fullscreen widgets.

Therefore I'm basically proposing that the scrollbar should by default assume it's attached to a non-fullscreen widget, and the parent should explicitly apply view padding to the scrollbar if the parent knows itself is fullscreen.

And in this way, in case the scrollbar is used without any further indication (such as in this unit test), yes, the scrollbar overlaps the navigation bar. The unit test's behavior is expected, although the widget in the unit test is written in a discoraged way.

the parent should explicitly apply view padding to the scrollbar if the parent knows itself is fullscreen.

Wouldn't this be a breaking change then?

yes, the scrollbar overlaps the navigation bar.

As a user I would not expect this, especially since on some platforms, the scrollbar is added automatically and the user does not have as much control over it.

Alternatively we can also add a paragraph to the doc to notify the user of the default padding, and suggest them to either use a SafeArea or pass in a zero padding in non-fullscreen cases. What do you think?

Alternatively we can also add a paragraph to the doc to notify the user of the default padding, and suggest them to either use a SafeArea or pass in a zero padding in non-fullscreen cases. What do you think?

I was thinking if we provide padding from top equal to kToolBarHeight and from bottom also kToolBarHeight only when bottom navigation bar and app bar is enabled for that Widget, else padding remains 0.
I might be wrong this is just an idea which i think might work.

Users already expect that to work though without having to manually configure it. 🤔

Another more complicated solution is to detect the boundary of the Scrollbar widget in terms of screen space, and calculate the padding it needs to use dynamically. For example, if there's scrollbar is on a card that is 10 px within the edge of the screen, then the padding of the scrollbar will be viewPadding-10.

I wonder if we should take a step back, maybe I have lost sight of the root issue. The scrollbar determines its bounds based on the child it wraps, which is usually the viewport (the window scrolling is happening in).

When I look at the video in the OP of the issue, I think oh yeah that padding should totally be removed so the notch of the phone does not affect the scrollbar in the dialog. Then I realized, shouldn't the dialog handle that then? The dialog has consumed the view padding. Should the dialog remove the padding then? Scaffold does this, it removes the padding as it's children consume it, like with AppBar and BottomNavigationBar.

Yeah, I'm mostly thinking that the current default is counterintuitive, since I would consider full screen scrollbars rarer than non fullscreen ones. But I agree that we don't have to change it, but if we don't I think we should improve the doc to explain it. It surprised me when I worked on it and I don't even know which parameter to turn it off, and it took me a while to realize that the scrollbar padding equals to view padding.

@Piinks How do you think we should move on? Should we improve the doc instead (or should we at all?)

How do you think we should move on?

If we can update this change to not have the overlap of the navigation bar by default, that would be a path forward. Right now as it is, it is a significant breaking change.

Piinks · 2024-07-26T20:25:31Z

I'm surprised to find that fractionVisible changed at all.

That part looks to be expected with this change, since the padding was used to compute _totalTrackMainAxisOffsets, which goes into the calculation of fractionVisible.

Piinks · 2024-10-02T18:04:11Z

Caught up with @dkwingsmt, this is not a change we think we want to make right now, so we are going to close this for now. Thank you @D-extremity for contributing!

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository labels Jun 24, 2024

D-extremity force-pushed the issue_149659 branch 2 times, most recently from 653ef4d to 5e13139 Compare June 24, 2024 17:09

Piinks reviewed Jun 26, 2024

View reviewed changes

Piinks requested a review from dkwingsmt June 26, 2024 18:27

D-extremity force-pushed the issue_149659 branch 2 times, most recently from 0403640 to 81a73e6 Compare July 20, 2024 08:57

D-extremity requested review from Piinks July 20, 2024 09:37