iPhone floating cursor selection #36643

moffatman · 2022-10-06T17:35:02Z

Floating cursor selection hasn't worked as we haven't been returning a real value for caretRectForPosition:. If we have the selection rectangles, we can do so.

Fixes #30476

Requires selection rects to be turned on for iPhone (flutter/flutter#113048)

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.

chinmaygarde · 2022-10-06T20:27:37Z

cc @cbracken @jmagman

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

jmagman · 2022-10-19T20:21:24Z

@LongCatIsLooong might know the most about this #26486

LongCatIsLooong · 2022-11-01T22:36:35Z

Is this implementing this feature?

A second force press leaves a selection anchor and then selects full words between the anchor and the current floating cursor position.

I think we should do that in the framework if possible. Is there a way that allows us to detect the second force press? Sending the bounding boxes of visible glyphs is going to be very expensive.

moffatman · 2022-11-01T22:56:23Z

I haven't found any way to detect the second press, the keyboard just changes the selection based on the position of the floating cursor.

jmagman

LGTM

jmagman · 2022-11-09T21:17:23Z

@LongCatIsLooong do you have any more review requests or can this land?

moffatman · 2022-11-09T21:36:31Z

@LongCatIsLooong We are already sending the rects since flutter/flutter#113048. I don't know how to look up benchmark results, maybe you can check the impact?

LongCatIsLooong · 2022-11-17T21:35:31Z

Sorry for the delayed response. I'm not aware of any existing benchmarks that cover this, the framework only sends new rects when the underlying text layout changes, or when the text field stops scrolling (and if the text is too long when the floating cursor moves close enough to an edge the text field may scroll, that could break the interaction here too), and I don't think the performance of entering text is currently measured/monitored (~~it's probably worth doing, I'll create an issue for that~~ on a second thought, may not be a good idea since we have to interact with the OS's input system flutter/flutter#115595).

Is the 2nd force press event detectable via {begin/update/end}floatingCursor? If so I think we can replicate UIKit's behavior in the framework, like we do for regular gestures?

moffatman · 2022-11-17T23:22:58Z

We can't find out about the second force press, the system just sends selection updates if used once we have the correct rectangles.

LongCatIsLooong · 2022-11-23T08:04:14Z

Ah I see it's using _UIKeyboardTextSelectionInteraction. What happens when you use the spacebar selection gesture to extend the selection outside of the text field's bounds? I think the framework currently stops sending rects if the text field starts to scroll.

moffatman · 2022-11-24T05:36:25Z

Right now only the rects of the visible characters are sent so you can't scroll outside of the current viewport. I just tried always sending all the rects, it works surprisingly well, as since the rects aren't updated during scroll, they remain stable as UIKit changes the selection, which causes the framework to scroll. But it sometimes seems kind of glitchy, there may be some update leaking through from somewhere.

In general you wouldn't need to send all the rects to get scrolling to work, since the keyboard is only a fixed width. Probably it would be optimal to send the current visible characters as well as the same width of the view off of each edge. No idea about vertical scrolling though...

chinmaygarde · 2022-12-01T21:03:35Z

Are we making progress on this? Can we land this perhaps and file a followup?

LongCatIsLooong

The implementation LGTM.

LongCatIsLooong · 2022-12-02T23:50:49Z

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

+                      characterAfterCaret.size.height);
+  }
+  CGRect characterBeforeCaret = rects[0].rect;
+  // Return a zero-width rectangle along the right edge of the character before the caret position.


Maybe use the center instead? I think that would probably work better for RTL and Bidi text since we don't send the bidi info along with the selection rects.

I don't think centre would work correctly, it should stay where it is for LTR. But for RTL you're right it likely won't work properly. Maybe a better approach would be putting the rectangle midway between the closest two edges of the adjacent selectionRects to the cursor. Then we don't care about the paint direction v.s. semantic direction.

it should stay where it is for LTR

Sorry I'm not sure how UIKit is going to use this for floating cursor, could you explain why that wouldn't work?

two edges of the adjacent selectionRects to the cursor.

If the position happens to be at a line break or bidi text boundary then the logically adjacent selection rects won't necessarily be visually adjacent right?

jmagman · 2022-12-07T21:19:51Z

Almost ready to land, waiting on answer to last questions from @LongCatIsLooong

moffatman · 2022-12-08T04:55:31Z

Upon revisiting this to check out linebreak / RTL text I think we will need some other framework changes first. Mainly to avoid existing floating cursor implementation from conflicting. Also working on engine improvements to handle various edge cases in caret rect positioning.

jmagman · 2022-12-13T19:28:22Z

@moffatman let us know when this is ready to re-review.

moffatman · 2022-12-13T19:35:24Z

@jmagman It's ready for re-review

jmagman · 2022-12-13T23:36:42Z

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

