[floating_cursor_selection] a somewhat "design doc" for floating cursor feature #42173

hellohuanlin · 2023-05-19T21:18:36Z

This PR gives an overview of how floating cursor feature works (including the new floating cursor selection gesture, and the existing floating cursor move gesture). It can serve as the missing design doc.

I put it in the engine rather than the framework, since all logics are in one place, and it's closer to the UIKit behavior that I want to cover.

Thanks @moffatman for helping me understanding it.

List which issues are fixed by this PR. You must list at least one issue.

flutter/flutter#30476

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

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

moffatman · 2023-05-24T03:36:03Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// Floating cursor's "selection" gesture also starts with 1 finger to force press the space bar,
+// so exactly the same functions as the "move gesture" discussed above will be called. When the
+// second finger is pressed, `setSelectedText` will be called, which indicates the beginning of
+// the selection gesture. (This mechanism requires `closestPositionFromPoint` to be implemented, so


setSelectedText doesn't necessarily mean the beginning of the selection gesture. It will be called during the selection gesture each time the finger moves enough to change the selection

Oh I meant to say when setSelectedText is first time called, we can tell that user started the gesture. But there are also some nuances, such as the distance threshold of the gesture, as you said here. I think i can remove this part.

moffatman · 2023-05-24T03:37:33Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+//
+// Notice that during the move gesture, the framework only animate the cursor visually. It's only
+// after the gesture is complete, will the framework update the selection to the cursor's
+// new position (with zero selection length). This means during the animation, the selection


I wouldn't say the selection is out of sync between framework and engine. It isn't modified in the framework during the move gesture until the end. So you can say the visual is out of sync with the selection state in both framework and engine.

moffatman · 2023-05-24T03:38:26Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// so exactly the same functions as the "move gesture" discussed above will be called. When the
+// second finger is pressed, `setSelectedText` will be called, which indicates the beginning of
+// the selection gesture. (This mechanism requires `closestPositionFromPoint` to be implemented, so
+// that UIKit can determine the selected text range based on points.) When the selection is


So that UIKit can determine which position in the text is the closest to any arbitrary screen point

How about "so that UIKit can determine which text position is the closest to any arbitrary screen point, hence determine the selected text range based on those points.".

It's definitely used for other features for sure, but I want this to be more specific to this particular feature.

chinmaygarde · 2023-06-01T20:13:13Z

Are we intending to land this? @hellohuanlin, there is some feedback to incorporate.

hellohuanlin · 2023-06-01T21:00:10Z

@chinmaygarde yep. will do.

LongCatIsLooong

LGTM with nits thanks for adding this!

LongCatIsLooong · 2023-06-08T20:08:10Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// Floating cursor's "move" gesture takes 1 finger to force press the space bar, and then move the
+// cursor. The process starts with `beginFloatingCursorAtPoint`. When the finger is moved,
+// `updateFloatingCursorAtPoint` will be called. When the finger is released, `endFloatingCursor`
+// will be called. In all cases, we send the relative point to the framework, so that framework


nit: relative to the initial position registered in beginFloatingCursorAtPoint or some such?

LongCatIsLooong · 2023-06-08T21:44:13Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// cursor. The process starts with `beginFloatingCursorAtPoint`. When the finger is moved,
+// `updateFloatingCursorAtPoint` will be called. When the finger is released, `endFloatingCursor`
+// will be called. In all cases, we send the relative point to the framework, so that framework
+// can animate the floating cursor.


super nit: animate -> correctly position?

it's just visually animate without actually updating the position.

LongCatIsLooong · 2023-06-08T21:54:39Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// will be called. In all cases, we send the relative point to the framework, so that framework
+// can animate the floating cursor.
+//
+// Notice that during the move gesture, the framework only animate the cursor visually. It's only


nit: Remove "Notice"? https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-empty-prose

