Support keyboard selection in SelectabledRegion #112584

chunhtai · 2022-09-28T18:59:38Z

added two new selection events, and implement the handling method in renderparagraph and SelectionContainerDelegate.

GranularlyExtendSelectionEvent: handles shift+modifier+left/right
DirectionallyExtendSelectionEvent: handles shift + up/down

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

LongCatIsLooong

I think I need to understand how seletable region works before I can do a full review.

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

LongCatIsLooong · 2022-10-13T00:37:24Z

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

+  SelectionResult _handleDirectionallyExtendSelection(double horizontalBaseline, bool isExtent, DirectionalMovement movement) {
+    final Matrix4 transform = paragraph.getTransformTo(null);
+    transform.invert();
+    final double baselineInParagraphCoordinates = MatrixUtils.transformPoint(transform, Offset(horizontalBaseline, 0)).dx;


Check the coordinates are finite?

it can be infinite, i add a check to make sure it is not nan

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

LongCatIsLooong · 2022-10-13T01:21:38Z

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

@@ -830,6 +840,64 @@ class _SelectableRegionState extends State<SelectableRegion> with TextSelectionD
    await Clipboard.setData(ClipboardData(text: data.plainText));
  }

+  bool? _adjustingSelectionExtent;
+  bool _determineIsAdjustingSelectionExtend(bool forward) {
+    if (_adjustingSelectionExtent != null) {


why do we need to cache the result? the computation doesn't seem to be very expensive? Also the method takes a parameter so shouldn't the method first check if the cache is valid?

The cache is here so that we will be moving the same selection handle once decided until user starts a new selection(a clear selection will be called).

Ah ok I see now. But since start can be larger than end, shouldn't end always represent the active edge?

I am not sure if I understand. why end always represent the active edge given that start can be larger or smaller?

According to https://api.flutter.dev/flutter/services/TextSelection/extent.html

When the user uses the arrow keys to adjust the selection, this is the value that changes.

Does it make sense to do the same here?

Is the document up to date? I thought even for TextField this may not be the case, it will pick the one that will expand the selection?

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

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

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

chunhtai · 2022-10-17T21:53:51Z

Thanks @LongCatIsLooong I addressed all the comments, PTAL

chunhtai · 2022-10-20T17:07:26Z

a friendly bump

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

LongCatIsLooong · 2022-10-29T04:43:46Z

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

+  nextLine,
+
+  /// Extends the selection to by moving the selection edges forward to a
+  /// certain horizontal offset in the same line.


maybe: Extends a selection edge toward the writing direction of the text under that selection edge? It may not be the same line.

I can't think of a case it would be in a different line. I can add more description.

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

LongCatIsLooong · 2022-10-29T05:30:51Z

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

+    } else if (start.localPosition.dy < end.localPosition.dy) {
+      isReversed = false;
+    } else {
+      isReversed = start.localPosition.dx > end.localPosition.dx;


This doesn't work for RTL text? forward means move towards the writing direction right?

I just tested with chrome, regardless of the text direction, forward=true will always move the selection handle that are lower in screen order. forward =false move the one one higher in screen order. so the logic here should sill hold.

Ah ok, I think macOS does the same for bidi when moving the caret (but that may not be true for deletion). Maybe rename this to left/right instead of forward/backward? I think in editable text land I used forward/backward for logical direction. When we unify the API it will be nice to make that consistent.

I renamed extent to end. I am using start and end to describe the handles. Do you think this is good enough? If you meant the input parameter forward I think that is accurate since it represent arrow right/arrow down.

LongCatIsLooong · 2022-10-29T05:59:15Z

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

@@ -830,6 +840,64 @@ class _SelectableRegionState extends State<SelectableRegion> with TextSelectionD
    await Clipboard.setData(ClipboardData(text: data.plainText));
  }

+  bool? _adjustingSelectionExtent;
+  bool _determineIsAdjustingSelectionExtend(bool forward) {
+    if (_adjustingSelectionExtent != null) {


Ah ok I see now. But since start can be larger than end, shouldn't end always represent the active edge?

justinmc

Reading this really makes me wish that we could unify the selection code here and in RenderEditable. I think this logic is partially duplicated in EditableText's Action handlers. Maybe the core problem is the RenderEditable/RenderParagraph divide. It is nice that this can receive the same Intents from DefaultTextEditingShortcuts at least, though.

It's also hard to be confident that our text selection keyboard shortcuts work identically in EditableText and SelectableRegion. I wonder if it would be possible to write a test that could be reused by both? Like sending keyboard keys should be the same for both, and both could produce a TextSelection where the same expects could be used. Then you could write a test that goes through all of typical keyboard shortcuts for modifying selection (not editing) and use it for both.

I'm just daydreaming though, it might not be feasible to write something like that in this PR...

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

justinmc · 2022-10-31T22:13:32Z

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

+///
+/// The [GranularlyExtendSelectionEvent] uses this enum to describe how
+/// [Selectable] should extend their selection.
+enum TextGranularity {


CC @Renzo-Olivares since this reminds me of your work on longpress dragging by word and triple click dragging by line.

justinmc · 2022-10-31T22:17:43Z

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

+  TextSelection getLineAtOffset(TextPosition position) {
+    final TextRange line = paragraph._getLineAtOffset(position);
+    final int start = line.start.clamp(range.start, range.end); // ignore_clamp_double_lint
+    final int end = line.end.clamp(range.start, range.end); // ignore_clamp_double_lint


Why can't you use clampDouble for these?

This is int type, so we should not use clampDouble.

the purpose of this ignore_clamp_double_lint is to make sure we DON'T use the .clamp method on double. The problem is that our current lint can't distinguish between .clamp called on int or double or any num subclass. So the current suggestion is to add this ignore if you are sure the clamp is called on non-double data type.

Ah I missed that, thanks for the explanation!

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

justinmc · 2022-10-31T22:28:33Z

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

@@ -830,6 +840,61 @@ class _SelectableRegionState extends State<SelectableRegion> with TextSelectionD
    await Clipboard.setData(ClipboardData(text: data.plainText));
  }

+  bool? _adjustingSelectionExtent;
+  bool _determineIsAdjustingSelectionExtend(bool forward) {


"Extend" => "Extent"?

I renamed it to End which is more accurate

justinmc · 2022-10-31T22:30:58Z

packages/flutter/test/rendering/paragraph_test.dart

          ),
        );
        selectable.dispatchSelectionEvent(
          SelectionEdgeUpdateEvent.forEnd(
-            globalPosition: paragraph.getOffsetForCaret(end, Rect.zero),
+            globalPosition: paragraph.getOffsetForCaret(end, Rect.zero) + const Offset(0, 5),


Why did you need to add that small y offset?

originally it was selecting right at the edge which can select across line if there is multi line text. It was ok because there was no multi line test in this file prior

justinmc · 2022-10-31T22:32:54Z

packages/flutter/test/rendering/paragraph_test.dart

+        ),
+      );
+      selection = paragraph.selections[0];
+      expect(selection.start, 4); // how `ar`e you


I think elsewhere in our tests I have seen how [ar]e you or how |ar|e you. Maybe we should standardize at some point.

No action needed though, just thinking out loud.

justinmc · 2022-10-31T22:34:11Z

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

-    const SingleActivator(LogicalKeyboardKey.arrowUp, shift: true, alt: true): const ExtendSelectionToLineBreakIntent(forward: false, collapseSelection: false, collapseAtReversal: true),
-    const SingleActivator(LogicalKeyboardKey.arrowDown, shift: true, alt: true): const ExtendSelectionToLineBreakIntent(forward: true, collapseSelection: false, collapseAtReversal: true),
+    const SingleActivator(LogicalKeyboardKey.arrowUp, shift: true, alt: true): const ExtendSelectionVerticallyToAdjacentLineIntent(forward: false, collapseSelection: false),
+    const SingleActivator(LogicalKeyboardKey.arrowDown, shift: true, alt: true): const ExtendSelectionVerticallyToAdjacentLineIntent(forward: true, collapseSelection: false),


I guess this was incorrect?

no they were not

Ok same behavior, different Intent 👍

justinmc · 2022-10-31T22:54:52Z

packages/flutter/test/widgets/scrollable_selection_test.dart

@@ -11,6 +11,27 @@ import 'package:flutter_test/flutter_test.dart';

 import 'clipboard_utils.dart';

+Future<void> sendKeyCombination(


Maybe we should get rid of sendKeys in editable_text_test and just use this. I like the idea of just using a SingleActivator.

I see that you added a copy of this function to another test file. It looks like this is already public though, maybe just add it to a shared _utils.dart file?

chunhtai · 2022-11-01T16:23:32Z

Reading this really makes me wish that we could unify the selection code here and in RenderEditable

That is my plan once the selectable region is at least feature on part with the EditableText.

justinmc

LGTM 👍

Thanks for the updates.

justinmc · 2022-11-02T17:13:08Z

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

+  TextPosition _getTextPositionAbove(TextPosition position) {
+    // The caret offset gives a location in the upper left hand corner of
+    // the caret so the middle of the line above is a half line above that
+    // point and the line below is 1.5 lines below that point.


Super nit here and below: You mention the caret here, but there's nothing in this method directly related to the caret (I see that you're using getOffsetForCaret in _getTextPositionVertical, but it's not obvious looking only at this method why you mention the caret).

Also similarly, it's not clear in this method why you're trying to find a point in the middle of the line (I guess you just need a point that's going to give you what you want from getPositionForOffset).

justinmc · 2022-11-02T17:14:46Z

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

+  final TextGranularity granularity;
+}
+
+/// The directional to extend a selection.


"directional" => "direction"

justinmc · 2022-11-02T17:17:46Z

packages/flutter/test/widgets/keyboard_utils.dart

+import 'package:flutter/widgets.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+Future<void> sendKeyCombination(


Looks good, thank you for taking the time to make this reusable!

LongCatIsLooong

LGTM for the most part.

Question: what order are selectables in? I guess what I don't understand is that why we can hand the event to the next selectable in the list instead of having to look for the next closest selectable in that direction?

LongCatIsLooong · 2022-11-03T06:20:56Z

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

+        if (result == SelectionResult.previous) {
+          if (targetIndex > 0) {
+            targetIndex -= 1;
+            dispatchSelectionEventToChild(


Is it possible for the result returned to not be .end? The previous selectable, if exists, is guaranteed to be able to handle the event by itself?

for instance what if the command was "extend to document boundary"?

The event is DirectionallyExtendSelectionEvent with SelectionExtendDirection.backward it must not return anything other than end. I will add some more check and documentation

chunhtai · 2022-11-03T16:41:48Z

The selectables are in screen order by default.

auto-submit · 2022-11-03T18:13:15Z

auto label is removed for flutter/flutter, pr: 112584, due to - The status or check suite Mac tool_tests_commands has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ter#112584)

* 77d6ecb 805b145d7 [web] Improve line breaker test exceptions (flutter/engine#37244) (flutter/flutter#114688) * 5233fd9 Roll Plugins from 5c11747 to a279b9d (6 revisions) (flutter/flutter#114689) * e1166e4 287a3ab59 Roll Skia from c901cb6ae66f to 1272b520c082 (1 revision) (flutter/engine#37314) (flutter/flutter#114691) * 48457d7 WidgetController.startGesture trackpad support (flutter/flutter#114631) * c1d2b85 Change some required nullable parameters in tool to non-null (flutter/flutter#114115) * cfb2f15 Roll Flutter Engine from 287a3ab59269 to e947833ceb5b (2 revisions) (flutter/flutter#114696) * 80bf355 Support keyboard selection in SelectabledRegion (flutter/flutter#112584) * be83c98 5117e858e Fix typo in overlay platform view iOS test (flutter/engine#37262) (flutter/flutter#114705) * 6ea01e6 Roll Flutter Engine from 5117e858ed1e to 6950689ed775 (3 revisions) (flutter/flutter#114710) * b187bc4 Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (flutter/flutter#114637) * 01507ba Revert "Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (#114637)" (flutter/flutter#114715) * 91a56cf roll packages (flutter/flutter#114366) * d68af26 Roll Flutter Engine from 6950689ed775 to 0933ca485a07 (3 revisions) (flutter/flutter#114714) * 4fd0a92 Remove auto_cleanup of xcode caches. (flutter/flutter#114719) * 96f9ca8 Roll Flutter Engine from 0933ca485a07 to 28e1ea0cc5b1 (4 revisions) (flutter/flutter#114720) * 1aada6f [New Feature]Support mouse wheel event on the scrollbar widget (flutter/flutter#109659) * 6265610 Roll Flutter Engine from 28e1ea0cc5b1 to ba390f2a7b7d (3 revisions) (flutter/flutter#114728) * 140a57e Roll Flutter Engine from ba390f2a7b7d to 223a485ceb02 (3 revisions) (flutter/flutter#114732) * 06ded49 Roll Flutter Engine from 223a485ceb02 to cf56eb5565e2 (3 revisions) (flutter/flutter#114735) * e0e7027 9c45b0ebe Roll Fuchsia Mac SDK from jAKH68TYoKUA5HNS2... to Ua8Jtf8Zka9uxIVdl... (flutter/engine#37345) (flutter/flutter#114746)

* Support keyboard selection in selectable region * fix some comments * addressing comments

* 77d6ecb 805b145d7 [web] Improve line breaker test exceptions (flutter/engine#37244) (flutter/flutter#114688) * 5233fd9 Roll Plugins from 5c11747 to a279b9d (6 revisions) (flutter/flutter#114689) * e1166e4 287a3ab59 Roll Skia from c901cb6ae66f to 1272b520c082 (1 revision) (flutter/engine#37314) (flutter/flutter#114691) * 48457d7 WidgetController.startGesture trackpad support (flutter/flutter#114631) * c1d2b85 Change some required nullable parameters in tool to non-null (flutter/flutter#114115) * cfb2f15 Roll Flutter Engine from 287a3ab59269 to e947833ceb5b (2 revisions) (flutter/flutter#114696) * 80bf355 Support keyboard selection in SelectabledRegion (flutter/flutter#112584) * be83c98 5117e858e Fix typo in overlay platform view iOS test (flutter/engine#37262) (flutter/flutter#114705) * 6ea01e6 Roll Flutter Engine from 5117e858ed1e to 6950689ed775 (3 revisions) (flutter/flutter#114710) * b187bc4 Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (flutter/flutter#114637) * 01507ba Revert "Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (#114637)" (flutter/flutter#114715) * 91a56cf roll packages (flutter/flutter#114366) * d68af26 Roll Flutter Engine from 6950689ed775 to 0933ca485a07 (3 revisions) (flutter/flutter#114714) * 4fd0a92 Remove auto_cleanup of xcode caches. (flutter/flutter#114719) * 96f9ca8 Roll Flutter Engine from 0933ca485a07 to 28e1ea0cc5b1 (4 revisions) (flutter/flutter#114720) * 1aada6f [New Feature]Support mouse wheel event on the scrollbar widget (flutter/flutter#109659) * 6265610 Roll Flutter Engine from 28e1ea0cc5b1 to ba390f2a7b7d (3 revisions) (flutter/flutter#114728) * 140a57e Roll Flutter Engine from ba390f2a7b7d to 223a485ceb02 (3 revisions) (flutter/flutter#114732) * 06ded49 Roll Flutter Engine from 223a485ceb02 to cf56eb5565e2 (3 revisions) (flutter/flutter#114735) * e0e7027 9c45b0ebe Roll Fuchsia Mac SDK from jAKH68TYoKUA5HNS2... to Ua8Jtf8Zka9uxIVdl... (flutter/engine#37345) (flutter/flutter#114746)

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Sep 28, 2022

chunhtai force-pushed the issues/104541 branch from 1a5fc69 to 320c071 Compare October 5, 2022 21:34

chunhtai marked this pull request as ready for review October 5, 2022 21:39

chunhtai force-pushed the issues/104541 branch 3 times, most recently from 5fc78e3 to 041e208 Compare October 11, 2022 18:03

chunhtai requested review from LongCatIsLooong and justinmc October 11, 2022 20:42

flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation f: material design flutter/packages/flutter/material repository. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Oct 11, 2022

LongCatIsLooong reviewed Oct 13, 2022

View reviewed changes

chunhtai force-pushed the issues/104541 branch from f28374e to 35542b4 Compare October 17, 2022 21:52

chunhtai requested a review from LongCatIsLooong October 17, 2022 21:53

LongCatIsLooong reviewed Oct 29, 2022

View reviewed changes

justinmc reviewed Oct 31, 2022

View reviewed changes

chunhtai force-pushed the issues/104541 branch 2 times, most recently from e425b28 to 55d4d18 Compare November 2, 2022 15:56

justinmc approved these changes Nov 2, 2022

View reviewed changes

LongCatIsLooong reviewed Nov 3, 2022

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2022

chunhtai added 3 commits November 3, 2022 12:07

Support keyboard selection in selectable region

6ab8fa6

fix some comments

e76d7c9

addressing comments

2371fd7

chunhtai force-pushed the issues/104541 branch from 8819728 to 2371fd7 Compare November 3, 2022 19:07

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2022

chunhtai merged commit 80bf355 into flutter:master Nov 4, 2022

This was referenced Nov 5, 2022

Roll Flutter from 55e8cd178621 to e0e7027b6263 (20 revisions) flutter/packages#2782

Merged

Roll Flutter from 55e8cd178621 to e0e7027b6263 (20 revisions) flutter/plugins#6673

Merged

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

80bf355 Support keyboard selection in SelectabledRegion (flutter/flut…

728013d

…ter#112584)

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

80bf355 Support keyboard selection in SelectabledRegion (flutter/flut…

1cb21b0

…ter#112584)

shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022

Support keyboard selection in SelectabledRegion (flutter#112584)

ddc20d8

* Support keyboard selection in selectable region * fix some comments * addressing comments

gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023

Support keyboard selection in SelectabledRegion (flutter#112584)

536939d

* Support keyboard selection in selectable region * fix some comments * addressing comments

		@@ -11,6 +11,27 @@ import 'package:flutter_test/flutter_test.dart';

		import 'clipboard_utils.dart';

		Future<void> sendKeyCombination(

Support keyboard selection in SelectabledRegion #112584

Support keyboard selection in SelectabledRegion #112584

Uh oh!

Conversation

chunhtai commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

LongCatIsLooong left a comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

chunhtai commented Oct 17, 2022

Uh oh!

chunhtai commented Oct 20, 2022

Uh oh!

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!

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!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Nov 1, 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!

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

chunhtai commented Sep 28, 2022 •

edited

Loading

chunhtai Nov 1, 2022 •

edited

Loading

chunhtai Nov 1, 2022 •

edited

Loading