Selection area right click behavior should match native #128224

Renzo-Olivares · 2023-06-05T08:41:28Z

This change updates SelectableRegions right-click gesture to match native platform behavior.

Before: Right-click gesture selects word at position and opens context menu (All Platforms)
After:

Linux, toggles context menu on/off, and collapses selection when click was not on an active selection (uncollapsed).
Windows, Android, Fuchsia, shows context menu at right-clicked position (unless the click is at an active selection).
macOS, toggles the context menu if right click was at the same position as the previous / or selects word at position and opens context menu.
iOS, selects word at position and opens context menu.

This change also prevents the copy menu button from being shown when there is a collapsed selection (nothing to copy).

Fixes #117561

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.

chunhtai · 2023-06-05T21:51:25Z

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

@@ -263,7 +263,7 @@ class SelectableRegion extends StatefulWidget {
    required final VoidCallback onCopy,
    required final VoidCallback onSelectAll,
  }) {
-    final bool canCopy = selectionGeometry.hasSelection;
+    final bool canCopy = selectionGeometry.hasSelection && selectionGeometry.startSelectionPoint?.localPosition != selectionGeometry.endSelectionPoint?.localPosition;


should this check for selectionGeometry.status == SelectionStatus.uncollapsed

chunhtai · 2023-06-05T21:55:47Z

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

+      case TargetPlatform.android:
+      case TargetPlatform.fuchsia:
+      case TargetPlatform.windows:
+        _selectStartTo(offset: details.globalPosition);


they don't select word? does the intention to collapse the selection at this globalPosition?

also this can be lastSecondaryTapDownPosition to be more readable

Yeah they do not, tested on chrome and wordpad/notepad. But good point about collapsing the selection, they only collapse the selection when right click happens outside of the currently active selection.

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

chunhtai · 2023-06-05T21:57:56Z

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

+          hideToolbar();
+          return;
+        }
+        _selectStartTo(offset: details.globalPosition);


same here, should this collapse the selection?

Good point, only when the right-click happens outside of the current selection.

justinmc

LGTM 👍 Just one thought below about possible duplicate logic.

Thanks for trying all the niche native behaviors to get them right. I think there are a lot of physical mouse behaviors on Android and iOS that we don't do correctly right now.

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

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

justinmc

Renewing my LGTM with the changes 👍

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

chunhtai · 2023-06-15T16:34:48Z

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

