[ios]fix ios 16 auto correction highlight showing on top left corner #47279

hellohuanlin · 2023-10-24T20:28:18Z

This PR hides the system highlights in iOS 16 (except when scribble is enabled).

Note that auto correction highlight is still drawn by flutter, so it still works.

I don't think we need to CP this, since it's iOS 16 only, and iOS 17 is already out.

Why not use system highlight?

Unlike iOS 17, the iOS 16 system highlight only respect the width provided by firstRectForRange, but not the height. I have audited all sizing related APIs in UITextInput (doc here), specifically, firstRect(for:), caretRect(for:) and selectionRects(for:), and they all return the correct height.

About scribble

The initial implementation of firstRectForRange (that returns the first selection rect) was introduced for the scribble feature on iPad (code here).

It turns out that a non-zero rect is required for scribble's advanced feature to work (e.g. inserting a space with a vertical bar). So we can't apply this fix for scribble.

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

Fixes flutter/flutter#136802

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 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.

hellohuanlin · 2023-10-27T18:46:21Z

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

-    CGFloat maxX = fmax(CGRectGetMaxX(startSelectionRect), CGRectGetMaxX(endSelectionRect));
-    return CGRectMake(minX, minY, maxX - minX, maxY - minY);
+    // Auto correction highlight on iOS 16 and older is rendered by Flutter.
+    return CGRectZero;


The initial implementation of firstRectForRange that returns the first selection rect (which, by the way, was incorrect) was introduced for the scribble feature on iPad (code here). It turns out that the change was not necessary for scribble - I have verified that keep returning CGRectZero doesn't break scribble.

LongCatIsLooong

Could you double tap the soft keyboard's space bar and verify floating cursor selection (which is different from the regular floating cursor) still works on iOS < 17?

/cc @fbcouch do you remember if there's a scribble call path that contains firstRectForRange: ?

fbcouch · 2023-10-27T20:30:31Z

@LongCatIsLooong I can't recall now what exactly it was required for, but I believe it was called at some point in the scribble process or I wouldn't have messed with it – I think the big thing is to make sure that the "advanced" scribble features still work on iPadOS 16, so selecting, deleting, adding/removing spaces (draw a vertical line between characters) and long hold to get extra space to write in.

hellohuanlin · 2023-10-27T22:47:52Z

Good point. Let me double check those features.

hellohuanlin · 2023-10-28T01:23:00Z

It turns out that for those advanced features in scribble to work (e.g. inserting a space with a vertical bar), we have to return a rect of at least 1 character length. So I updated the code to only hide this highlight when scribble is not available.

This isn't ideal, because scribble devices (i.e. iPad Pro & Air) will still see the highlight. Let me investigate more about UITextInput if we can use system highlight just like in iOS 17, but this PR will be a fallback solution if no luck there.

zanderso · 2023-11-02T20:06:49Z

From PR review triage: Looks like this is still WIP, so I will add the tag.

hellohuanlin · 2023-11-14T20:18:37Z

I think this is ready for another review @LongCatIsLooong @stuartmorgan

LongCatIsLooong · 2023-11-14T22:58:02Z

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

+        // API (unlike iOS 17). So we return CGRectZero to hide it (unless if scribble is enabled).
+        // To support scribble's advanced gestures (e.g. insert a space with a vertical bar),
+        // at least 1 character's width is required.
+        return [self isScribbleAvailable] ? _selectionRects[i].rect : CGRectZero;


Why doing the check and returning CGRectZero here (in a loop), instead of, say, before the loop?

Also could you elaborate a bit on what caused the highlight to show at incorrect location? You mentioned the height isn't respected but that shouldn't affect the offset of the rects right?

Also could you elaborate a bit on what caused the highlight to show at incorrect location?

The system highlight shows at the correct location. It should be removed (on iOS 16) because we already show the highlight in Flutter.

flutter/flutter#136802 seems to indicate it's showing at the top left corner?

oh i misunderstood you. that is when scribble is not available, where the FlutterTextInputView is positioned at CGRectZero. Since we have to hide it anyways, I did not move it when scribble is not available.

On the other hand, when scribble is available, the FlutterTextInputView has to be moved to overlap with the actual rendered text field. Which unfortunately means there will be a small highlight region for scribble case, and I don't see a solution to fix that case, unless we can use system highlight in iOS 16 as well, which i don't think is doable (see my PR description).

Thanks for the explanation. I thought we only receive the rects on iPad but it looks like in the framework we always send the character bounding boxes by default as long as the target platform is iOS.

hellohuanlin · 2023-11-15T18:54:39Z

It's shown as approved but i think it was stamped before I updated my code. So I'd like another stamp from you @LongCatIsLooong

LongCatIsLooong · 2023-11-15T18:58:23Z

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

