Fix: stop auto-scrolling during selection handle drag - #185206
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies EditableText to prevent automatic scrolling to the caret when a selection change is caused by a drag. Review feedback indicates that the accompanying regression test will fail because it relies on the now-disabled scrolling logic when calling userUpdateTextEditingValue directly. It is recommended to update the test to use simulated drag gestures to verify the viewport's behavior.
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Hi @nicolinx, thank you for your contribution! This is definitely a worthwhile fix we want to land! I left a few comments regarding testing and my thoughts on the approach.
|
Hi @Renzo-Olivares, thanks for the detailed feedback! I've addressed everything and added the new mouse drag test. Ready for another review. |
|
Hi @Renzo-Olivares, I've updated both tests to use TestWidgetsApp and TestTextField. (I also swapped out the ScrollController for the internal ScrollableState since TestTextField doesn't take a controller parameter). It's ready for another review. Thank you! |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Thank you for the changes @nicolinx. Left a few more comments mainly around testing.
|
Hi @Renzo-Olivares , thank you for the feedback. I have addressed all the comments and it's ready for another look. Thank you! |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Hi @nicolinx, thanks for the quick changes. Left a few more comments about the tests.
|
|
||
| // Populate the viewport and scroll to the bottom to prepare for an upward drag. | ||
| scrollable.position.jumpTo(scrollable.position.maxScrollExtent); | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
nit: verify the scroll position after jumping to the end.
There was a problem hiding this comment.
Done as well. Added the verification check right after jumping to the end in the same commit.
There was a problem hiding this comment.
I think this comment still needs to be addressed.
There was a problem hiding this comment.
Sorry I missed this part. I've added a verification check to verify the scroll position is at the max scroll extent after jumping.
|
Hi @Renzo-Olivares. I have addressed all the comments and it's ready for another look. Thank you! |
| final int lastWordOffset = controller.text.length - 5; | ||
| final Offset lastWordPos = textOffsetToPosition(tester, lastWordOffset); | ||
| await tester.longPressAt(lastWordPos, pointer: 7); | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
nit: consider verifying the last word is actually selected after the pumpAndSettle.
There was a problem hiding this comment.
Done. Added verification that the last word is selected after pumpAndSettle.
| final int lastWordOffset = controller.text.length - 5; | ||
| final Offset lastWordPos = textOffsetToPosition(tester, lastWordOffset); | ||
| await tester.longPressAt(lastWordPos, pointer: 7); | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
|
|
||
| // Populate the viewport and scroll to the bottom to prepare for an upward drag. | ||
| scrollable.position.jumpTo(scrollable.position.maxScrollExtent); | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
I think this comment still needs to be addressed.
|
|
||
| // Scroll to the bottom of the long text. | ||
| scrollable.position.jumpTo(scrollable.position.maxScrollExtent); | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
nit: verify the scroll position after jumping to the end.
There was a problem hiding this comment.
Done. Added a verification check to verify the scroll position is at the max scroll extent after jumping.
| // (called via _formatAndSetValue), which ensures the active handle is kept in view. | ||
| // Bypassing the default caret auto-scroll here prevents conflicts that would snap | ||
| // the viewport back to the opposite, static selection end. | ||
| // Follow up work: https://github.com/flutter/flutter/issues/192595 |
There was a problem hiding this comment.
Consider changing follow up comment to todo:
// TODO(Renzo-Olivares): Remove this special case once the caret reveal paths are
// consolidated, https://github.com/flutter/flutter/issues/192595.
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM modulo small comment, thank you for your patience on this one!
Sure! I have updated the comment as requested. Please take another look. |
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM, thank you for the contribution!
This PR prevents the text viewport from jumping or "flickering" while a user is dragging selection handles. By disabling automated scrolling during a drag, the viewport remains stable and avoids incorrectly snapping to the bottom of the selection when the user is dragging the top handle.
Fixes #132047
Included a regression test in
packages/flutter/test/widgets/editable_text_test.dartto verify that the scroll offset remains stable during a selection drag.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.