Automatically applying Scrollbars on desktop platforms with configurable ScrollBehaviors#78588
Automatically applying Scrollbars on desktop platforms with configurable ScrollBehaviors#78588Piinks merged 38 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
f: cupertino
Piinks
added
a: desktop
Piinks
changed the title
|
|
||
| // Do not use the platform-specific default scroll configuration. | ||
| // Dropdown menus should never overscroll or display an overscroll indicator. | ||
| // The default scrollbar platforms will apply. |
There was a problem hiding this comment.
It will use the scrollbars defined in ScrollBehavior, but it should be using the ones from the behavior in the context, right?
There was a problem hiding this comment.
Ah I see what you mean, I will update the copyWith function to accommodate this.
| axisDirection: _isMultiline ? AxisDirection.down : AxisDirection.right, | ||
| controller: _scrollController, | ||
| physics: widget.scrollPhysics, | ||
| physics: widget.scrollPhysics ?? widget.scrollBehavior?.getScrollPhysics(context), |
There was a problem hiding this comment.
Is this actually necessary? When widget.scrollPhysics is null, it should be the Scrollable that does this lookup?
There was a problem hiding this comment.
Oh yeah, you are right. I guess that makes it a bit redundant here and in Scrollable.
| 'Temporary migration flag, do not use. ' | ||
| 'This feature was deprecated after v2.1.0-11.0.pre.' | ||
| ) | ||
| bool useDecoration = false, |
There was a problem hiding this comment.
Do you think we could avoid this flag altogether by making buildOverscrollIndicator just call buildViewportChrome by default?
There was a problem hiding this comment.
I am not sure what you mean. My plan is to remove buildViewportChrome, so if we are calling it, won't it then take more steps to remove it?
buildViewportChrome may not be just specific to the overscroll indicator for some of our customers currently. I may be misunderstanding. :)
Also, that would make it such that scrollbars will be built before folks have had a chance to migrate to their desired behavior..
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
LGTM assuming tests on PR and in google pass
🥳 Thanks for sticking with this!
| physics: widget.scrollPhysics, | ||
| dragStartBehavior: widget.dragStartBehavior, | ||
| restorationId: widget.restorationId, | ||
| scrollBehavior: widget.scrollBehavior ?? ScrollConfiguration.of(context).copyWith(scrollbars: _isMultiline), |
There was a problem hiding this comment.
Maybe only do the copy if _isMultiline is true? (This avoids a dependency on the ScrollConfig (which may trigger rebuilds) if we don't need it). If _isMultiline is false, we can just use null as scrollBehvaior to have the lower levels look it up.
There was a problem hiding this comment.
I think it's the other way around, if _isMultiLine is true, we want the ScrollBehavior that will apply scrollbars. If it is false, then we copy to remove the scrollbar.
Scrollbars are automatically applied on desktop since flutter/flutter#78588
Scrollbars are automatically applied on desktop since flutter/flutter#78588
|
I have found an issue with this change and how it interacts with TabBarView Widgets within a NestedScrollView Widget: Basically on desktop, it causes a scrollbar to be attached to each ScrollView in the tabs, this causes the scroll controller to be connected to more than one tab bar and throws an error. |
3 participants
Originally #75354
Design Document
This change applies
Raw/Cupertino/Scrollbars by default on the Web and Desktop platforms. I've done this in a similar way to how we apply theGlowingOverscrollIndicator, in eachWidget/Material/CupertinoScrollBehavior. This allows us to get the rightScrollbarfor the givenScrollBehavior.This current proposal requires a breaking change toUpdate: Instead I have deprecatedScrollBehavior.buildViewportChrome, adding an optionalcontrollerparameter asScrollbarrequires aScrollControllerfor full functionality.buildViewportChromein favor ofbuildViewportDecoration. Breaking the former was a really hard break, so I have introduced a new method, and added an opt-in flag (ScrollBehavior.useDecoration) so customers can be migrated gracefully.This new method will use (also new)
ScrollableDetailsto ascertain the right decoration. PassingScrollableDetailswill be a bit more future-proof, where if needed we can add to the details rather than needing to change a function signature. The ScrollableDetails currently contains the required AxisDirection (used for the GlowingOverscrollIndicator decoration), and an optional controller. If a controller is not provided, noScrollbarwill be created.Some instances of
Scrollableare excluded from this including:EditableTextListWheelScrollViewPageViewNestedScrollViewScrollBehaviorthat I have, for now, excluded scrollbars from)To give users control over this, I have added the ability to toggle this feature on and off inUpdate: This was a bit over the top, exposing ScrollBehaviors is a cleaner way for users to control defaults. That will be a prerequisite change, in #76739ThemeDataandCupertinoThemeData.Blocked by: #76739Fixes: #40107
Fixes: #70866 (Last part)
Also related as part of ScrollBehavior overhaul: #75728
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.