+  // at least 1 character's width is required.
+  if (@available(iOS 17, *)) {
+    // No-op
+  } else {


uber nit: else if

LongCatIsLooong

LGTM

…ft corner (flutter/engine#47279)

…138529) flutter/engine@3cfcdeb...8aff9c1 2023-11-16 [email protected] Roll Dart SDK from cc6acfd7d57c to 5cccc24d127f (1 revision) (flutter/engine#48109) 2023-11-16 [email protected] Roll Skia from 5d6bdbf69dea to add865f891c8 (1 revision) (flutter/engine#48108) 2023-11-16 [email protected] Roll Dart SDK from 65819963fb17 to cc6acfd7d57c (5 revisions) (flutter/engine#48100) 2023-11-16 [email protected] Make `fml/status_or.h` compatible with `.clang_tidy`. (flutter/engine#48002) 2023-11-16 [email protected] [Impeller] Gate Vulkan selection on API 29 (flutter/engine#48089) 2023-11-16 [email protected] [macOS] Clean up allocations in menu plugin test (flutter/engine#48093) 2023-11-16 [email protected] Re-land "Make `fml/...` compatible with `.clang_tidy` (flutter/engine#48030) 2023-11-15 [email protected] [ios] introduce weak_nsobject (flutter/engine#47947) 2023-11-15 [email protected] Roll Skia from e954d1a1972c to 5d6bdbf69dea (2 revisions) (flutter/engine#48094) 2023-11-15 [email protected] [Impeller] add async command submission for blit pass. (flutter/engine#48040) 2023-11-15 [email protected] Make `lib/ui/compositing/...` compatible with `.clang_tidy`. (flutter/engine#48001) 2023-11-15 [email protected] Remove the linux fuchsia v1 build. (flutter/engine#48085) 2023-11-15 [email protected] [web] Apply global styles before inserting the DOM element (flutter/engine#48027) 2023-11-15 [email protected] Roll Skia from b23074a79bda to e954d1a1972c (7 revisions) (flutter/engine#48092) 2023-11-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`." (flutter/engine#48083) 2023-11-15 [email protected] [ios]fix ios 16 auto correction highlight showing on top left corner (flutter/engine#47279) 2023-11-15 [email protected] Roll Skia from c42226314a4f to b23074a79bda (3 revisions) (flutter/engine#48081) 2023-11-15 [email protected] Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`. (flutter/engine#48000) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added the platform-ios label Oct 24, 2023

hellohuanlin changed the title ~~wip~~ [ios]fix ios 16 auto correction highlight showing on top left corner Oct 24, 2023

hellohuanlin force-pushed the auto_correction_highlight_ios_16 branch from b2287dd to 4df0fc6 Compare October 27, 2023 18:25

hellohuanlin marked this pull request as ready for review October 27, 2023 18:41

hellohuanlin added 4 commits October 27, 2023 11:41

[ios]fix ios 16 auto selection highlight on top left corner

e964865

format

e0e7f96

add tests

6ebe8df

format

7deccfd

hellohuanlin force-pushed the auto_correction_highlight_ios_16 branch from 4df0fc6 to 7deccfd Compare October 27, 2023 18:41

hellohuanlin requested review from LongCatIsLooong and stuartmorgan-g October 27, 2023 18:42

hellohuanlin commented Oct 27, 2023

View reviewed changes

LongCatIsLooong approved these changes Oct 27, 2023

View reviewed changes

scribble fix

e2d32f1

hellohuanlin force-pushed the auto_correction_highlight_ios_16 branch 2 times, most recently from 0e10ead to 2fc762b Compare October 28, 2023 01:15

format

8c84515

hellohuanlin force-pushed the auto_correction_highlight_ios_16 branch from 2fc762b to 8c84515 Compare October 28, 2023 01:17

zanderso added the Work in progress (WIP) Not ready (yet) for review! label Nov 2, 2023

hellohuanlin requested a review from LongCatIsLooong November 14, 2023 20:18

hellohuanlin removed the Work in progress (WIP) Not ready (yet) for review! label Nov 14, 2023

LongCatIsLooong reviewed Nov 14, 2023

View reviewed changes

address comment

066a643

hellohuanlin requested a review from LongCatIsLooong November 15, 2023 18:52

LongCatIsLooong reviewed Nov 15, 2023

View reviewed changes

LongCatIsLooong approved these changes Nov 15, 2023

View reviewed changes

nit

337dbf2

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

auto-submit bot merged commit a0a1a1d into flutter:main Nov 15, 2023

engine-flutter-autoroll mentioned this pull request Nov 16, 2023

Roll Flutter Engine from 3cfcdebe8623 to 8aff9c134b8f (18 revisions) flutter/flutter#138529

Merged

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

a0a1a1d25 [ios]fix ios 16 auto correction highlight showing on top le…

2ad12a7

…ft corner (flutter/engine#47279)

[ios]fix ios 16 auto correction highlight showing on top left corner #47279

[ios]fix ios 16 auto correction highlight showing on top left corner #47279

Uh oh!

Conversation

hellohuanlin commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why not use system highlight?

About scribble

Pre-launch Checklist

Uh oh!

hellohuanlin Oct 27, 2023 • 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!

LongCatIsLooong left a comment • 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!

fbcouch commented Oct 27, 2023

Uh oh!

hellohuanlin commented Oct 27, 2023

Uh oh!

hellohuanlin commented Oct 28, 2023

Uh oh!

zanderso commented Nov 2, 2023

Uh oh!

hellohuanlin commented Nov 14, 2023

Uh oh!

LongCatIsLooong Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Nov 14, 2023 • 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!

LongCatIsLooong Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

hellohuanlin Nov 14, 2023 • 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!

LongCatIsLooong Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

hellohuanlin commented Nov 15, 2023

Uh oh!

LongCatIsLooong Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hellohuanlin commented Oct 24, 2023 •

edited

Loading

hellohuanlin Oct 27, 2023 •

edited

Loading

LongCatIsLooong left a comment •

edited

Loading

hellohuanlin Nov 14, 2023 •

edited

Loading

hellohuanlin Nov 14, 2023 •

edited

Loading