-
- (CGRect)bounds {
-  return _isFloatingCursorActive ? kSpacePanBounds : super.bounds;
+  NSInteger index = ((FlutterTextPosition*)position).index;


nit I think this is a NSUInteger

It have it as signed to do MAX(0, index - 1) later

jmagman · 2022-12-13T23:38:35Z

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

+    // There is no character before the caret, so this will be the bounds of the character after the
+    // caret position.
+    CGRect characterAfterCaret = rects[0].rect;
+    // Return a zero-width rectangle 30% in from the left edge of the character after the caret


Does this work RTL?

I thought it did but I tested the wrong way, looking into changes now to handle proper RTL unicode chars.

I think there is a bug in Skia getGlyphPositionAtCoordinate in RTL, looking at that now

moffatman · 2022-12-16T01:01:53Z

Here is the Skia change: https://skia-review.googlesource.com/c/skia/+/619838

Will also need another framework PR to send the text directionality of each selection rect

It fixes issues with characters of varying heights Don't reduce the precision for pencil usage

Instead of the hack using the _selectionAffinity for the user's current cursor position, which may not be the same as the one passed to caretRectForPosition. Also optimize closestPositionToPoint, we don't need a second loop. The only possible closer position is the far edge of the current closest position.

hellohuanlin

LGTM. Thanks for making all these changes.

hellohuanlin · 2023-05-10T18:11:58Z

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

-  BOOL isFartherToRight =
-      selectionRect.origin.x + (checkRightBoundary ? selectionRect.size.width : 0) >
-      otherSelectionRect.origin.x;
+  BOOL isFarther;


nit: maybe isCloserToTrailing?

It's not about "closer" to anything, just further in the direction of text layout.

hellohuanlin · 2023-05-10T18:18:35Z

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

-    if (_selectionRects[i].position >= start && _selectionRects[i].position <= end) {
+    if (_selectionRects[i].position >= start &&
+        (_selectionRects[i].position < end ||
+         (start == end && _selectionRects[i].position <= end))) {


what is this new condition for?

In the old behaviour, you would always get one more extra rect (since it was comparing <= end). Now, we are correctly using < end. But we still want to return the special rect (zero-width) if start == end instead of no rects. So we can use the old comparison <= end but only if we have start == end.

Text Range Old rects returned New rects returned

0..0 [0] with width zero [0] with width zero

0..1 [0], [1] [0]

0..2 [0], [1], [2] [0], [1]

And so on...

Gotcha. Looks like the old behavior is a bug then.

hellohuanlin

LGTM. Thanks for making all these changes.

…126622) flutter/engine@f38f46f...748ef96 2023-05-11 [email protected] Roll Skia from aed81125e6bc to ccec5093ca38 (10 revisions) (flutter/engine#41963) 2023-05-11 [email protected] Roll Fuchsia Mac SDK from 2tQjI0g3aDmjHAtMw... to MjcKzcsqMDkuRKopu... (flutter/engine#41960) 2023-05-11 [email protected] iPhone floating cursor selection (flutter/engine#36643) 2023-05-11 [email protected] Update README.md (flutter/engine#41953) 2023-05-11 [email protected] Roll Clang from 5344d8e10bb7 to 6d667d4b261e (flutter/engine#41949) 2023-05-11 [email protected] Use moved SkImage procs (flutter/engine#41947) 2023-05-11 [email protected] allow supplying custom gn args in gn wrapper (flutter/engine#41794) 2023-05-11 [email protected] fix: platform_dispatcher documentation typo (flutter/engine#41739) 2023-05-11 [email protected] Remove GN staging flag for save layer bounds (flutter/engine#41940) 2023-05-11 [email protected] switch MockTexture off of MockCanvas calls (flutter/engine#41906) 2023-05-11 [email protected] [Android] Lifecycle defaults to focused instead of unfocused (flutter/engine#41875) 2023-05-11 [email protected] Roll Skia from ccf73af6ca91 to aed81125e6bc (9 revisions) (flutter/engine#41946) 2023-05-11 [email protected] Clobber caches in licenses test. (flutter/engine#41942) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 2tQjI0g3aDmj to MjcKzcsqMDku 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

…lutter#126622) flutter/engine@f38f46f...748ef96 2023-05-11 [email protected] Roll Skia from aed81125e6bc to ccec5093ca38 (10 revisions) (flutter/engine#41963) 2023-05-11 [email protected] Roll Fuchsia Mac SDK from 2tQjI0g3aDmjHAtMw... to MjcKzcsqMDkuRKopu... (flutter/engine#41960) 2023-05-11 [email protected] iPhone floating cursor selection (flutter/engine#36643) 2023-05-11 [email protected] Update README.md (flutter/engine#41953) 2023-05-11 [email protected] Roll Clang from 5344d8e10bb7 to 6d667d4b261e (flutter/engine#41949) 2023-05-11 [email protected] Use moved SkImage procs (flutter/engine#41947) 2023-05-11 [email protected] allow supplying custom gn args in gn wrapper (flutter/engine#41794) 2023-05-11 [email protected] fix: platform_dispatcher documentation typo (flutter/engine#41739) 2023-05-11 [email protected] Remove GN staging flag for save layer bounds (flutter/engine#41940) 2023-05-11 [email protected] switch MockTexture off of MockCanvas calls (flutter/engine#41906) 2023-05-11 [email protected] [Android] Lifecycle defaults to focused instead of unfocused (flutter/engine#41875) 2023-05-11 [email protected] Roll Skia from ccf73af6ca91 to aed81125e6bc (9 revisions) (flutter/engine#41946) 2023-05-11 [email protected] Clobber caches in licenses test. (flutter/engine#41942) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 2tQjI0g3aDmj to MjcKzcsqMDku 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

The `isRTLAtPosition` method had a bug, it used `NSInteger max = [_selectionRects count]` instead of `NSInteger max = [_selectionRects count] - 1`. But I realized we don't even need the function any more, it was used in a few places in previous iterations of #36643, but in the only place remaining, we actually already have the selection rect and don't need to search for it by position. Btw as an explanation of the crash, I guess there is some mismatch between code point and character count somewhere. UIKit was asking for `caretRectForPosition:2` when we only had 1 character. This could have only crashed when floating cursor selection was used, but actually when switching to CJK keyboard, UIKit turns out to use `caretRectForPosition` to calculate something about the composing rect. Fixes flutter/flutter#128031

…42539) The `isRTLAtPosition` method had a bug, it used `NSInteger max = [_selectionRects count]` instead of `NSInteger max = [_selectionRects count] - 1`. But I realized we don't even need the function any more, it was used in a few places in previous iterations of flutter#36643, but in the only place remaining, we actually already have the selection rect and don't need to search for it by position. Btw as an explanation of the crash, I guess there is some mismatch between code point and character count somewhere. UIKit was asking for `caretRectForPosition:2` when we only had 1 character. This could have only crashed when floating cursor selection was used, but actually when switching to CJK keyboard, UIKit turns out to use `caretRectForPosition` to calculate something about the composing rect. Fixes flutter/flutter#128031

…42540) The `isRTLAtPosition` method had a bug, it used `NSInteger max = [_selectionRects count]` instead of `NSInteger max = [_selectionRects count] - 1`. But I realized we don't even need the function any more, it was used in a few places in previous iterations of flutter#36643, but in the only place remaining, we actually already have the selection rect and don't need to search for it by position. Btw as an explanation of the crash, I guess there is some mismatch between code point and character count somewhere. UIKit was asking for `caretRectForPosition:2` when we only had 1 character. This could have only crashed when floating cursor selection was used, but actually when switching to CJK keyboard, UIKit turns out to use `caretRectForPosition` to calculate something about the composing rect. Fixes flutter/flutter#128031

flutter-dashboard bot added the platform-ios label Oct 6, 2022

moffatman mentioned this pull request Oct 6, 2022

Send text selection rects to engine on iPhone flutter/flutter#113048

Merged

8 tasks

jmagman mentioned this pull request Oct 10, 2022

Support floating cursor double press on iOS flutter/flutter#30476

Closed

jmagman reviewed Oct 19, 2022

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm Outdated Show resolved Hide resolved

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm Outdated Show resolved Hide resolved

moffatman force-pushed the keyboard_select branch from 2e042e8 to 7f953c3 Compare October 20, 2022 05:49

moffatman requested a review from LongCatIsLooong October 26, 2022 17:47

jmagman previously approved these changes Nov 5, 2022

View reviewed changes

LongCatIsLooong approved these changes Dec 2, 2022

View reviewed changes

moffatman force-pushed the keyboard_select branch from 7f953c3 to e1d1d27 Compare December 8, 2022 22:22

moffatman mentioned this pull request Dec 8, 2022

Floating cursor cleanup flutter/flutter#116746

Merged

8 tasks

moffatman requested review from LongCatIsLooong and jmagman December 13, 2022 19:34

jmagman reviewed Dec 13, 2022

View reviewed changes

moffatman mentioned this pull request Dec 21, 2022

Send text direction in selection rects flutter/flutter#117436

Merged

8 tasks

moffatman added 7 commits May 9, 2023 15:30

Reduce vertical precision

031a6cc

It fixes issues with characters of varying heights Don't reduce the precision for pencil usage

Address feedback

2573435

Fix test failures

5725902

Some renames for clarity

d2ceb76

Simplify rect comparison function

9d8d65f

Put the loop back

58bd8ed

moffatman force-pushed the keyboard_select branch from e68ec30 to 58bd8ed Compare May 9, 2023 19:30

hellohuanlin approved these changes May 10, 2023

View reviewed changes

moffatman added the autosubmit Merge PR when tree becomes green via auto submit App label May 11, 2023

auto-submit bot merged commit b120180 into flutter:main May 11, 2023

engine-flutter-autoroll mentioned this pull request May 11, 2023

Roll Flutter Engine from f38f46f66e9a to b1201809a803 (11 revisions) flutter/flutter#126614

Closed

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

b1201809a iPhone floating cursor selection (flutter/engine#36643)

724bd9d

engine-flutter-autoroll mentioned this pull request May 11, 2023

Roll Flutter Engine from f38f46f66e9a to 6cb37b2a5626 (12 revisions) flutter/flutter#126618

Closed

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

b1201809a iPhone floating cursor selection (flutter/engine#36643)

a997953

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

Roll Flutter Engine from f38f46f66e9a to 748ef964b993 (13 revisions) flutter/flutter#126622

Merged

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

b1201809a iPhone floating cursor selection (flutter/engine#36643)

67cbba7

justinmc mentioned this pull request May 26, 2023

Double cursor using iOS floating cursor (keyboard cursor move with spacebar) flutter/flutter#89228

Closed

moffatman mentioned this pull request Jun 3, 2023

Fix crash with CJK keyboard with emoji at end of text field #42540

Merged

8 tasks

Jasguerrero mentioned this pull request Jun 16, 2023

Fix crash with CJK keyboard with emoji at end of text field (#42539) #42922

Closed

8 tasks

Jasguerrero mentioned this pull request Jun 16, 2023

[CP] Fix crash with CJK keyboard with emoji at end of text field (#42539) #42924

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iPhone floating cursor selection #36643

iPhone floating cursor selection #36643

moffatman commented Oct 6, 2022

chinmaygarde commented Oct 6, 2022

jmagman commented Oct 19, 2022

LongCatIsLooong commented Nov 1, 2022

moffatman commented Nov 1, 2022

jmagman left a comment

jmagman commented Nov 9, 2022

moffatman commented Nov 9, 2022

LongCatIsLooong commented Nov 17, 2022 •

edited

Loading

moffatman commented Nov 17, 2022

LongCatIsLooong commented Nov 23, 2022 •

edited

Loading

moffatman commented Nov 24, 2022

chinmaygarde commented Dec 1, 2022

LongCatIsLooong left a comment

LongCatIsLooong Dec 2, 2022

moffatman Dec 3, 2022

LongCatIsLooong Dec 3, 2022 •

edited

Loading

jmagman commented Dec 7, 2022

moffatman commented Dec 8, 2022

jmagman commented Dec 13, 2022

moffatman commented Dec 13, 2022

jmagman Dec 13, 2022

moffatman Dec 21, 2022 •

edited

Loading

jmagman Dec 13, 2022

moffatman Dec 14, 2022

moffatman Dec 14, 2022

moffatman commented Dec 16, 2022

hellohuanlin left a comment

hellohuanlin May 10, 2023

moffatman May 10, 2023

hellohuanlin May 10, 2023

moffatman May 10, 2023

hellohuanlin May 11, 2023

hellohuanlin left a comment

Text Range	Old rects returned	New rects returned
0..0	[0] with width zero	[0] with width zero
0..1	[0], [1]	[0]
0..2	[0], [1], [2]	[0], [1]

iPhone floating cursor selection #36643

iPhone floating cursor selection #36643

Conversation

moffatman commented Oct 6, 2022

Pre-launch Checklist

chinmaygarde commented Oct 6, 2022

jmagman commented Oct 19, 2022

LongCatIsLooong commented Nov 1, 2022

moffatman commented Nov 1, 2022

jmagman left a comment

Choose a reason for hiding this comment

jmagman commented Nov 9, 2022

moffatman commented Nov 9, 2022

LongCatIsLooong commented Nov 17, 2022 • edited Loading

moffatman commented Nov 17, 2022

LongCatIsLooong commented Nov 23, 2022 • edited Loading

moffatman commented Nov 24, 2022

chinmaygarde commented Dec 1, 2022

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong Dec 3, 2022 • edited Loading

Choose a reason for hiding this comment

jmagman commented Dec 7, 2022

moffatman commented Dec 8, 2022

jmagman commented Dec 13, 2022

moffatman commented Dec 13, 2022

Choose a reason for hiding this comment

moffatman Dec 21, 2022 • edited Loading

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

moffatman commented Dec 16, 2022

hellohuanlin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin left a comment

Choose a reason for hiding this comment

LongCatIsLooong commented Nov 17, 2022 •

edited

Loading

LongCatIsLooong commented Nov 23, 2022 •

edited

Loading

LongCatIsLooong Dec 3, 2022 •

edited

Loading

moffatman Dec 21, 2022 •

edited

Loading