Supports global selection for all devices by chunhtai · Pull Request #95226 · flutter/flutter

chunhtai · 2021-12-14T00:54:55Z

Support for mouse drag selection and mobile selection

#81839

design doc: http://flutter.dev/go/global-selection

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.

goderbauer

Haven't reviewed it all, just some comments from an initial read-through.

goderbauer · 2021-12-14T23:14:49Z

+
+  /// Called when the selectables this widget collected has changed.
+  ///
+  /// Use this method to extract the collected selectables in the subtree.


document that the selectables are given in screen order in the list?

goderbauer · 2021-12-14T23:15:17Z

+
+  DragSelectionStartEvent? _currentDragStart;
+
+  /// Updates the selectables this updater manages.


Document that it expects the selectables in screen order (I think?)

goderbauer · 2021-12-14T23:16:35Z

+  /// Gets the list selectables this updater is responsible for updating.
+  ///
+  /// The subclasses is responsible for updating the selection when the
+  /// selectables in the list has changed.


document expected order?

goderbauer · 2021-12-14T23:17:44Z

+  }
+
+  /// Copies the selected contents of all the selectables.
+  Object? copy() {


Should this be typed String?? Seems to be the only possible return type?

It may not be the case if we support multimedia, but i guess we can change that later?

goderbauer · 2021-12-14T23:19:13Z

+
+  /// Initialize the selection of the selectable children.
+  ///
+  /// The goal is to find which selectable child the selection starts on


missing .

goderbauer · 2021-12-14T23:29:36Z

This looks pretty interesting. It's nice to see that we can do this without adding things to RenderObject.

What wasn't clear to me from my initial read-through is why we need SelectionResult.next/SelectionResult.previous. Why can that not just be determined based on the rect of a Selectable?

goderbauer · 2021-12-14T23:44:04Z

+    late final SelectionResult result;
+    if (event is DragSelectionEvent) {
+      if (event is DragSelectionStartEvent) {
+        _dragSelectionStartInGlobalCoordinate = event.offset;


Wouldn't we want to hold on to the start point in local coordinates und use that for the calculations in _updateDragSelection?

I am thinking about a scenario where you start the selection with a pointer down in a large body of text and while the pointer is down you scroll a surrounding scrollable. I think that scroll needs to extend the selection and the selection needs to start at the letter that the pointer touched when it cam down first. Due to the scroll, that letter will have moved in the global coordinate system, though.

goderbauer · 2021-12-14T23:56:36Z

+  /// should start before all of its children. Returns [SelectionResult.next]
+  /// if the selection should start after all of its children.
+  SelectionResult _initSelection(DragSelectionUpdateEvent event) {
+    for (int index = 0; index < selectables.length; index += 1){


nit: space before {

chunhtai · 2021-12-14T23:57:15Z

This looks pretty interesting. It's nice to see that we can do this without adding things to RenderObject.

What wasn't clear to me from my initial read-through is why we need SelectionResult.next/SelectionResult.previous. Why can that not just be determined based on the rect of a Selectable?

The biggest concern is select to scroll. Imagine a container with 3 child [A, B, C]. If B is scrollable and the drag selection goes to C, IMO we can't send update event to C until B finishes scrolling. We can also makes B returns some kind of flag to tell the container not to proceed and get rid of SelectionResult, but I think the current implementation is more systematic since there maybe more cases where selectable itself has opinion on the selection order.

The other concern is keyboard selection. Imagine developer press shit+ down, how does the container know which selectable should be the new selection edge. With SelectionResult, the selectable itself can tell the container whether the shift down should reach the next selectable or not.

skia-gold · 2021-12-30T21:07:46Z

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

skia-gold · 2022-01-04T21:07:24Z

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

skia-gold · 2022-01-06T17:57:24Z

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

skia-gold · 2022-01-07T20:22:24Z

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

chunhtai · 2022-01-27T01:16:12Z

please ignore the failing test for now, It is because I want to hide the selection specific logic in RenderParagraph by making a private subclass of it, and it failed the describe node test. I am thinking whether there is way around it or i will have to expose the API

chunhtai · 2022-01-27T01:21:27Z

I will add more tests. This PR now supports both mouse selection and mobile selection. I figure i may need to tear up bunch of API in Selectable to separate both them into two PR, which may make it hard to comprehend. So I keep them in the same PR.

chunhtai · 2022-01-27T01:22:48Z

also cc @jonahwilliams

goderbauer

Overall this LGTM after the last few comments are addressed.

goderbauer · 2022-05-24T16:55:26Z

What if it changes from non-null to null?

It will be handled in didUpdateWidget

goderbauer · 2022-05-24T17:05:02Z

Let's add some TODO comments here explaining that.

goderbauer

LGTM

)

nilsreichardt · 2022-05-24T22:48:30Z

Right clicking a word selects the word, but I think it shouldn't. In a TextField, right clicking just sets the cursor.

From #95226 (review)

@justinmc On macOS this not true for web (I don't know how Windows it handels):

Screen.Recording.2022-05-25.at.00.46.29.mov

(I used the flutter.dev website)

justinmc · 2022-05-24T22:52:25Z

@nilsreichardt Good catch. I think we're doing those gestures in another PR so we'll have to get this right there.

nilsreichardt · 2022-05-24T23:07:28Z

@justinmc Yes, I also opened a few issue for the SelectionArea widget which can be implemented in separated PRs 👍 My intention with the comment was the current selection behavior of SelectionArea is correct (right click and the word becomes selected) ✅

* Support global selection * addressing comments * add new test * Addressing review comments * update * addressing comments * addressing comments * Addressing comments * fix build

)

goderbauer reviewed Dec 14, 2021

View reviewed changes

guidezpl mentioned this pull request Dec 15, 2021

New flutter gallery has higher average frame build times #92919

Closed

chunhtai force-pushed the inherited-selection branch from 1387fce to bfab82c Compare December 30, 2021 23:23

chunhtai force-pushed the inherited-selection branch 6 times, most recently from 523b432 to 2cf4f8e Compare January 18, 2022 21:44

chunhtai force-pushed the inherited-selection branch 2 times, most recently from 9425c57 to 01d68b6 Compare January 26, 2022 01:03

chunhtai marked this pull request as ready for review January 27, 2022 01:14

flutter-dashboard Bot added the p: material_ui material_ui package in flutter/packages label Jan 27, 2022

chunhtai requested a review from goderbauer January 27, 2022 01:14

goderbauer reviewed May 24, 2022

View reviewed changes

goderbauer approved these changes May 24, 2022

View reviewed changes

chunhtai added 9 commits May 24, 2022 13:09

Support global selection

d9025d6

addressing comments

f1dfdf4

add new test

e7e3bb8

Addressing review comments

48500dc

update

bdc95db

addressing comments

bae0fe3

addressing comments

140e21d

Addressing comments

9527c10

fix build

577d624

chunhtai force-pushed the inherited-selection branch from 42e052a to 577d624 Compare May 24, 2022 20:09

chunhtai added the waiting for tree to go green label May 24, 2022

chunhtai merged commit efb9368 into flutter:master May 24, 2022

engine-flutter-autoroll mentioned this pull request May 24, 2022

Roll Flutter from ac29c11aecb8 to efb9368573f6 (3 revisions) flutter-team-archive/plugins#5827

Merged

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

efb9368 Supports global selection for all devices (flutter/flutter#95226

d20e7ec

)

engine-flutter-autoroll mentioned this pull request May 24, 2022

Roll Flutter from c29a7a2d22e6 to 7b6ec2c82a4b (4 revisions) flutter/packages#2068

Merged

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

efb9368 Supports global selection for all devices (flutter/flutter#95226

a8ea4a7

)

This was referenced May 24, 2022

SelectionArea does not copy the text sometimes #104549

Closed

SelectionArea does not select the text when using double and triple tap #104552

Closed

SelectionArea should support selecting text with arrow keys #104556

Closed

daohoangson mentioned this pull request May 25, 2022

Select all only select one paragraph. daohoangson/flutter_widget_from_html#727

Closed

andrigel mentioned this pull request Jun 23, 2022

Add ability to select text in message team113/messenger#17

Closed

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

efb9368 Supports global selection for all devices (flutter/flutter#95226

5d78275

)

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

efb9368 Supports global selection for all devices (flutter/flutter#95226

8d9ed1c

)

nilsreichardt mentioned this pull request Feb 3, 2023

Documentation on SelectableText should mention SelectionArea #119911

Closed


		DragSelectionStartEvent? _currentDragStart;

		/// Updates the selectables this updater manages.

Conversation

chunhtai commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

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!

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!

goderbauer commented Dec 14, 2021

Uh oh!

goderbauer Dec 14, 2021 • 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!

chunhtai commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skia-gold commented Dec 30, 2021

Uh oh!

skia-gold commented Jan 4, 2022

Uh oh!

skia-gold commented Jan 6, 2022

Uh oh!

skia-gold commented Jan 7, 2022

Uh oh!

chunhtai commented Jan 27, 2022

Uh oh!

chunhtai commented Jan 27, 2022

Uh oh!

chunhtai commented Jan 27, 2022

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai May 24, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

nilsreichardt commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented May 24, 2022

Uh oh!

nilsreichardt commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

chunhtai commented Dec 14, 2021 •

edited

Loading

goderbauer Dec 14, 2021 •

edited

Loading

chunhtai commented Dec 14, 2021 •

edited

Loading

chunhtai May 24, 2022 •

edited

Loading

nilsreichardt commented May 24, 2022 •

edited

Loading