-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Selection area right click behavior should match native #128224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Selection area right click behavior should match native #128224
Conversation
fe9a8cc to
25680e3
Compare
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
justinmc
left a comment
There was a problem hiding this 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.
0f1f41f to
756881b
Compare
justinmc
left a comment
There was a problem hiding this 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 👍
0d5d039 to
3e24425
Compare
c2d25df to
026d88a
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
db4ebe7 to
3e43f86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| final bool canCopy = selectionGeometry.hasSelection && selectionGeometry.status == SelectionStatus.uncollapsed; | |
| final bool canCopy = selectionGeometry.status == SelectionStatus.uncollapsed; |
it guarantee to hasSelection if it is uncollapsed
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@chunhtai Would it be helpful to have an assertion in the constructor of The closest we could get with a const constructor is making 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
ba8c770 to
921cb34
Compare
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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

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:
This change also prevents the
copymenu button from being shown when there is a collapsed selection (nothing to copy).Fixes #117561
Pre-launch Checklist
///).