moving the left handle automatically scrolls EditableText to the right handle, which doesn't happen on native - #105836

cc @chunhtai

@justinmc @chunhtai
any updates?

This whole comment assumes that there's no way to drag the right handle without it already being in view. If that's a wrong assumption, then disregard :).

I wonder if this is the correct solution for this problem... The reason why this is not a problem for the second selection handle is that the call to selectionDelegate.bringIntoView in _handleSelectionHandleChanged always does nothing since dragging the second selection handle means the end of the selection extent is already in view. It gets called for both left and right handles, but never ever does anything for the right handle.

if we know that it shouldn't be called when dragging the left handle, and never does anything when dragging the right handle - do we need this call at all? Can't we just delete it and all is well?

@justinmc
thank you for review :) I fixed it based on your review.
but I'm not sure where I should write enum. Where is the most optimal location to write enum?

@antholeole
thank you for your comments!
Why doesbringIntoView exist there originally? actually, I can remove the line and it passed all tests in my local...

@justinmc
and I think this issue is mainly on mobile, so I changed TargetPlatformVariant.all() to TargetPlatformVariant.mobile() in tests

If we go with @antholeole's approach, can we get rid of the idea of isEnd altogether? That would make things really simple.

@antholeole thank you for your comments! Why doesbringIntoView exist there originally? actually, I can remove the line and it passed all tests in my local...

@takassh It was an old implementation of edge scrolling, I think. Edge scrolling was moved elsewhere, but this line was overlooked. I think it is the sole culprit of #105821 . When you said "passes all tests" are you including the tests you wrote for this PR? If so, I think it would be best (i.e. smallest footprint) if this PR was just deleting that line, and the tests you wrote.

@justinmc @antholeole
I see. thank you.
yeah, I said I could remove selectionDelegate.bringIntoView in _handleSelectionHandleChanged only when there is my fix.
however, we can't remove other bringIntoView to pass tests. e.g. _editableText?.bringIntoView in _handleSelectionChanged.

it seems selectionDelegate.userUpdateTextEditingValue in _handleSelectionHandleChanged internally calls other bringIntoView also. (in short , as a result, _handleSelectionHandleChanged is calling bringIntoView two times now)
therefore, anyway I think we need some flag like isEnd to tell which handle is dragged now.

@takassh So, just to be clear, when you remove the line that I mentioned, #105821 still happens? I removed that line and this is the behavior I get:

It looks like it fixes the problem for me, unless I'm testing wrong.

EDIT: Oh I see, it's behind a platform switch flag. Let me take another look, you may be right!

Looks like you're right! The further calls are needed on platforms not iOS / macOS. thanks for this!

@antholeole

So, just to be clear, when you remove the line that I mentioned, #105821 still happens?

yes

@justinmc
so I think we need the enum. where should I write enum definition?

@justinmc what about changing ‘SelectionChangedCause.drag’ to something like SelectionChangedCause.dragLeftHandle and SelectionChangedCause.dragRightHandle?

Just a thought!

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@justinmc
any updates?
Is there anything I can do?

@takassh I'll be taking over the review on this one :) sorry about the slowness, I'll get this reviewed asap. It looks almost done though, good job!

Something like this:

diff --git a/packages/flutter/lib/src/widgets/text_selection.dart b/packages/flutter/lib/src/widgets/text_selection.dart
index 30f96187ba..e59d176fd9 100644
--- a/packages/flutter/lib/src/widgets/text_selection.dart
+++ b/packages/flutter/lib/src/widgets/text_selection.dart
@@ -2286,6 +2286,11 @@ class ClipboardStatusNotifier extends ValueNotifier<ClipboardStatus> with Widget
   }
 }
 
+enum TextSelectionHandleSide {
+  BaseHandle,
+  ExtentHandle
+}
+
 /// An enumeration of the status of the content on the user's clipboard.
 enum ClipboardStatus {
   /// The clipboard content can be pasted, such as a String of nonzero length.

@antholeole
thank you for reviewing!
I fixed bool IsEnd to TextSelectionDraggedHandleSide handleSide.

however, I encountered following CI failure, and it seems not to be my failure, because I didn't touch about flutter_logo in this PR.

| Warning - golden differed less than .06% (0.006202083333333334%), ignoring failure but producing output
| Golden "golden_widget/flutter_logo.string.color_filter.png": Pixel test failed, 0.62% diff detected.
| Failure feedback can be found at /b/s/w/ir/x/t/flutter_customer_testing.flutter_svg.DAWBDP/tests/test/failures
...
| Warning - golden differed less than .06% (0.008170833333333334%), ignoring failure but producing output
| Golden "golden_widget/flutter_logo.string.color_filter.blendmode_color.png": Pixel test failed, 0.82% diff detected.
| Failure feedback can be found at /b/s/w/ir/x/t/flutter_customer_testing.flutter_svg.DAWBDP/tests/test/failures

Could you check the CI failure?

@antholeole
wow, thank you for quick review!!

Code was changed based on your comment without #105836 (comment)

@justinmc
hi 👋 long time to see you!
I updated from your new comments and my pr was synched with the latest master branch.

Thanks for the quick response, merged!