Add long-press-drag cursor move support for text fields by xster · Pull Request #26001 · flutter/flutter

xster · 2019-01-03T07:15:01Z

Add long press drag recognizer as a separate recognizer to the normal long press which gets canceled if moved.

Use on iOS to move cursor.

Long press drag on Android, which moves word selections rather than a collapsed cursor, will be a different PR.

Fixes #20693.

Is API breaking since it adds a new GestureRecognizerState enum though I doubt anyone uses it.

Left #26394 for Android

jslavitz

Looks good, mostly just comments.

jslavitz · 2019-01-04T18:13:28Z

Is there a need for a sourceTimeStamp in this gesture detector?

Not right now but I feel like our gesture classes are a bit relatively under-engineered for a low level API. I'm keeping it mostly consistent with the drag details which this is comparable to.

being underengineered is intentional. :-)
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#lazy-programming

jslavitz · 2019-01-04T18:56:46Z

If the touch was canceled, do you need to call onLongPressUp here?

I don't think we're still calling 'onLongPressUp' still. Which line were you referring to?

jslavitz · 2019-01-04T18:58:40Z

Would it make sense to also include the touch delta in this class?

Did you mean something similar to DragUpdateDetails? Chatted with Hans a bit and I feel like providing absolute values is better than providing relative to relative values. It's a lot easier to go from one way to the other than the reverse.

xster · 2019-01-15T10:40:26Z

I think these lack of new lines in the test data are bugs

Did the tests start failing?

I don't think this should be changed. It looks like tests that used kThreeLines set the font size to 34, which would cause the text field to insert soft breaks at the ends of the lines.

No but rather the test has always been passing by pure coincidence where we weren't actually dragging the handle in some parts of the test that tried to exercise it. Though the outcome was the same when the drag was done on the textfield behind it which scrolled nevertheless.

Looking at the existing usages of kThreeLines and kMoreThanFourLines also seem to expect that the beginning of lines match the beginnings of each test lines.

HansMuller · 2019-01-15T17:59:44Z

triggered from => triggered by?

Not sure what this is referring to exactly. Maybe a "See also" that sheds some light on "proxied events"?

ah good point. Linked

HansMuller · 2019-01-15T18:06:10Z

Recognizes a long press followed by a drag, followed by an up event.

It doesn't have to be dragged around

HansMuller · 2019-01-15T19:28:54Z

Did the tests start failing?

I don't think this should be changed. It looks like tests that used kThreeLines set the font size to 34, which would cause the text field to insert soft breaks at the ends of the lines.

xster · 2019-01-16T03:38:06Z

GitHub notifications is pretty wonky. @HansMuller ready for another look.

…tests

xster · 2019-02-07T01:53:55Z

@HansMuller this is ready for another look.
@goderbauer I didn't merge the 2 long press recognizers as we talked about. Unfortunately, the existing long press recognizer's callback signature is too spartan to make use of them. Rather than breaking that API, which would be a bit more significant, I just left the newly created one.

Hixie · 2019-02-07T04:57:49Z

