Skip to content

Conversation

@Renzo-Olivares
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares commented Jun 5, 2023

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.

@flutter-dashboard 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 Renzo-Olivares force-pushed the selection-area-right-click branch from fe9a8cc to 25680e3 Compare June 5, 2023 19:08
@Renzo-Olivares Renzo-Olivares marked this pull request as ready for review June 5, 2023 19:10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

also this can be lastSecondaryTapDownPosition to be more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, should this collapse the selection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renewing my LGTM with the changes 👍

@Renzo-Olivares 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 github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jun 13, 2023
@Renzo-Olivares Renzo-Olivares force-pushed the selection-area-right-click branch from c2d25df to 026d88a Compare June 13, 2023 20:15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2023-06-15 at 11 28 44 AM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is possible given the current implementation.

Screenshot 2023-06-15 at 12 02 51 PM

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.

Screenshot 2023-06-15 at 12 04 59 PM

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what coordinates are these rects in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Renzo-Olivares Renzo-Olivares Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right

@Renzo-Olivares Renzo-Olivares force-pushed the selection-area-right-click branch from db4ebe7 to 3e43f86 Compare June 15, 2023 18:30
@Renzo-Olivares Renzo-Olivares requested a review from chunhtai June 15, 2023 20:59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Renzo-Olivares
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
@Renzo-Olivares Renzo-Olivares force-pushed the selection-area-right-click branch from ba8c770 to 921cb34 Compare June 17, 2023 01:18
@Renzo-Olivares Renzo-Olivares requested a review from chunhtai June 20, 2023 17:37
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Renzo-Olivares Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 21, 2023
@auto-submit auto-submit bot merged commit b36ef58 into flutter:master Jun 21, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 22, 2023
flutter/flutter@c40baf4...042c036

2023-06-22 [email protected] Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066)
2023-06-22 [email protected] Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974)
2023-06-22 [email protected] Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566)
2023-06-22 [email protected] Add `InputDecorationTheme.merge` (flutter/flutter#129011)
2023-06-22 [email protected] Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351)
2023-06-22 [email protected] Prevent crashes on range errors when selecting device (flutter/flutter#129290)
2023-06-22 [email protected] Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353)
2023-06-22 [email protected] Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339)
2023-06-22 [email protected] Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337)
2023-06-22 [email protected] Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334)
2023-06-22 [email protected] Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331)
2023-06-22 [email protected] Manual roll of packages to 9af50d4 (flutter/flutter#129328)
2023-06-21 [email protected] Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306)
2023-06-21 [email protected] [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366)
2023-06-21 [email protected] Roll pub packages (flutter/flutter#128966)
2023-06-21 [email protected] Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298)
2023-06-21 [email protected] Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464)
2023-06-21 [email protected] [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222)
2023-06-21 [email protected] Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293)
2023-06-21 [email protected] 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 [email protected],[email protected],[email protected] 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[web] Right click in SelectionArea incorrectly selects text (on Windows)

3 participants