[framework] Allow nested perpendicular scrollables to both scroll from a diagonal pointer signal#186638
[framework] Allow nested perpendicular scrollables to both scroll from a diagonal pointer signal#186638xlash5 wants to merge 2 commits into
Conversation
… a diagonal pointer signal When a horizontal scrollable (e.g. CarouselView, PageView) is nested inside a vertical scrollable (e.g. ListView), a two-finger trackpad diagonal scroll event was consumed entirely by the deepest scrollable, preventing the outer scrollable from receiving its perpendicular axis component. Root cause: PointerSignalResolver used a first-registered-wins model with a single callback slot. The deepest scrollable would register first and call respond(false), consuming the entire event delta including the perpendicular component intended for the outer scrollable. Fix: - PointerSignalResolver now supports multiple registrations, deduplicated by an optional Object? key. Only one registration per key is kept, allowing perpendicular-axis scrollables to each register independently while same-axis widgets (e.g. scrollable + attached scrollbar) are deduplicated. - Scrollable._receivedPointerSignal passes widget.axis as the registration key. - Scrollable._handlePointerScroll no longer calls event.respond() — the resolver handles it after all callbacks have run. - Scrollbar._receivedPointerSignal also passes position.axis as key to prevent double-scrolling its backing ScrollPosition. Fixes flutter#186634
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request modifies PointerSignalResolver to allow multiple callbacks per event, deduplicated by an optional key. This enables simultaneous scrolling on different axes for nested scrollables. The Scrollable and RawScrollbar widgets now provide their axis as a registration key. A review comment suggests deferring the clearing of internal state in the resolve method until after all callbacks have been executed to avoid side effects from re-registrations.
| _registrations.clear(); | ||
| _currentEvent = null; | ||
| for (final _Registration reg in registrations) { | ||
| try { | ||
| reg.callback(registeredEvent); | ||
| } catch (exception, stack) { | ||
| InformationCollector? collector; | ||
| assert(() { | ||
| collector = () => <DiagnosticsNode>[ | ||
| DiagnosticsProperty<PointerSignalEvent>( | ||
| 'Event', | ||
| event, | ||
| style: DiagnosticsTreeStyle.errorProperty, | ||
| ), | ||
| ]; | ||
| return true; | ||
| }()); | ||
| FlutterError.reportError( | ||
| FlutterErrorDetails( | ||
| exception: exception, | ||
| stack: stack, | ||
| library: 'gesture library', | ||
| context: ErrorDescription('while resolving a PointerSignalEvent'), | ||
| informationCollector: collector, | ||
| ), | ||
| ]; | ||
| return true; | ||
| }()); | ||
| FlutterError.reportError( | ||
| FlutterErrorDetails( | ||
| exception: exception, | ||
| stack: stack, | ||
| library: 'gesture library', | ||
| context: ErrorDescription('while resolving a PointerSignalEvent'), | ||
| informationCollector: collector, | ||
| ), | ||
| ); | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clearing the registration state (_registrations and _currentEvent) before executing the callbacks allows any calls to register made within those callbacks to persist into the next event's resolution cycle. This could lead to unexpected behavior or assertion failures when the next event is processed. It is safer to clear the state after the loop, ensuring that any re-registrations during the current resolution are correctly discarded.
for (final _Registration reg in registrations) {
try {
reg.callback(registeredEvent);
} catch (exception, stack) {
InformationCollector? collector;
assert(() {
collector = () => <DiagnosticsNode>[
DiagnosticsProperty<PointerSignalEvent>(
'Event',
event,
style: DiagnosticsTreeStyle.errorProperty,
),
];
return true;
}());
FlutterError.reportError(
FlutterErrorDetails(
exception: exception,
stack: stack,
library: 'gesture library',
context: ErrorDescription('while resolving a PointerSignalEvent'),
informationCollector: collector,
),
);
}
}
_registrations.clear();
_currentEvent = null;|
@xlash5 While allowing both to scroll is better than what's currently in place, the issue specifically mention that they should NOT both scroll, but that the vertical scrollable should take priority. Picking vertical is not arbitrary as it is the primary scroll direction. Yet this issue title says allow BOTH. |
Piinks
left a comment
There was a problem hiding this comment.
Hi @xlash5 thanks for contributing.
While the goal of enabling diagonal scrolling is clear, this implementation introduces some inconsistencies and conflicts with Flutter's 1D scrolling architecture.
Specifically:
- This uses two conflicting models—'innermost-wins' for identical keys and 'outermost-wins' for different keys. Additionally, keyless registrations bypass the filtering entirely, resulting in unpredictable behavior.
- Framework Alignment: 1D scrollables in Flutter are intentionally designed to be exclusive (innermost-wins) to ensure a stable, locked feel. Changing the PointerSignalResolver to allow diagonal components to leak into parent axes breaks this contract. True diagonal support is better handled by 2D scrolling APIs (like TwoDimensionalScrollable) rather than modifying the fundamental behavior of 1D lists.
- Docs vs. Code: The code (which uses reversed iteration and winningKey filtering) directly contradicts the PR description and doc comments, which claim that all callbacks are invoked in registration order.
Given that this fundamentally changes the expected 1D scrolling behavior and introduces a complex priority state, I don't believe we should land this as-is.
|
This pull request is being closed because it has not been updated in the last 21 days after a request for more information. If you are still working on this, please feel free to reopen it or file a new pull request. Thanks for your contribution. |
Closes #186634
Problem
When a horizontal scrollable (e.g. \CarouselView, \PageView, horizontal \ListView) is placed inside a vertical scrollable, trackpad two-finger scroll events at a diagonal angle cause the horizontal scrollable to capture the entire event — the vertical component intended for the outer scrollable is lost.
Root Cause
\PointerSignalResolver\ used a first-registered-wins model with a single callback slot. The deepest scrollable registered first and called
espond(false), consuming the entire \PointerScrollEvent\ delta — including the perpendicular component meant for the outer scrollable.
Fix
Three changes, all backwards-compatible:
1. \PointerSignalResolver\ — support multiple key-deduplicated registrations
egister(): only one registration per key is kept
esolve()\ now calls all registered callbacks in registration order, then calls
espond(false)\ once
2. \Scrollable._receivedPointerSignal\ — pass axis as registration key
esolver.register()\
3. \Scrollbar._receivedPointerSignal\ — pass axis as registration key
esolver.register(), preventing double-scroll with the backing \Scrollable\
Testing
Migration
No migration needed. The change is purely additive:
egister(event, callback)\ without a key continue to work identically (first-registration-wins for keyless callbacks)