Set selection on tap down for desktop platforms and tap up for mobile by Renzo-Olivares · Pull Request #105505 · flutter/flutter

Renzo-Olivares · 2022-06-07T07:15:22Z

On desktop platforms such as MacOS, Linux, Windows, and Web, the selection in an input field should be set during a tap down on the input field at a position.

On mobile platforms such as Android, Fuchsia, and iOS, the selection in an input field should be set during a tap up on the input field at a position.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

justinmc

LGTM with some nits 👍

justinmc · 2022-06-17T16:19:48Z

packages/flutter/lib/src/widgets/text_selection.dart

+          // On macOS/iOS/iPadOS a touch tap places the cursor at the edge
+          // of the word.


Nit: Would this comment be better above the touch case?

justinmc · 2022-06-17T16:21:10Z

packages/flutter/lib/src/widgets/text_selection.dart

+          break;
+        case TargetPlatform.iOS:
+          if (isShiftPressedValid) {
+            // On these platforms, a shift-tapped unfocused field expands from 0,


Nit: "these platforms" => "this platform" or "iOS"

justinmc · 2022-06-17T16:22:24Z

packages/flutter/test/cupertino/text_field_test.dart

+    await gesture.down(gPos);
+    await tester.pumpAndSettle();
+    expect(controller.selection.baseOffset, 8);
+    expect(controller.selection.extentOffset, 8);


Nit: Maybe do another gesture.up after this and expect that the selection is still the same? Maybe that's a waste, up to you.

justinmc · 2022-06-17T16:23:54Z

packages/flutter/test/cupertino/text_field_test.dart

+
+    await touchGesture.down(gPos);
+    await tester.pumpAndSettle();
+    // On iOS a tap to select, selects the word edge instead of the exact tap position.


Nit: "On iOS a tap to select," => "On iOS, a tap to select"

justinmc · 2022-06-17T16:26:28Z

packages/flutter/test/cupertino/text_field_test.dart

+    expect(controller.selection.baseOffset, isTargetPlatformApple ? 4 : 5);
+    expect(controller.selection.extentOffset, isTargetPlatformApple ? 4 : 5);


Wait so on tap down it moves to 4/5, and then on tap up it goes to 8? I was expecting that on down it would do nothing.

Oh I can see how this can be unclear. It moves to "4/5" on the last tap up before this tap down, but I check it after this tap down. I'll check it after the tap up as well to make it clearer.

justinmc · 2022-06-17T16:28:11Z

packages/flutter/test/material/text_field_test.dart

    expect(controller.selection.baseOffset, 8);
    expect(controller.selection.extentOffset, 26);
-  }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS,  TargetPlatform.macOS }));
+  }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS, TargetPlatform.macOS }));


Good catch, I've seen this elsewhere too.

Renzo-Olivares · 2022-06-17T17:46:59Z

packages/flutter/test/cupertino/text_field_test.dart

    expect(controller.selection.extentOffset, 23);

    // Expand the selection a bit.
+    if (isTargetPlatformMobile) {


@justinmc This is where the 23->24 I was talking about is.

Renzo-Olivares · 2022-06-17T17:47:55Z

packages/flutter/test/cupertino/text_field_test.dart


    // Expand the selection a bit.
+    if (isTargetPlatformMobile) {
+      await gesture.down(textOffsetToPosition(tester, 7));


@justinmc The 8->7 I was referring to.

LongCatIsLooong · 2022-06-17T21:38:46Z

On desktop platforms such as MacOS, Linux, Windows, and Web, the selection in an input field should be set during a tap down on the input field at a position.

On mobile platforms such as Android, Fuchsia, and iOS, the selection in an input field should be set during a tap up on the input field at a position.

Could you confirm that this is an operating system difference instead of a touch device thing (e.g., touch versus mouse click)?

Renzo-Olivares · 2022-06-17T21:57:43Z

On desktop platforms such as MacOS, Linux, Windows, and Web, the selection in an input field should be set during a tap down on the input field at a position.
On mobile platforms such as Android, Fuchsia, and iOS, the selection in an input field should be set during a tap up on the input field at a position.

Could you confirm that this is an operating system difference instead of a touch device thing (e.g., touch versus mouse click)?

On my pixel 6 pro tested on google keep with a mouse the selection is set on tap up. Same with touch.
On my iPad with a mouse the selection is set on tap up, to the precise click location. With touch it is set to the word edge closest to the tap location. Both happen on tap up."
On macOS tap on trackpad or click on track pad sets the selection on tap down. Not sure if there is a way to simulate "touch".
On Windows the selection is set on tap down with a mouse. I'm not able to confirm if this is the same with a touchscreen windows device.
On Linux the selection is set on tap down with a mouse. I'm not able to confirm if this is the same with a touchscreen linux device.

justinmc

LGTM 👍

Thanks for straightening this out!

justinmc · 2022-06-29T20:04:05Z

packages/flutter/test/cupertino/text_field_test.dart

-        controller.selection,
-        const TextSelection.collapsed(offset: 7, affinity: TextAffinity.upstream),
-      );
+      // Place collapsed selection.


Why did "Plain" change to "Place"?

I thought it was a typo, but I understand what was meant by plain now. Changing it back.

justinmc · 2022-06-29T20:05:03Z

packages/flutter/test/cupertino/text_field_test.dart

+      expect(controller.selection.baseOffset, isTargetPlatformMobile ? 7 : 6);
+      expect(controller.selection.extentOffset, isTargetPlatformMobile ? 7 : 6);


Nit: Not sure if this is better but here's another option:

expect(controller.selection.isCollapsed, isTrue); expect(controller.selection.baseOffset, isTargetPlatformMobile ? 7 : 6);

And I think there are a few other places with the same pattern.

I like that pattern. I'll change it, thanks!

justinmc · 2022-06-29T20:09:31Z

packages/flutter/test/material/text_field_test.dart

+    expect(controller.selection.baseOffset, 8);
+    expect(controller.selection.extentOffset, 8);
+  },
+      variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.linux, TargetPlatform.macOS, TargetPlatform.windows })


