Shift tap gesture #93835

justinmc · 2021-11-18T00:57:23Z

Exploring how we can do shift + tap in the least hacky way.

Really we should have a way to do this kind of thing with actions (coming soon but not soon enough).

Partial fix for #91979

Screen.Recording.2021-11-18.at.11.14.55.AM.mov

skia-gold · 2021-11-18T01:09:20Z

Gold has detected about 2 new digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/93835

justinmc · 2021-11-18T19:05:27Z

packages/flutter/lib/src/rendering/editable.dart

+  ///
+  /// Either base or extent will be moved to the last tapped position, whichever
+  /// is closest. The selection will never shrink or pivot, only grow.
+  void expandSelection({ required SelectionChangedCause cause }) {


I realized that the expand behavior from Mac has disappeared with #90684. I filed this issue to track that #93883.

Ideally the actions code in EditableTextState could share this logic, but I guess that won't be possible until we can do tap-based actions.

justinmc · 2021-11-18T19:14:13Z

packages/flutter/lib/src/material/text_field.dart

-          break;
-      }
-    }
+    super.onSingleTapUp(details);


I'm moving these removed lines into widgets/text_selection.dart to be shared with CupertinoTextField as well. There was likely a bug here that CupertinoTextField didn't support all of these different platform and PointerDevice kind distinctions. More reason that we should add support for tap actions and centralize all of this text editing logic!

justinmc · 2021-11-18T19:16:01Z

The gold failures in the comment above seem to be non-existent.

skia-gold · 2021-11-18T19:19:20Z

Gold has detected about 1 new digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/93835

LongCatIsLooong · 2021-11-18T21:32:21Z

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

+  /// True iff the shift key on a physical keyboard is currently pressed down.
+  ///
+  /// Used for shift + tap gestures.
+  bool isShiftPressed = false;


instead of keeping track of the shift pressed state here is it possible to directly get that info from the keyboard service? Maybe this? https://api.flutter.dev/flutter/services/HardwareKeyboard/logicalKeysPressed.html

Also the public field should be read-only it seems?

Yes, I was looking for something like that, thanks! I've also gotten rid of this public isShiftPressed then.

LongCatIsLooong · 2021-11-18T23:11:45Z

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

+      switch (defaultTargetPlatform) {
+        case TargetPlatform.iOS:
+        case TargetPlatform.macOS:
+          renderEditable.expandSelection(cause: SelectionChangedCause.tap);


Does the current RenderEditable interface expose a method that converts global coordinates to TextPosition? If there is then maybe we can implement expandSelection here as a private method? Eventually I think we'd like to remove all the mutating methods on RenderEditable right? So adding new mutable APIs to it increases the chance that the API removal becomes a breaking change.

Looks like yes, getPositionForPoint, and then I can set the selection with editableText.userUpdateTextEditingValue. Thanks for the suggestion, this was the biggest thing bugging me about my approach.

justinmc · 2021-11-19T21:19:06Z

@LongCatIsLooong I've updated the PR with your suggestions and I think it's a lot cleaner. I'm going to write some tests and move this out of draft mode, but let me know if you have any other qualms.

skia-gold · 2021-11-19T21:24:20Z

Gold has detected about 2 new digest(s) on patchset 6.
View them at https://flutter-gold.skia.org/cl/github/93835

skia-gold · 2021-11-20T00:04:20Z

Gold has detected about 2 new digest(s) on patchset 7.
View them at https://flutter-gold.skia.org/cl/github/93835

justinmc · 2021-11-20T00:32:26Z

packages/flutter/test/cupertino/text_field_test.dart

      expect(find.byType(CupertinoButton), findsNothing);
-    },
-  );
+  }, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS,  TargetPlatform.macOS }));


All of these tests that I added specific platforms to were testing Apple-specific behavior on all platforms. Before I moved that platform switch out of TextField and into the shared TextSelectionGestureDetectorBuilder class, CupertinoTextField was incorrectly forcing all platforms to have single taps that behaved like iOS/Mac. Now taps will behave however the platform should behave regardless of what type of field is involved.

skia-gold · 2021-11-20T00:39:20Z

Gold has detected about 2 new digest(s) on patchset 9.
View them at https://flutter-gold.skia.org/cl/github/93835

… different for other platforms

skia-gold · 2021-11-23T23:49:20Z

Gold has detected about 5 new digest(s) on patchset 11.
View them at https://flutter-gold.skia.org/cl/github/93835

justinmc · 2021-12-04T00:54:54Z

@LongCatIsLooong Ready for review.

skia-gold · 2021-12-04T01:27:46Z

Gold has detected about 1 new digest(s) on patchset 13.
View them at https://flutter-gold.skia.org/cl/github/93835

