[ios][ios17]fix auto correction highlight on top left corner #44779

hellohuanlin · 2023-08-16T20:12:45Z

Fix native auto-correction highlight region on top left corner.

This PR uses the system auto-correction highlight on iOS 17, which was disabled by #44354

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

Fixes flutter/flutter#131622
Fixes flutter/flutter#131695
Fixes flutter/flutter#130818

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.

stuartmorgan-g · 2023-08-16T20:21:49Z

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

+  NSArray* transform = dictionary[@"transform"];
+  [_activeView setEditableTransform:transform];
+  int leftIndex = 12;
+  int topIndex = 13;


Nit: these should be const.

stuartmorgan-g · 2023-08-16T20:25:01Z

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

+    _activeView.selectionRects = rectsAsRect;
+    [_activeView.inputDelegate textDidChange:_activeView];
+  } else {
+    _activeView.selectionRects = rectsAsRect;


Rather than duplicate this line, which could cause drift over time, we should refactor to look more like the other method where there's a bool for sending updates.:

BOOL triggerTextUpdates = NO; if (@available(iOS 17, *)) { // move explanatory comment here. triggerTextUpdates = YES; } if (triggerTextUpdates) { [_activeView.inputDelegate textWillChange:_activeView]; } _activeView.selectionRects = rectsAsRect; if (triggerTextUpdates) { [_activeView.inputDelegate textDidChange:_activeView]; }

stuartmorgan-g · 2023-08-16T20:27:27Z

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

    _activeView.frame =
        CGRectMake(0, 0, [dictionary[@"width"] intValue], [dictionary[@"height"] intValue]);
    _activeView.tintColor = [UIColor clearColor];
+  } else {
+    if (@available(iOS 17, *)) {


Please add a TODO to simplify all of this logic. It looks like post cherry-pick we can probably just make the _inputHider.frame logic unconditional?

The iOS 16 behavior is different - the highlight region does not match the size of text (see screenshot in flutter/flutter#131695 for iOS 16). But will add a TODO to explain this.

justinmc

Some comments to make sure I understand how this bug affects versions before iOS 17, but looks good overall.

justinmc · 2023-08-16T22:31:34Z

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

+      // This is to fix an issue where the system auto-correction highlight is displayed at
+      // the top left corner of the screen on iOS 17+.
+      // See https://github.com/flutter/flutter/issues/131695


The autocorrect highlight is also displayed in the top left corner on earlier versions, it's just smaller, right? This sentence seems misleading, maybe remove it but keep the "See...". Really this is just positioning the input in the correct place, the place as Flutter's input.

That's right. How about instead of deleting it, I add a new line saying "This problem also happens on iOS 16, but the size of highlight does not match the text"?

justinmc · 2023-08-16T22:32:16Z

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

+      // the top left corner of the screen on iOS 17+.
+      // See https://github.com/flutter/flutter/issues/131695
+      _inputHider.frame =
+          CGRectMake([transform[leftIndex] intValue], [transform[topIndex] intValue], 0, 0);


What happens if you pass the real width and height here like in the scribble case above instead of 0,0?
[dictionary[@"width"] intValue], [dictionary[@"height"] intValue]

I've tried that and got the same result in my tests. Nothing seems to break. But wanted to be safe and keep the same zero size as before.

(This is one of the things I was thinking would be part of unifying the these two codepaths in a follow-up, master-only PR.)

added a todo there.

justinmc · 2023-08-16T22:33:40Z

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

+  BOOL shouldNotifyTextChange = NO;
+  if (@available(iOS 17, *)) {
+    // Force UIKit to query the selectionRects again on iOS 17+
+    // This is to fix a bug on iOS 17+ where UIKit queries the outdated selectionRects after


Similar to above, I think this bug exists on all versions of iOS, right?

Not in this case - in iOS 16, we do not use system highlight, so no such bug.

I also tried to notify the change on iOS 16 and nothing seems to break, but just to be safe here.

stuartmorgan-g

LGTM

stuartmorgan-g · 2023-08-17T14:42:15Z

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

+      // the top left corner of the screen on iOS 17+.
+      // See https://github.com/flutter/flutter/issues/131695
+      _inputHider.frame =
+          CGRectMake([transform[leftIndex] intValue], [transform[topIndex] intValue], 0, 0);


(This is one of the things I was thinking would be part of unifying the these two codepaths in a follow-up, master-only PR.)

flutter/engine#44779)

…132797) flutter/engine@5336702...5019d6d 2023-08-17 dkwingsmt@users.noreply.github.com Basic view management for engine classes (flutter/engine#42991) 2023-08-17 skia-flutter-autoroll@skia.org Roll Skia from d0d390f9310d to bfd45173e5e3 (5 revisions) (flutter/engine#44820) 2023-08-17 matanlurey@users.noreply.github.com Implement 2 suggested Clang Tidy fixes we don't look for yet. (flutter/engine#44816) 2023-08-17 louiseh0313@gmail.com Add share to selection controls (flutter/engine#44554) 2023-08-17 zanderso@users.noreply.github.com Adds new builders for partial clang-tidy checks. (flutter/engine#44811) 2023-08-17 41930132+hellohuanlin@users.noreply.github.com [ios][ios17]fix auto correction highlight on top left corner (flutter/engine#44779) 2023-08-17 109111084+yaakovschectman@users.noreply.github.com [Windows] Delay enabling app lifecycle states until requested (flutter/engine#44238) 2023-08-17 dkwingsmt@users.noreply.github.com Move `viewConfiguration` parsing from `PlatformDispatcher` to `_hooks` (flutter/engine#44787) 2023-08-17 skia-flutter-autoroll@skia.org Roll Dart SDK from 92c32df13d31 to 7e4e5796ee99 (2 revisions) (flutter/engine#44810) 2023-08-17 skia-flutter-autoroll@skia.org Roll Skia from c4805a975ab3 to d0d390f9310d (2 revisions) (flutter/engine#44807) 2023-08-17 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from cPncZK6z8HmuOmQr_... to 7xOzci7fempFgHNk9... (flutter/engine#44809) 2023-08-17 skia-flutter-autoroll@skia.org Roll Skia from efb5a5e0b78b to c4805a975ab3 (2 revisions) (flutter/engine#44795) 2023-08-17 skia-flutter-autoroll@skia.org Roll Skia from 11cb8cdd37c1 to efb5a5e0b78b (1 revision) (flutter/engine#44792) 2023-08-17 matanlurey@users.noreply.github.com Passthrough stderr results of clang_tidy when --enable-check-profile. (flutter/engine#44789) 2023-08-17 skia-flutter-autoroll@skia.org Roll Dart SDK from d6e1fca5dbdf to 92c32df13d31 (1 revision) (flutter/engine#44788) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from cPncZK6z8Hmu to 7xOzci7fempF 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 jsimmons@google.com,rmistry@google.com,zra@google.com 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

cyanglaz · 2023-08-18T17:33:26Z

I checked out this fix, now the system popup is not showing at all for flutter/flutter#132594

justinmc · 2023-08-18T17:57:40Z

Hmm that's not what I would expect for this change... @cyanglaz You tried it on iOS 17?

cyanglaz · 2023-08-18T18:03:42Z

yes on simulator tho

…OS 17 (again) (#44812) CP for #44779 Tested the stable branch with the fix on iPhone and iPad, with hardware and software keyboards, and with and without IME languages, iOS 16 and iOS 17 *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#131622 Fixes flutter/flutter#131695 Fixes flutter/flutter#130818 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

cyanglaz · 2023-08-21T16:23:24Z

@justinmc I saw that the fix is also cherry-picked. Can you or someone else confirm the issue that I saw and maybe we need to revert this fix and the cp if the issue does exist?

hellohuanlin · 2023-08-21T16:52:15Z

Hmmm strange. When I tried it the system menu shows right next to the text field (the correct position) rather than the top left corner. But it did show up

hellohuanlin · 2023-08-21T17:28:54Z

When i tried it on the release candidate branch (with the fix), the button does show up. But looks like the buttons are tappable. Here's iPhone simulator + iOS 17:

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-21.at.10.26.35.mp4

Hmmm, I may have remembered incorrectly previously. Could be a false alarm. Sorry :(

…#44779) Fix native auto-correction highlight region on top left corner. This PR uses the system auto-correction highlight on iOS 17, which was disabled by flutter#44354 <img width="479" alt="Screenshot 2023-08-16 at 1 19 39 PM" src="https://github.com/flutter/engine/assets/41930132/a5a1dda7-ba21-462e-a65c-1afeecf7559f"> *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#131622 Fixes flutter/flutter#131695 Fixes flutter/flutter#130818 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…ain)" (#45523) This reverts part of #44779. Verified this to be the regression for flutter/flutter#133908. This pair of `textWill/DidChange` calls was added in Beta 1, in order to fix highlight region missing last character (more details [here](https://docs.google.com/document/d/1sM3HMv-SQin39yX1aPUU7vtGv7Hcef1Quc3QhRXBl6A/edit?resourcekey=0-SFYD8vmOIkXiXCZvB1Wlcw#heading=h.ddlvu2i2epyl)). However, that fix doesn't work anymore for Beta 7 - it turns out that in Beta 7, the system doesn't rely on the `selectionRects` to determine the highlight region anymore. Instead, the system relies on `firstRectForRange` API. So we fixed the system highlight in `firstRectForRange` in Beta 7. So even after reverting this change, the system highlight still works correctly in Beta 7. I was initially leaning towards keeping these calls even after Beta 7, because without it, the `selectionRects` are still out of sync with iOS text input system. (more details [here](https://docs.google.com/document/d/1sM3HMv-SQin39yX1aPUU7vtGv7Hcef1Quc3QhRXBl6A/edit?resourcekey=0-SFYD8vmOIkXiXCZvB1Wlcw)). However, I was able to verify that it is indeed `textWill/DidChange` that introduced the regression for IME inputs (It's unclear why they are related, so need further investigation). In summary, after this revert, system highlights are still in the right place, and with the right length. *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#133908 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

hellohuanlin mentioned this pull request Aug 16, 2023

[ios][iOS 17] fix auto-correction highlight on top left corner #44604

Closed

8 tasks

github-actions bot added the platform-ios label Aug 16, 2023

[ios][ios17]fix auto correction highlight on top left corner

7d09940

hellohuanlin force-pushed the top_left_highlight_again branch from 23b0e46 to 7d09940 Compare August 16, 2023 20:14

hellohuanlin marked this pull request as ready for review August 16, 2023 20:16

hellohuanlin mentioned this pull request Aug 16, 2023

[iOS][iOS 17] IME shows at the wrong place for text input on iPhone + iOS 17 + hardware keyboard flutter/flutter#132551

Open

2 tasks

stuartmorgan-g suggested changes Aug 16, 2023

View reviewed changes

address some comments and nits

56edf77

hellohuanlin requested a review from stuartmorgan-g August 16, 2023 21:19

fix todo

6ab4faa

hellohuanlin requested review from justinmc and LongCatIsLooong August 16, 2023 22:21

justinmc reviewed Aug 16, 2023

View reviewed changes

stuartmorgan-g approved these changes Aug 17, 2023

View reviewed changes

updated some comments

18c8269

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

hellohuanlin mentioned this pull request Aug 17, 2023

[cp][ios][ios17]fix auto correction highlight on top left corner on iOS 17 (again) #44812

Merged

8 tasks

auto-submit bot merged commit 5c05519 into flutter:main Aug 17, 2023

engine-flutter-autoroll mentioned this pull request Aug 17, 2023

Roll Flutter Engine from 533670269c99 to 9fae689d10a9 (12 revisions) flutter/flutter#132785

Closed

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

5c05519b7 [ios][ios17]fix auto correction highlight on top left corner (

488ad19

flutter/engine#44779)

engine-flutter-autoroll mentioned this pull request Aug 17, 2023

Roll Flutter Engine from 533670269c99 to 5019d6da655c (15 revisions) flutter/flutter#132797

Merged

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

5c05519b7 [ios][ios17]fix auto correction highlight on top left corner (

6f3ccca

flutter/engine#44779)

hellohuanlin mentioned this pull request Sep 7, 2023

Reverts part of "fix auto-correction highlight on top left corner (Again)" #45523

Merged

8 tasks

[ios][ios17]fix auto correction highlight on top left corner #44779

[ios][ios17]fix auto correction highlight on top left corner #44779

Uh oh!

Conversation

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

Uh oh!

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!

justinmc 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!

hellohuanlin Aug 16, 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!

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!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 18, 2023

Uh oh!

justinmc commented Aug 18, 2023

Uh oh!

cyanglaz commented Aug 18, 2023

Uh oh!

cyanglaz commented Aug 21, 2023

Uh oh!

hellohuanlin commented Aug 21, 2023

Uh oh!

hellohuanlin commented Aug 21, 2023

Uh oh!

Uh oh!

hellohuanlin commented Aug 16, 2023 •

edited

Loading

hellohuanlin Aug 16, 2023 •

edited

Loading