Make EdgeDraggingAutoScroller respect ScrollPhysics#186541
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the EdgeDraggingAutoScroller to respect scroll physics that disallow user offsets, such as NeverScrollableScrollPhysics, by preventing or stopping auto-scroll. A regression test was added to ensure this behavior and prevent future regressions. Feedback was provided to simplify the implementation by removing a redundant null check on the non-nullable resolvedPhysics property, aligning with the repository's readability guidelines.
navaronbracke
left a comment
There was a problem hiding this comment.
Just some remarks on the test
| await tester.pumpWidget( | ||
| MaterialApp( | ||
| home: SelectionArea( | ||
| selectionControls: materialTextSelectionControls, |
There was a problem hiding this comment.
Can we use testTextSelectionHandleControls, per the same remark about using Material here ?
| ); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| final RenderParagraph paragraph0 = tester.renderObject<RenderParagraph>( |
There was a problem hiding this comment.
Is there a reason this requires finding the paragraph, as opposed to just using a text finder, and using for example the center of the text for the gesture start position?
There was a problem hiding this comment.
That makes sense to me. We only use RenderParagraph if we need to precisely select and/or verify the TextSelection or other RenderParagraph members.
Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>
| addTearDown(controller.dispose); | ||
| await tester.pumpWidget( | ||
| TestWidgetsApp( | ||
| home: SelectionArea( |
There was a problem hiding this comment.
We should use SelectableRegion here instead of SelectionArea to avoid using Material in the widgets library tests.
|
|
||
| // Drag past the bottom of the scrollable; this would normally trigger | ||
| // edge auto-scroll. | ||
| await gesture.moveTo(tester.getBottomRight(find.byType(ListView)) + const Offset(0, 40)); |
There was a problem hiding this comment.
nit: per the style guide use double literals for double constants https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#use-double-literals-for-double-constants
Offset(0, 40) -> Offset(0.0, 40.0).
| }); | ||
|
|
||
| testWidgets( | ||
| 'drag-select edge auto-scroll respects NeverScrollableScrollPhysics', |
There was a problem hiding this comment.
I think this can just be automatic edge scrolling respects NeverScrollableScrollPhysics.
| // A Scrollable whose physics refuses user offset (e.g. | ||
| // NeverScrollableScrollPhysics) must not be advanced by the selection | ||
| // edge auto-scroller, and the auto-scroller must not trip its drag-target | ||
| // size assertion in the process. |
There was a problem hiding this comment.
Consider the wording below to avoid specific implementation details that may not be true in the future.
// When a scrollable view with non-scrollable physics (e.g.,
// NeverScrollableScrollPhysics) is wrapped in a SelectableRegion, dragging
// a selection past the viewport boundary must not advance the scroll offset
// or throw exceptions.
|
Hi @mbcorona, thank you for working on this! The code change LGTM just some small documentation comments. |
| await tester.pump(); | ||
| await tester.pump(const Duration(seconds: 1)); | ||
|
|
||
| // The scroll position must not have advanced, and no exception (e.g. the |
There was a problem hiding this comment.
I think leaving as "no exception must have been thrown" is good enough. The specific assertion text may change so this specific comment may not be true later on.
|
|
||
| await gesture.up(); | ||
| await tester.pumpAndSettle(); | ||
| expect(tester.takeException(), isNull); |
There was a problem hiding this comment.
consider checking controller.offset again after the pumpAndSettle.
|
@Renzo-Olivares @navaronbracke — pushed addressing all your comments. Ready for re-review when you have a minute. |
|
Looks like Linux analyze is not happy with some of the formatting |
Fixes #140654
Problem
When a
ScrollablewithNeverScrollableScrollPhysicslives under aSelectionArea, drag-selecting past the edge of the viewport still auto-scrolls it, and can additionally trip the "Drag target size is larger than scrollable size, which may cause bouncing" assertion inscrollable_helpers.dart. The original report uses aPageView(physics: NeverScrollableScrollPhysics())containing aDataTable; reproduces on web, macOS, and Android, back to 3.16.Root cause
EdgeDraggingAutoScroller._scroll()advances the scroll position by callingScrollPosition.animateTo(...)directly. That bypassesScrollPhysics.applyUserOffset/applyBoundaryConditions, so aScrollablewhose physics returnsfalsefromshouldAcceptUserOffsetwas still being driven during a drag-select. The two other user-driven scroll entry points already had this guard —ScrollAction.invokeinscrollable_helpers.dartandScrollable._receivedPointerSignalinscrollable.dart— the auto-scroller was the missing one. The assertion lives inside_scroll(), so it fires only because_scroll()runs when it shouldn't; gating physics resolves both symptoms in one place.(Root cause originally diagnosed by @Renzo-Olivares on the issue thread)
Change
EdgeDraggingAutoScroller.startAutoScrollIfNecessarynow consultsscrollable.resolvedPhysics.shouldAcceptUserOffset(scrollable.position). If physics refuses user offset, itstopAutoScroll()s and returns early — mirroring the pattern at the existing guard sites. No new public API.The auto-scroller is shared infrastructure:
SliverReorderableListuses the samestartAutoScrollIfNecessaryfor drag-reorder auto-scroll. Under default physics the new guard is a pass-through, so reorder behavior is unchanged; the existingSliverReorderableList auto scrolls speed is configurabletest serves as the regression guard and still passes.Test
Added a
testWidgetsinscrollable_selection_test.dartthat mouse-drag-selects past the bottom of aListViewwrapped in aSelectionAreawithNeverScrollableScrollPhysics, then asserts the scroll offset stays at0.0and no exception is thrown. The new test fails onmasterwithExpected: <0.0> Actual: <20.0>and passes with this change.Manual verification
Tested with the original repro (a
SelectionArea>PageView(NeverScrollableScrollPhysics)with aDataTablefirst page and aTextsecond page). Before: drag-selecting near the right edge of the DataTable slid the PageView to page 2 and threw the assertion. After: PageView stays on page 1 and selection continues normally. Video attached.Screen.Recording.2026-05-14.at.11.07.19.a.m.mov
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.