Nit: There's also TargetPlatformVariant.desktop FYI.

justinmc · 2022-06-29T20:10:14Z

packages/flutter/test/material/text_field_test.dart

+    expect(controller.selection.baseOffset, 8);
+    expect(controller.selection.extentOffset, 8);
+  },
+      variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.android, TargetPlatform.fuchsia, TargetPlatform.iOS })


Nit: And likewise there is TargetPlatformVariant.mobile.

…bile on tap up

…lick

…r mobile (flutter/flutter#105505)

…flutter#105505) Co-authored-by: Renzo Olivares <[email protected]>

…r mobile (flutter/flutter#105505)

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Jun 7, 2022

Renzo-Olivares force-pushed the select_on_tapdown branch from 75fd559 to 1c977df Compare June 9, 2022 20:22

Renzo-Olivares marked this pull request as ready for review June 9, 2022 21:11

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. f: cupertino flutter/packages/flutter/cupertino repository labels Jun 9, 2022

Renzo-Olivares requested review from LongCatIsLooong and justinmc June 16, 2022 23:22

justinmc approved these changes Jun 17, 2022

View reviewed changes

Renzo-Olivares commented Jun 17, 2022

View reviewed changes

Renzo-Olivares force-pushed the select_on_tapdown branch from 40e5d7e to 3d9f2fe Compare June 19, 2022 08:34

Renzo-Olivares requested a review from justinmc June 29, 2022 00:21

flutter-dashboard bot added the f: focus Focus traversal, gaining or losing focus label Jun 29, 2022

justinmc approved these changes Jun 29, 2022

View reviewed changes

Renzo Olivares added 13 commits June 29, 2022 15:32

For desktop platforms selection should be set on tap down, and for mo…

39c1380

…bile on tap up

don't execute tap down if selection is disabled

aa1d6c0

update some tests

cfdc8f1

fix more tests

34045f7

fix reverse tests

870d186

fix tests

e59af46

fix indentations

5106c45

add tests

c10d744

avoid redundant arguments

7bf5c67

updates

f4ce3b5

fix tests

45148b7

Fix tests - ios/cupertino

f9e87a4

fix focus test

13c5056

Renzo Olivares added 8 commits June 29, 2022 15:32

Address nits

bd2dd31

on macOS we always select the precise position regardless of tap or c…

743c11b

…lick

Add some comments for iOS/macOS differing selection behavior in tests

ec0cd65

fix test

873d06c

make tests more clear by using textOffsetToPosition

fa3e511

clean up from last commit

056dfbc

Address reviewer comments

df4ad99

update comments

d59bfd2

Renzo-Olivares force-pushed the select_on_tapdown branch from 631be8f to d59bfd2 Compare June 29, 2022 22:32

Renzo-Olivares merged commit 4c1d887 into flutter:master Jun 29, 2022

Renzo-Olivares deleted the select_on_tapdown branch June 29, 2022 23:35

engine-flutter-autoroll mentioned this pull request Jun 30, 2022

Roll Flutter from 3b11ad8cfe78 to 6c6ae063814c (28 revisions) flutter/plugins#6060

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 30, 2022

4c1d887 Set selection on tap down for desktop platforms and tap up fo…

c8b3326

…r mobile (flutter/flutter#105505)

engine-flutter-autoroll mentioned this pull request Jun 30, 2022

Roll Flutter from 2c13db3b22dd to 6c6ae063814c (16 revisions) flutter/packages#2296

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 30, 2022

4c1d887 Set selection on tap down for desktop platforms and tap up fo…

e1a305b

…r mobile (flutter/flutter#105505)

engine-flutter-autoroll mentioned this pull request Jun 30, 2022

Roll Flutter from 2c13db3b22dd to cf6b91d0ec2a (17 revisions) flutter/packages#2297

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 30, 2022

4c1d887 Set selection on tap down for desktop platforms and tap up fo…

e6ebdaf

…r mobile (flutter/flutter#105505)

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022

Set selection on tap down for desktop platforms and tap up for mobile (…

1bc2604

…flutter#105505) Co-authored-by: Renzo Olivares <[email protected]>

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022

4c1d887 Set selection on tap down for desktop platforms and tap up fo…

daa8154

…r mobile (flutter/flutter#105505)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022

4c1d887 Set selection on tap down for desktop platforms and tap up fo…

bebb02b

…r mobile (flutter/flutter#105505)

		// On macOS/iOS/iPadOS a touch tap places the cursor at the edge
		// of the word.

		expect(controller.selection.baseOffset, isTargetPlatformApple ? 4 : 5);
		expect(controller.selection.extentOffset, isTargetPlatformApple ? 4 : 5);

Conversation

Renzo-Olivares commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

justinmc 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!

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!

LongCatIsLooong commented Jun 17, 2022

Uh oh!

Renzo-Olivares commented Jun 17, 2022

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Jun 29, 2022 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Renzo-Olivares commented Jun 7, 2022 •

edited

Loading

Renzo-Olivares Jun 29, 2022 •

edited

Loading