+          && _getDistance(event) > postAcceptSlopTolerance;
+
+      if (event is PointerMoveEvent && (isPreAcceptSlopPastTolerance || isPostAcceptSlopPastTolerance)) {
        resolve(GestureDisposition.rejected);


I don't understand. If isPostAcceptSlopPastTolerance is true, then we know we've already been accepted, so what does it mean to reject it?

I don't have a strong opinion about this. I'm mostly leaning towards not introducing more API breaks in this PR since it was the existing behaviour (whether the gesture was accepted or not). Rejecting an accepted gesture makes future events not go through, such as the up event.

I don't understand how your comment corresponds to my question.

If isPostAcceptSlopPastTolerance is true, then we know we've already been accepted

Yes

so what does it mean to reject it?

In the existing implementation,

if (event is PointerMoveEvent && _getDistance(event) > kTouchSlop) { resolve(GestureDisposition.rejected);

causes the recognizer to stop emitting post-accept events such as the up event. I leaned towards not altering that behavior and left it in this PR though we can make that change if we feel that we have a strong reason to.

Hixie · 2019-02-07T05:01:01Z

+///
+///  * [LongPressGestureRecognizer], which cancels its gesture if a drag event
+///    occurs at any point during the long-press.
+class LongPressDragGestureRecognizer extends PrimaryPointerGestureRecognizer {


shouldn't this be a DragGestureRecognizer subclass?

The DragGestureRecognizer is unconcerned with the time dimension which is important here. Perhaps both PrimaryPointerGestureRecognizer and/or DragGestureRecognizer can be refactored to be mixins, though that would break more APIs

Presumably adding the time dimension would be what you would do in the DragGestureRecognizer subclass.

We could add a time component to DragGestureRecognizer or a move component to PrimaryPointerGestureRecognizer. I don't feel strongly either way. We can modify DragGestureRecognizer and have this subclass it instead if you prefer.

Ideally, the time and position components would both come in via mixins. We can also just not change either subclasses and do it here directly but we'd have to repeat that work for some of the other gestures that are coming up such as double tap drag.

HansMuller

LGTM

Looks like there's still feedback from Ian to deal with.

xster · 2019-02-08T20:56:31Z

@Hixie can you see if you have additional comments?

xster · 2019-02-12T23:10:42Z

I'm landing this to unblock follow up PRs. Let me know if you have additional comments @Hixie and I'll address them.

Hixie · 2019-02-15T23:15:08Z

-  void rejectGesture(int pointer) {
+  void acceptGesture(int pointer) {
+    // Ignore state 'ready' here because that would happen if this recognizer
+    // won by a sweep.


this comment doesn't make much sense to me

Hixie · 2019-02-15T23:15:29Z

+  void rejectGesture(int pointer) {
+    if (pointer == primaryPointer
+        && (state == GestureRecognizerState.possible ||
+            state == GestureRecognizerState.accepted)) {


i think we should assert that we're not accepted here. it doesn't make sense to reject when you're accepted.

It's the existing behavior since there are no knowledge of acceptance in this class yet. We're moving a step closer in this PR but I haven't changed this behavior.

Hixie · 2019-02-15T23:16:44Z

  possible,

+  /// The gesture has been definitively accepted by the recognizer.
+  accepted,


i am not convinced about adding this. I don't really understand what it gives us.

Hixie · 2019-02-15T23:17:37Z

Sorry about not getting back promptly. In general, please don't land things until you get a clear indication from everyone who has left comments that they are happy.

xster · 2019-02-16T00:30:06Z

#28032 to revert for more discussions.

xster · 2019-02-20T02:01:05Z

Some notes from offline chat:

Change the various name references to not conflate with anything from the gesture arena ("accept" and "reject") since in the gesture arena sense, the arena is done once someone accepts.
Rename to not have the word 'drag' in the recognizer since it's not a conventional drag gesture.
Consider adding new callbacks to the non-drag long press to get around not breaking APIs with existing callback signatures that don't have enough data to work with.

xster requested review from HansMuller and jslavitz January 3, 2019 07:15

googlebot added the cla: yes label Jan 3, 2019

zoechi added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages p: cupertino_ui cupertino_ui package in flutter/packages labels Jan 3, 2019

jslavitz reviewed Jan 4, 2019

View reviewed changes

xster force-pushed the long-press-drag branch from 5e77579 to 6f42ced Compare January 8, 2019 01:49

xster added the work in progress; do not review label Jan 10, 2019

xster force-pushed the long-press-drag branch 2 times, most recently from f4575ee to 3953929 Compare January 15, 2019 09:52

xster commented Jan 15, 2019

View reviewed changes

xster removed the work in progress; do not review label Jan 15, 2019

HansMuller reviewed Jan 15, 2019

View reviewed changes

xster force-pushed the long-press-drag branch from 02331c0 to 10d0c4c Compare January 16, 2019 03:36

xster added the c: API break Backwards-incompatible API changes label Jan 16, 2019

goderbauer mentioned this pull request Jan 26, 2019

Add callback when long long press event is rejected #25612

Closed

xster mentioned this pull request Feb 6, 2019

It's not possible to move the cursor in a text field by dragging #20693

Closed

xster added 10 commits February 6, 2019 16:31

start

50881c4

Bring it to work with cupertino text field

44e4418

make sure it's slops after accepting only

100ce23

Tests docs and some fixes

f810ccb

add test to material text field test too

19a9e0e

Some review comments

22bedf8

rename long press drag down -> long press drag start

7886f41

rename more stuff in tests

f0ff7df

Implement edge scrolling

a457cc6

Still debugging the test

8e454dc

xster added 2 commits February 6, 2019 17:01

Fix some merge issues

31548a9

Implement long press drag for word selection on Android and fix some …

4b6ffeb

…tests

xster removed the work in progress; do not review label Feb 7, 2019

xster mentioned this pull request Feb 7, 2019

Long press drag on Android selects word and expands selection by words #26394

Closed

Forgot that I don't need to track the origin myself

39c53b0

Hixie reviewed Feb 7, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/gestures/recognizer.dart Outdated

Hixie reviewed Feb 7, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/gestures/recognizer.dart Outdated

Hixie reviewed Feb 7, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/gestures/recognizer.dart Outdated

Hixie reviewed Feb 7, 2019

View reviewed changes

review

5725a26

HansMuller approved these changes Feb 8, 2019

View reviewed changes

xster merged commit 892c891 into flutter:master Feb 12, 2019

xster deleted the long-press-drag branch February 12, 2019 23:10

xster changed the title ~~Add long-press-drag cursor move support on iOS~~ Add long-press-drag cursor move support for text fields Feb 13, 2019

Hixie reviewed Feb 15, 2019

View reviewed changes

xster mentioned this pull request Feb 21, 2019

Add long-press-move support for text fields 2 #28242

Merged

9 of 10 tasks

xster added a commit to xster/flutter that referenced this pull request Feb 21, 2019

Add long-press-drag cursor move support on iOS (flutter#26001)

cb0ec51

mdebbar pushed a commit to mdebbar/flutter that referenced this pull request Feb 21, 2019

Add long-press-drag cursor move support on iOS (flutter#26001)

ecffa19

xster mentioned this pull request Sep 30, 2020

[iOS 13] Long press and drag to select #35010

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 7, 2021

Conversation

xster commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jslavitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xster commented Jan 16, 2019

Uh oh!

xster commented Feb 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

xster commented Feb 8, 2019

xster commented Jan 3, 2019 •

edited

Loading

xster Feb 7, 2019 •

edited

Loading

xster commented Feb 16, 2019 •

edited

Loading