skia-gold · 2021-12-07T18:37:46Z

Gold has detected about 2 new digest(s) on patchset 14.
View them at https://flutter-gold.skia.org/cl/github/93835

flutter-dashboard · 2021-12-07T19:05:01Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

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.

Changes reported for pull request #93835 at sha 7ec9165

…er text selection toolbar

justinmc · 2021-12-08T01:06:46Z

@LongCatIsLooong Goldens are fixed and ready for re-review. There was a golden test that tested tapping "Select All" in the text selection toolbar on web, which isn't a thing, so I've skipped that test on web.

LongCatIsLooong

LGTM!

LongCatIsLooong · 2021-12-09T21:55:26Z

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

  ///    this callback.
  @protected
  void onSingleTapUp(TapUpDetails details) {
+    if (_isShiftTapping) {


This reminds me, what happens if the user presses shift and drag?

Good call, I didn't even think to try that. Looks like the drag should extend the selection created by the shift + tapdown. Unfortunately what Flutter does with this PR is to start a new selection when the drag starts. I'll merge this PR since it's ready to go, but I'll follow up with another one to handle the drag. I won't close the issue (#91979) until both PRs are merged.

justinmc added 3 commits November 17, 2021 16:31

Reuse some fragmented tapup code

f949e71

Works for handling shift + tap

bf60442

Expands selection instead of pivoting

f73848b

justinmc self-assigned this Nov 18, 2021

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Nov 18, 2021

google-cla bot added the cla: yes label Nov 18, 2021

justinmc commented Nov 18, 2021

View reviewed changes

Expand on Apple platforms, extend on others

8ae867c

justinmc commented Nov 18, 2021

View reviewed changes

justinmc requested a review from LongCatIsLooong November 18, 2021 19:16

LongCatIsLooong reviewed Nov 18, 2021

View reviewed changes

justinmc added 2 commits November 19, 2021 12:47

Use logicalKeysPressed instead of listening to presses

a2fecc3

No more new API exposed on RenderEditable

ffe9454

justinmc added 2 commits November 19, 2021 15:53

Tests

4ee0de8

Cupertino tests

f031843

Fix Cupertino tests - lots of tests testing specific Apple behavior

0852234

justinmc commented Nov 20, 2021

View reviewed changes

justinmc changed the title ~~(WIP) Shift tap gesture~~ Shift tap gesture Nov 20, 2021

flutter-dashboard bot added the f: cupertino flutter/packages/flutter/cupertino repository label Nov 20, 2021

justinmc added 2 commits November 23, 2021 15:33

Fix another test that is now platform-specific

ebc1cc7

Goldens should be only on apple platforms, or images would need to be…

acd44cb

… different for other platforms

justinmc added 2 commits December 3, 2021 16:18

Merge branch 'master' into shift-click

01f68b9

Golden tests should only be mobile since they use mobile behavior

2e21e01

justinmc force-pushed the shift-click branch from 3e5522d to 2e21e01 Compare December 4, 2021 00:52

justinmc marked this pull request as ready for review December 4, 2021 00:53

Merge branch 'master' into shift-click

7ec9165

flutter-dashboard bot added the will affect goldens Changes to golden files label Dec 7, 2021

Skip goldens on web that were incorrect due to web not having a Flutt…

6b170dc

…er text selection toolbar

LongCatIsLooong approved these changes Dec 9, 2021

View reviewed changes

justinmc mentioned this pull request Dec 10, 2021

TextField: issues with selection using shift+mouse click on the web #91979

Closed

justinmc added the waiting for tree to go green label Dec 10, 2021

fluttergithubbot merged commit 2b21c3e into flutter:master Dec 10, 2021

justinmc mentioned this pull request Dec 13, 2021

Text editing shift + tap + drag interaction #95213

Merged

justinmc deleted the shift-click branch December 29, 2021 17:57

Shift tap gesture #93835

Shift tap gesture #93835

Uh oh!

Conversation

justinmc commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skia-gold commented Nov 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Nov 18, 2021

Uh oh!

skia-gold commented Nov 18, 2021

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 Nov 19, 2021

Uh oh!

skia-gold commented Nov 19, 2021

Uh oh!

skia-gold commented Nov 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skia-gold commented Nov 20, 2021

Uh oh!

skia-gold commented Nov 23, 2021

Uh oh!

justinmc commented Dec 4, 2021

Uh oh!

skia-gold commented Dec 4, 2021

Uh oh!

skia-gold commented Dec 7, 2021

Uh oh!

flutter-dashboard bot commented Dec 7, 2021

Uh oh!

justinmc commented Dec 8, 2021

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinmc commented Nov 18, 2021 •

edited

Loading