Repository navigation
Default Keyboard ScrollActions with PrimaryScrollController - #69795
Conversation
flutter-dashboard
Bot
added
p: cupertino_ui
| ); | ||
| } | ||
|
|
||
| result = PrimaryScrollController( |
There was a problem hiding this comment.
You probably need more than just one controller per app, no? E.g. if you have multiple routes stacked on top of each other, they can all have primary scrollables, that need to scroll independent of each other?
There was a problem hiding this comment.
In that case, would this be better placed in the Navigator like it originally was in #69616?
Piinks
changed the title
Piinks
added
a: desktop
| bool isEnabled(ScrollIntent intent) { | ||
| final FocusNode? focus = primaryFocus; | ||
| return focus != null && focus.context != null && Scrollable.of(focus.context!) != null; | ||
| final bool contextIsValid = focus != null && focus.context != null; |
There was a problem hiding this comment.
You could just move this into the if, since you only use it once.
There was a problem hiding this comment.
Or, you could leave it. The name does provide inherent documentation. Your choice.
Co-authored-by: Greg Spencer <[email protected]>
| final ScrollController _primaryScrollController = ScrollController(); | ||
|
|
||
| void _handleStatusBarTap() { | ||
| final ScrollController? _primaryScrollController = PrimaryScrollController.of(context); |
There was a problem hiding this comment.
Is there a good place where we could document on this class that it uses the PrimaryScrollController from the environment?
|
|
||
| void _handleStatusBarTap() { | ||
| if (_primaryScrollController.hasClients) { | ||
| final ScrollController? _primaryScrollController = PrimaryScrollController.of(context); |
There was a problem hiding this comment.
The comment above probably needs updating (we are no longer providing the primary scroll controller here.
| /// Another default behavior handled by the PrimaryScrollController is default | ||
| /// [ScrollAction]s. If a ScrollAction is not handled by an otherwise focused | ||
| /// part of the application, the ScrollAction will be evaluated using the scroll | ||
| /// view associated with this ScrollController, for example, when executing |
There was a problem hiding this comment.
"this ScrollController" - it's not guaranteed that "this" scrollcontroller is used. The action uses whatever PrimaryScrollController it can find in its context, no?
| ignoring: ignoreEvents, | ||
| child: child, | ||
| child: PrimaryScrollController( | ||
| controller: ScrollController(), |
There was a problem hiding this comment.
We should probably not instantiate a new controller every time this rebuilds and instead reuse the same scrollcontroller instance?
5 participants
Description
This PR adds default behavior for ScrollActions by using the PrimaryScrollController. It also moves the PrimaryScrollController that is instantiated in CupertinoPageScaffold and Scaffold to be in the _ModalScopeState, so each route can have their own PrimaryScrollController. This also places it high enough to be found by a ScrollAction from the default/root focus point.
If a Scrollable is not found enclosing the primaryFocus when evaluating a ScrollAction, it will look up the PrimaryScrollController to see if the ScrollAction can be invoked. By using the primary flag in a ScrollView, you can connect your scroll view to the PrimaryScrollController and have ScrollActions work by default.
Original PR #69616
Design Document
Related Issues
Fixes #43694
Tests
Updates tests that were broken by moving the PrimaryScrollController to the route level from the Scaffold
Added test for default primary ScrollAction execution
Fixed and un-skip 6 web tests related to ScrollActions
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
Did any tests fail when you ran them? Please read Handling breaking changes.