LongCatIsLooong · 2023-06-08T21:56:07Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+//
+// Notice that during the move gesture, the framework only animate the cursor visually. It's only
+// after the gesture is complete, will the framework update the selection to the cursor's
+// new position (with zero selection length). This means during the animation, the visual effect


after the gesture is complete, will the framework update the selection to the cursor's new position

I think this should probably be considered a bug? IIRC in a UIKit app the selection will actually be updated as you drag?

i'd say it's "by design" - we did it on purpose. not saying whether this design is good or bad.

LongCatIsLooong · 2023-06-08T22:01:30Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// Floating cursor's "selection" gesture also starts with 1 finger to force press the space bar,
+// so exactly the same functions as the "move gesture" discussed above will be called. When the
+// second finger is pressed, `setSelectedText` will be called. This mechanism requires
+// `closestPositionFromPoint` to be implemented, so that UIKit can determine which text position


nit: ... to be implemented to allow UIKit to translate the finger touch location displacement to the text range to select.

LongCatIsLooong · 2023-06-08T22:07:04Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

+// of the cursor is temporarily out of sync with the selection state in both framework and engine.
+// But it will be in sync again after the animation is complete.
+//
+// Floating cursor's "selection" gesture also starts with 1 finger to force press the space bar,


I vaguely remember the "selection" gesture doesn't call {start, update, end}FloatingCursor for the 2nd finger and that was the reason the whole selection thing had to be implemented in the iOS embedder (I think we usually prefer implementing selection handling logic in the framework to avoid synchronization problems), maybe mention that if it isn't documented already?

There is only one start,update,...,end for the whole sequence whether it's one or two fingers. We do continue to get update calls during the second finger movement, but no specific event when the second finger is pressed.

yeah this is something i should include. good that you remember it.

… and selection logic

auto-submit · 2023-06-12T17:42:29Z

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

…ating cursor feature (flutter/engine#42173)

…128726) flutter/engine@de68fba...12def73 2023-06-12 [email protected] [floating_cursor_selection] a somewhat "design doc" for floating cursor feature (flutter/engine#42173) 2023-06-12 [email protected] Roll ANGLE from 43ef50f389e9 to 21f16cb16333 (1 revision) (flutter/engine#42779) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll 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 Flutter: 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

flutter-dashboard bot added the platform-ios label May 19, 2023

hellohuanlin requested review from LongCatIsLooong and moffatman May 19, 2023 21:18

moffatman reviewed May 24, 2023

View reviewed changes

hellohuanlin force-pushed the floating_cursor_selection_design_doc branch 3 times, most recently from 0d5601a to c3dc3e3 Compare June 8, 2023 17:52

hellohuanlin requested a review from moffatman June 8, 2023 21:15

LongCatIsLooong approved these changes Jun 8, 2023

View reviewed changes

hellohuanlin added 3 commits June 12, 2023 10:25

[floating_cursor_selection]add overall logic for floating cursor move…

16f995c

… and selection logic

address some commments

633c64d

address more comments

ce62668

hellohuanlin force-pushed the floating_cursor_selection_design_doc branch from c3dc3e3 to ce62668 Compare June 12, 2023 17:25

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2023

hellohuanlin added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 12, 2023

auto-submit bot merged commit 12def73 into flutter:main Jun 12, 2023

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

Roll Flutter Engine from de68fba09338 to 12def739b1f6 (2 revisions) flutter/flutter#128726

Merged

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

12def739b [floating_cursor_selection] a somewhat "design doc" for flo…

8a6031c

…ating cursor feature (flutter/engine#42173)

[floating_cursor_selection] a somewhat "design doc" for floating cursor feature #42173

[floating_cursor_selection] a somewhat "design doc" for floating cursor feature #42173

Uh oh!

Conversation

hellohuanlin commented May 19, 2023

Pre-launch Checklist

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!

chinmaygarde commented Jun 1, 2023

Uh oh!

hellohuanlin commented Jun 1, 2023

Uh oh!

LongCatIsLooong left a comment

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!

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!

auto-submit bot commented Jun 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants