SelectableText long press on iOS by justinmc · Pull Request #63994 · flutter/flutter

justinmc · 2020-08-17T20:48:29Z

Description

Previously, iOS did not allow selection with long press in SelectableText (only Android did). This PR enables it.

Old	New

Related Issues

Closes #62658

Tests

I've updated existing tests to check the correct behavior. There were many tests that were testing incorrect behavior (that longpressing a word just shows the menu without selecting anything).

justinmc · 2020-08-17T20:52:44Z

@chunhtai Am I misunderstanding how this should work? It seems like this behavior was deliberately removed from iOS along with lots of tests to confirm that long pressing in iOS doesn't allow selection. Take a look at the code changes I made and let me know.

I left two TODOs in the code while I confirm that this is what we want to do.

chunhtai · 2020-08-17T21:13:22Z

I copied the code from the original textfield, I am not sure why it was like this either. My guess is that probable has something to do with precision pointer like mouse or trackpad. cc @gspencergoog @dkwingsmt
If this is indeed the desire behavior we should probably update that? Furthermore, if we don't need two different version between CupertinoTextField and TextField/Selectable, we can update the parent method and don't need to override this method in the sub classes

flutter-dashboard · 2020-08-18T16:26:56Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

justinmc · 2020-08-18T17:45:48Z

Another thing I've discovered while working on this is that edge scrolling doesn't seem to work: #64059

justinmc · 2020-08-18T17:48:28Z

@gspencergoog and @dkwingsmt are there any special concerns for desktop/mouse text selection, and will I get myself into trouble by making the changes in this PR? Adding you as reviewers.

gspencergoog · 2020-08-18T17:54:27Z

I think as long as long press on a mouse doesn't select, it's probably fine. It should only happen from a touch long press.

justinmc · 2020-08-18T21:00:15Z

Ok got it. I'll plan to make these changes and just make sure that things work as I think they should, including longpressing with a mouse. I'll @ mention for review when I'm done. Thanks!

…been broken. Remove Select All test, since Select All is no longer available

justinmc · 2020-08-19T21:36:22Z

One more bug I noticed while working on this is that longpressing in a normal field on iOS doesn't seem to have the correct behavior either: #64190

justinmc · 2020-08-19T21:39:03Z

+      renderEditable.selectWordsInRange(
+        from: details.globalPosition - details.offsetFromOrigin,
+        to: details.globalPosition,
+        cause: SelectionChangedCause.longPress,
+      );


Some of this code could probably be deduplicated between here, TextField, CupertinoTextField, and TextSelectionGestureDetectorBuilder, but I think I'll have to fix #64190 first. I'll make sure I clean up any deduplication when I fix that issue.

Same with onSingleLongTapStart below.

I just double check it seems like selectable text is correct, but there will be different for long press on textfield when it has keyboard focus vs when it does not

@chunhtai Are you talking about #64190 specifically or this PR?

I meant we probably can't merge those methods for selectableText and TextField/CupertinoTextField because they have different behavior when long pressed in native iOS

SelectableText -> always select the word Textfield -> If keyboard focused, then move collapsed cursor to the word boundary and shows selectAll -> if keyboard not focused, select the word

justinmc · 2020-08-19T21:42:50Z

    }),
  );
-
-  testWidgets('The handles show after pressing Select All (iOS and Mac)', (WidgetTester tester) async {


I believe there is now no way to make Select All appear for a SelectableText on iOS, so I've removed this test.

That seems consistent with native iOS in my experimentation.

justinmc · 2020-08-19T21:44:14Z

-  }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS,  TargetPlatform.macOS }));
+  },
+    variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS,  TargetPlatform.macOS }),
+    skip: true, // https://github.com/flutter/flutter/issues/64059


I don't know why this test was passing in the first place, but it started failing when I made this change, despite the behavior it describes being broken either way. I've created the issue #64059 to deal with fixing it and unskipping it.

justinmc · 2020-08-19T22:05:43Z

@gspencergoog @dkwingsmt @chunhtai Ready for final review.

Here is the long press behavior on each platform now:

iOS	Android	Mac
Word selected, Copy shows.	Word selected, Copy and Select All show.	Nothing

That seems consistent with native in all cases to me.

chunhtai · 2020-08-20T23:38:42Z

-          Feedback.forLongPress(_state.context);
-          break;
-      }
+      renderEditable.selectWord(cause: SelectionChangedCause.longPress);


How do we differentiate between precision devices? the mouse and stylus should not make a selection right?

Hmm I'm not sure but that would make sense. @gspencergoog Didn't you have a keyboard and mouse setup for an Android device? Is there different behavior for longpressing with the mouse vs. a finger?

for macOS at least, the mouse long press does not select the word

That continues to work even after this change. The behavior on all 3 platforms is shown in #63994 (comment).

chunhtai · 2020-08-20T23:43:28Z

+      renderEditable.selectWordsInRange(
+        from: details.globalPosition - details.offsetFromOrigin,
+        to: details.globalPosition,
+        cause: SelectionChangedCause.longPress,
+      );


I just double check it seems like selectable text is correct, but there will be different for long press on textfield when it has keyboard focus vs when it does not

chunhtai

LGTM, although I found a clipping issue that is parallel to this. I am using iphone XR

justinmc · 2020-08-28T17:25:15Z

@chunhtai Is there an issue for that clipping bug? It's not something I've seen before I don't think.

chunhtai · 2020-08-28T17:33:51Z

I don't think so, it is also hard to reproduce. I can only reproduce it on my physical device.

justinmc added 2 commits August 17, 2020 13:40

Fix by removing the deliberate special case for Apple platforms

5d7f86b

Update tests to expect long press to select. Some unfixed still

42b31e9

justinmc requested a review from chunhtai August 17, 2020 20:48

justinmc self-assigned this Aug 17, 2020

flutter-dashboard Bot added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Aug 17, 2020

googlebot added the cla: yes label Aug 17, 2020

justinmc marked this pull request as draft August 17, 2020 20:48

justinmc requested review from dkwingsmt and gspencergoog August 18, 2020 17:48

Skip edge scrolling test for now since that seems to have previously …

359cf8c

…been broken. Remove Select All test, since Select All is no longer available

justinmc commented Aug 19, 2020

View reviewed changes

flutter-dashboard Bot added the work in progress; do not review label Aug 19, 2020

Remove an accidental extra space

f739630

justinmc removed the work in progress; do not review label Aug 20, 2020

justinmc marked this pull request as ready for review August 20, 2020 18:55

Merge branch 'master' into selectable-ios-long-press

f7bdc25

chunhtai reviewed Aug 20, 2020

View reviewed changes

justinmc mentioned this pull request Aug 25, 2020

Support double-tap-drag gesture to select a block of text #64550

Closed

Merge branch 'master' into selectable-ios-long-press

4467405

chunhtai reviewed Aug 27, 2020

View reviewed changes

chunhtai approved these changes Aug 27, 2020

View reviewed changes

justinmc added the waiting for tree to go green label Aug 28, 2020

fluttergithubbot merged commit 8f370df into flutter:master Aug 28, 2020

justinmc deleted the selectable-ios-long-press branch August 28, 2020 19:47

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

SelectableText long press on iOS (flutter#63994)

7c8e89b

Jethro87 mentioned this pull request Sep 18, 2020

[regression on beta] double tapping to select text doesn't show toolbar and long hold in the textfield sometimes causes ghosting effect from toolbar #66149

Closed

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

Conversation

justinmc commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Uh oh!

justinmc commented Aug 17, 2020

Uh oh!

chunhtai commented Aug 17, 2020

Uh oh!

flutter-dashboard Bot commented Aug 18, 2020

Uh oh!

justinmc commented Aug 18, 2020

Uh oh!

justinmc commented Aug 18, 2020

Uh oh!

gspencergoog commented Aug 18, 2020

Uh oh!

justinmc commented Aug 18, 2020

Uh oh!

justinmc commented Aug 19, 2020

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!

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!

justinmc commented Aug 19, 2020

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!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc commented Aug 28, 2020

Uh oh!

chunhtai commented Aug 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

justinmc commented Aug 17, 2020 •

edited

Loading