+    );
+    final List<Rect> selectionRects = <Rect>[];
+    for (final TextBox textBox in paragraph.getBoxesForSelection(selection)) {
+      selectionRects.add(textBox.toRect());


If you click in between selected lines, should it consider inside the selection?

Do you mean if I click in the overlap between the selections?

I mean if it is possible the line height is large that each line does not overlap. I am not sure if that is possible though.

I think it is possible given the current implementation.

This is due to getBoxesForSelection defaulting to SelectionHeightStyle.tight.

List<ui.TextBox> getBoxesForSelection( TextSelection selection, { ui.BoxHeightStyle boxHeightStyle = ui.BoxHeightStyle.tight, ui.BoxWidthStyle boxWidthStyle = ui.BoxWidthStyle.tight, }) { assert(!debugNeedsLayout); _layoutTextWithConstraints(constraints); return _textPainter.getBoxesForSelection( selection, boxHeightStyle: boxHeightStyle, boxWidthStyle: boxWidthStyle, ); }

In SelectableText and TextField these values are customizable. Setting to SelectionHeightStyle.max would result in this instead.

What I see on WordPad for windows is that it looks like they use SelectionHeightStyle.max.

Though I think adding this type of customizability can be done is a separate PR. I think if the line do not overlap it might be reasonable to expect that the gap is not within the selection?

I think if the line do not overlap it might be reasonable to expect that the gap is not within the selection?

I don't have an answer for this, since I am not sure if there is a native app that does SelectionHeightStyle.tight?

I am ok as-is, just want to make sure we are aware of this.

chunhtai · 2023-06-15T16:36:20Z

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

@@ -627,6 +628,9 @@ class SelectionGeometry {
  /// The status of ongoing selection in the [Selectable] or [SelectionHandler].
  final SelectionStatus status;

+  /// The rects that represent the selection if there is any.
+  final List<Rect>? selectionRects;


what coordinates are these rects in?

These should be in the local coordinates of the Selectable. I'll update the docs.

you can give it a default value of const <Rect>[] and make it not nullable.

chunhtai · 2023-06-15T16:37:12Z

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

+    for (int index = currentSelectionStartIndex; index <= currentSelectionEndIndex; index++) {
+      final List<Rect>? currSelectableSelectionRects = selectables[index].value.selectionRects;
+      if (currSelectableSelectionRects != null) {
+        selectionRects.addAll(currSelectableSelectionRects);


are they in global coordinates? otherwise, don we need to do transformation?

They are in the local coordinates of the Selectable. I think this fits with the rest of the SelectionGeometry since the SelectionPoints are also in the local coordinates.

Oh are you saying if they are local coordinates then they must be transformed to the coordinates of the SelectableRegionContainerDelegate/global?

chunhtai · 2023-06-16T18:05:28Z

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

@@ -263,7 +263,7 @@ class SelectableRegion extends StatefulWidget {
    required final VoidCallback onCopy,
    required final VoidCallback onSelectAll,
  }) {
-    final bool canCopy = selectionGeometry.hasSelection;
+    final bool canCopy = selectionGeometry.hasSelection && selectionGeometry.status == SelectionStatus.uncollapsed;


Suggested change

final bool canCopy = selectionGeometry.hasSelection && selectionGeometry.status == SelectionStatus.uncollapsed;

final bool canCopy = selectionGeometry.status == SelectionStatus.uncollapsed;

it guarantee to hasSelection if it is uncollapsed

chunhtai · 2023-06-16T18:10:55Z

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

+        // If lastSecondaryTapDownPosition is within the current selection then
+        // keep the current selection, if not then collapse it.
+        final bool uncollapsedSelectionExists = _selectionDelegate.value.hasSelection && _selectionDelegate.value.status == SelectionStatus.uncollapsed;
+        final bool lastSecondaryTapDownPositionWasOnActiveSelection = uncollapsedSelectionExists && _positionIsOnActiveSelection(globalPosition: details.globalPosition);


You can just check _positionIsOnActiveSelection directly if the selectionRects is not nullable?

chunhtai · 2023-06-16T18:16:15Z

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

+      final List<Rect>? currSelectableSelectionRects = selectables[index].value.selectionRects;
+      if (currSelectableSelectionRects != null) {
+        selectionRects.addAll(currSelectableSelectionRects.map((Rect selectionRect) {
+          final Matrix4 transform = selectables[index].getTransformTo(null);


This should not be stored in global coordinates, but in local coordinates of SelectionContainer.

Either everything is stored in global coordinates starting from the selectionGeomery of _SelectableFragment, or everything is stored in local coordinates of the Selectable including the SelectionContainer.

Since other properties in SelectionGeometry are in local coordinate, this should too. Use getTransformFrom(selectables[index]) instead.

chunhtai · 2023-06-16T18:18:34Z

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

@@ -627,6 +628,9 @@ class SelectionGeometry {
  /// The status of ongoing selection in the [Selectable] or [SelectionHandler].
  final SelectionStatus status;

+  /// The rects that represent the selection if there is any.
+  final List<Rect>? selectionRects;


you can give it a default value of const <Rect>[] and make it not nullable.

Renzo-Olivares · 2023-06-16T21:28:19Z

@chunhtai Would it be helpful to have an assertion in the constructor of SelectionGeometry that asserts if SelectionStatus.uncollapsed then selectionRects should be non-empty? Though this wouldn't be possible with a const constructor.

The closest we could get with a const constructor is making selectionRects nullable, and checking if it is non-null when SelectionStatus.uncollapsed.

What do you think?

chunhtai · 2023-06-16T21:42:39Z

@chunhtai Would it be helpful to have an assertion in the constructor of SelectionGeometry that asserts if SelectionStatus.uncollapsed then selectionRects should be non-empty? Though this wouldn't be possible with a const constructor.

The closest we could get with a const constructor is making selectionRects nullable, and checking if it is non-null when SelectionStatus.uncollapsed.

What do you think?

I think you can have SelectionStatus.uncollapsed, but selectionRects empty if you scroll the selection off screen in a scrollable

chunhtai · 2023-06-16T21:43:21Z

@chunhtai Would it be helpful to have an assertion in the constructor of SelectionGeometry that asserts if SelectionStatus.uncollapsed then selectionRects should be non-empty? Though this wouldn't be possible with a const constructor.
The closest we could get with a const constructor is making selectionRects nullable, and checking if it is non-null when SelectionStatus.uncollapsed.
What do you think?

I think you can have SelectionStatus.uncollapsed, but selectionRects empty if you scroll the selection off screen in a scrollable

that remind me we should probably clip the selection rects in scrollable.

…tions this includes exposing toolbarIsVisible to SelectionOverlay which was already exposed to TextSelectionOverlay and also copy should not be shown on a collapsed selection

chunhtai

LGTM

…utter/flutter#128224)

flutter/flutter@c40baf4...042c036 2023-06-22 ahmedelsaayid@gmail.com Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 tessertaha@gmail.com Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 utisam@gmail.com Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 tessertaha@gmail.com Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 engine-flutter-autoroll@skia.org Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 42216813+eliasyishak@users.noreply.github.com Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 zanderso@users.noreply.github.com Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 jason-simmons@users.noreply.github.com Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 yjbanov@google.com [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 fluttergithubbot@gmail.com Roll pub packages (flutter/flutter#128966) 2023-06-21 6655696+guidezpl@users.noreply.github.com Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 jmccandless@google.com Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 christopherfujino@gmail.com [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 rmolivares@renzo-olivares.dev Selection area right click behavior should match native (flutter/flutter#128224) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC dit@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…utter/flutter#128224)

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 5, 2023

Renzo-Olivares force-pushed the selection-area-right-click branch from fe9a8cc to 25680e3 Compare June 5, 2023 19:08

Renzo-Olivares marked this pull request as ready for review June 5, 2023 19:10

Renzo-Olivares requested review from justinmc, chunhtai and hangyujin June 5, 2023 19:21

chunhtai reviewed Jun 5, 2023

View reviewed changes

justinmc approved these changes Jun 8, 2023

View reviewed changes

packages/flutter/lib/src/widgets/selectable_region.dart Show resolved Hide resolved

packages/flutter/lib/src/widgets/text_selection.dart Show resolved Hide resolved

Renzo-Olivares force-pushed the selection-area-right-click branch from 0f1f41f to 756881b Compare June 9, 2023 19:39

justinmc approved these changes Jun 12, 2023

View reviewed changes

packages/flutter/lib/src/widgets/selectable_region.dart Show resolved Hide resolved

Renzo-Olivares force-pushed the selection-area-right-click branch 2 times, most recently from 0d5d039 to 3e24425 Compare June 13, 2023 16:57

github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Jun 13, 2023

Renzo-Olivares force-pushed the selection-area-right-click branch from c2d25df to 026d88a Compare June 13, 2023 20:15

Renzo-Olivares requested review from chunhtai and justinmc June 13, 2023 21:29

chunhtai reviewed Jun 15, 2023

View reviewed changes

Renzo-Olivares force-pushed the selection-area-right-click branch from db4ebe7 to 3e43f86 Compare June 15, 2023 18:30

Renzo-Olivares requested a review from chunhtai June 15, 2023 20:59

chunhtai reviewed Jun 16, 2023

View reviewed changes

Renzo-Olivares added 3 commits June 16, 2023 18:18

Adjust selectable region right click behavior based on native observa…

3d69b30

…tions this includes exposing toolbarIsVisible to SelectionOverlay which was already exposed to TextSelectionOverlay and also copy should not be shown on a collapsed selection

Add some tests, still needs some fixing

f4d1258

Fix tests

4f13e91

Renzo-Olivares added 11 commits June 16, 2023 18:18

Fix issue where wrong transform is used

20e83ad

extract positionIsOnSelection

152d607

update

894ca62

simplify

1fd29a3

update docs

2e0e5e3

Address comments

e3d3332

Address reviewer comments

1b87c18

if the selection is uncollapsed, then selection rects should be provided

87a6376

revert

4ea2ada

clip selectionRects to drawableArea

9ddbdf7

update naming

921cb34

Renzo-Olivares force-pushed the selection-area-right-click branch from ba8c770 to 921cb34 Compare June 17, 2023 01:18

Renzo-Olivares requested a review from chunhtai June 20, 2023 17:37

chunhtai approved these changes Jun 21, 2023

View reviewed changes

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 21, 2023

auto-submit bot merged commit b36ef58 into flutter:master Jun 21, 2023
72 checks passed

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to a2739c09a722 (13 revisions) flutter/packages#4275

Closed

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

b36ef583f Selection area right click behavior should match native (fl…

3789e37

…utter/flutter#128224)

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 2e05371c88da (17 revisions) flutter/packages#4276

Closed

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

b36ef583f Selection area right click behavior should match native (fl…

942c43c

…utter/flutter#128224)

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 0da8012cc77d (18 revisions) flutter/packages#4277

Closed

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

b36ef583f Selection area right click behavior should match native (fl…

473a899

…utter/flutter#128224)

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 042c0366c9a9 (20 revisions) flutter/packages#4278

Merged

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

b36ef583f Selection area right click behavior should match native (fl…

faa1fd1

…utter/flutter#128224)

HansMuller mentioned this pull request Jun 30, 2023

Flutter Framework Team: Notable Commits #121415

Open

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

b36ef583f Selection area right click behavior should match native (fl…

9f375cc

…utter/flutter#128224)

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

b36ef583f Selection area right click behavior should match native (fl…

54b4902

…utter/flutter#128224)

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

b36ef583f Selection area right click behavior should match native (fl…

73a027b

…utter/flutter#128224)

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

b36ef583f Selection area right click behavior should match native (fl…

61fd729

…utter/flutter#128224)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selection area right click behavior should match native #128224

Selection area right click behavior should match native #128224

Renzo-Olivares commented Jun 5, 2023 •

edited

chunhtai Jun 5, 2023

chunhtai Jun 5, 2023

Renzo-Olivares Jun 5, 2023

chunhtai Jun 5, 2023

Renzo-Olivares Jun 5, 2023

justinmc left a comment

justinmc left a comment

chunhtai Jun 15, 2023

Renzo-Olivares Jun 15, 2023

chunhtai Jun 15, 2023

Renzo-Olivares Jun 15, 2023 •

edited

chunhtai Jun 16, 2023

chunhtai Jun 15, 2023

Renzo-Olivares Jun 15, 2023

chunhtai Jun 16, 2023

chunhtai Jun 15, 2023

Renzo-Olivares Jun 15, 2023

Renzo-Olivares Jun 15, 2023 •

edited

chunhtai Jun 15, 2023

chunhtai Jun 16, 2023

chunhtai Jun 16, 2023

chunhtai Jun 16, 2023

chunhtai Jun 16, 2023

Renzo-Olivares commented Jun 16, 2023

chunhtai commented Jun 16, 2023

chunhtai commented Jun 16, 2023

chunhtai left a comment

	final bool canCopy = selectionGeometry.hasSelection && selectionGeometry.status == SelectionStatus.uncollapsed;
	final bool canCopy = selectionGeometry.status == SelectionStatus.uncollapsed;

Selection area right click behavior should match native #128224

Selection area right click behavior should match native #128224

Conversation

Renzo-Olivares commented Jun 5, 2023 • edited

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Jun 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Jun 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares commented Jun 16, 2023

chunhtai commented Jun 16, 2023

chunhtai commented Jun 16, 2023

chunhtai left a comment

Choose a reason for hiding this comment

Renzo-Olivares commented Jun 5, 2023 •

edited

Renzo-Olivares Jun 15, 2023 •

edited

Renzo-Olivares Jun 15, 2023 •

edited