[iOS] Remove selectionDidChange call in UndoManager #45657

LongCatIsLooong · 2023-09-11T18:15:44Z

The -[TextInputDelegate selectionDidChange:] call actually triggers some unwanted keyboard NLP actions that generate a bunch of candidates and automatically accept the first candidate. This causes -[UITextInput setMarkedText:selection] to be called with the first candidate and that inserts extraneous characters after the user types certain characters on the iPad software keyboard.

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.

LongCatIsLooong · 2023-09-11T19:29:24Z

/cc @fbcouch fyi -[TextInputDelegate selectionDidChange:] is causing some unexpected issues so I'm reverting to your original approach.

hellohuanlin · 2023-09-14T21:24:40Z

actually triggers some unwanted keyboard NLP actions that generate a bunch of candidates and automatically accept the first candidate.

Could you explain more about this? Did you see a bar showing up and immediately dismissed?

hellohuanlin · 2023-09-14T21:26:05Z

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

@@ -104,9 +104,11 @@ - (void)setUndoState:(NSDictionary*)dictionary API_AVAILABLE(ios(9.0)) {
    // This is needed to notify the iPadOS keyboard that it needs to update the


is this comment still relevant?

hellohuanlin · 2023-09-14T21:27:26Z

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

+        _viewController.engine.textInputPlugin.textInputView.inputAssistantItem;
+    NSArray<UIBarButtonItemGroup*>* leadingGroups = assistantItem.leadingBarButtonGroups;
+    assistantItem.leadingBarButtonGroups = @[];
+    assistantItem.leadingBarButtonGroups = leadingGroups;


hy is it setting leadingBarButtonGroups to empty array, and then setting back to the original value? Adding some comments here will be helpful.

This was a workaround to notify the iPadOS keyboard to update the state of these buttons. It's been awhile, but I believe the behavior I was seeing was that state changes to canUndo/canRedo would not show up until the next keypress – so if I press a letter, the undo button should light up, but it won't until I press another key.

Worth trying if it works if we simply do assistantItem.leandingBarButtonGroups = assistantItem.leandingBarButtonGroups, in case apple doesn't check for no-ops in the setter.

BTW if it's iOS 17 related, feel free to contribute to the design doc which already discusses about several iOS 17 text input issues. It can be a good reference doc for future developers.

the original issue is reproduced on an iOS 16 device so probably not ios specific.

I tested removing the intermediate empty array step it appears the setter indeed doesn't skip even when given the same array instance. Updated. I'll file a radar later since the keyboard could just rely on notifications to properly update the undo/redo UI.

hellohuanlin

LGTM. thanks for the update.

…r/engine#45657)

…134998) flutter/engine@e1c784e...589bde9 2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 4122791099ce to 744807d740c7 (1 revision) (flutter/engine#46019) 2023-09-19 jonahwilliams@google.com [Android] Add support for setting thread affinity based on core speed. (flutter/engine#45673) 2023-09-19 chinmaygarde@google.com [Impeller] Fix STB backend to account for max texture sizes. (flutter/engine#46010) 2023-09-19 matanlurey@users.noreply.github.com [Impeller] Hold the CommandPoolVK at a higher scope. (flutter/engine#46013) 2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 0c990ab9e097 to 4122791099ce (19 revisions) (flutter/engine#46016) 2023-09-18 kjlubick@users.noreply.github.com Add missing include of SkPath (flutter/engine#45996) 2023-09-18 chinmaygarde@google.com [Impeller] Respect max supported texture size when allocating glyph atlas texture. (flutter/engine#45992) 2023-09-18 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 3_Lh8otTpmVuf-Zwb... to qy5FU4y6sx1FscCpd... (flutter/engine#45998) 2023-09-18 chris@bracken.jp Revert "[Windows] Update vsync on raster thread (#45310)" (flutter/engine#46000) 2023-09-18 matanlurey@users.noreply.github.com Provide a default `--target-variant` for `clang_tidy`. (flutter/engine#45909) 2023-09-18 ychris@google.com Revert "[ios] use python script to generate extension safe frameworks and code sign them" (flutter/engine#46004) 2023-09-18 john@johnmccutchan.com Disable HardwareBuffer backed Platform Views temporarily (flutter/engine#45986) 2023-09-18 john@johnmccutchan.com Tighten up ImageReaderPlatformViewRenderTarget code (flutter/engine#45889) 2023-09-18 ychris@google.com [ios] use python script to generate extension safe frameworks and code sign them (flutter/engine#45781) 2023-09-18 bdero@google.com Bump impeller-cmake to HEAD. (flutter/engine#45953) 2023-09-18 31859944+LongCatIsLooong@users.noreply.github.com [iOS] Remove selectionDidChange call in UndoManager (flutter/engine#45657) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 3_Lh8otTpmVu to qy5FU4y6sx1F 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 bdero@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

…lutter#134998) flutter/engine@e1c784e...589bde9 2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 4122791099ce to 744807d740c7 (1 revision) (flutter/engine#46019) 2023-09-19 jonahwilliams@google.com [Android] Add support for setting thread affinity based on core speed. (flutter/engine#45673) 2023-09-19 chinmaygarde@google.com [Impeller] Fix STB backend to account for max texture sizes. (flutter/engine#46010) 2023-09-19 matanlurey@users.noreply.github.com [Impeller] Hold the CommandPoolVK at a higher scope. (flutter/engine#46013) 2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 0c990ab9e097 to 4122791099ce (19 revisions) (flutter/engine#46016) 2023-09-18 kjlubick@users.noreply.github.com Add missing include of SkPath (flutter/engine#45996) 2023-09-18 chinmaygarde@google.com [Impeller] Respect max supported texture size when allocating glyph atlas texture. (flutter/engine#45992) 2023-09-18 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 3_Lh8otTpmVuf-Zwb... to qy5FU4y6sx1FscCpd... (flutter/engine#45998) 2023-09-18 chris@bracken.jp Revert "[Windows] Update vsync on raster thread (flutter#45310)" (flutter/engine#46000) 2023-09-18 matanlurey@users.noreply.github.com Provide a default `--target-variant` for `clang_tidy`. (flutter/engine#45909) 2023-09-18 ychris@google.com Revert "[ios] use python script to generate extension safe frameworks and code sign them" (flutter/engine#46004) 2023-09-18 john@johnmccutchan.com Disable HardwareBuffer backed Platform Views temporarily (flutter/engine#45986) 2023-09-18 john@johnmccutchan.com Tighten up ImageReaderPlatformViewRenderTarget code (flutter/engine#45889) 2023-09-18 ychris@google.com [ios] use python script to generate extension safe frameworks and code sign them (flutter/engine#45781) 2023-09-18 bdero@google.com Bump impeller-cmake to HEAD. (flutter/engine#45953) 2023-09-18 31859944+LongCatIsLooong@users.noreply.github.com [iOS] Remove selectionDidChange call in UndoManager (flutter/engine#45657) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from 3_Lh8otTpmVu to qy5FU4y6sx1F 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 bdero@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

Fixes flutter/flutter#133424 The `-[TextInputDelegate selectionDidChange:]` call actually triggers some unwanted keyboard NLP actions that generate a bunch of candidates and automatically accept the first candidate. This causes `-[UITextInput setMarkedText:selection]` to be called with the first candidate and that inserts extraneous characters after the user types certain characters on the iPad software keyboard. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Remove selectionDidChange call in ios undo manager

0c2d559

github-actions bot added the platform-ios label Sep 11, 2023

LongCatIsLooong requested a review from cyanglaz September 11, 2023 19:26

LongCatIsLooong marked this pull request as ready for review September 11, 2023 19:26

LongCatIsLooong requested a review from hellohuanlin September 14, 2023 18:57

hellohuanlin reviewed Sep 14, 2023

View reviewed changes

hellohuanlin approved these changes Sep 15, 2023

View reviewed changes

review

d9da08f

LongCatIsLooong force-pushed the remove-selectionDidChange branch from 8333758 to d9da08f Compare September 15, 2023 21:58

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 18, 2023

auto-submit bot merged commit 2178248 into flutter:main Sep 18, 2023
25 checks passed

LongCatIsLooong deleted the remove-selectionDidChange branch September 18, 2023 17:32

engine-flutter-autoroll mentioned this pull request Sep 18, 2023

Roll Flutter Engine from e1c784e3f841 to 217824895cad (1 revision) flutter/flutter#134960

Closed

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

168e6bb

…r/engine#45657)

engine-flutter-autoroll mentioned this pull request Sep 18, 2023

Roll Flutter Engine from e1c784e3f841 to 945a79c8b642 (3 revisions) flutter/flutter#134962

Closed

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

c8d124a

…r/engine#45657)

engine-flutter-autoroll mentioned this pull request Sep 18, 2023

Roll Flutter Engine from e1c784e3f841 to a55da8279022 (4 revisions) flutter/flutter#134964

Closed

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

71ea4ff

…r/engine#45657)

engine-flutter-autoroll mentioned this pull request Sep 18, 2023

Roll Flutter Engine from e1c784e3f841 to ab30b6812f12 (5 revisions) flutter/flutter#134976

Closed

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

7134a83

…r/engine#45657)

engine-flutter-autoroll mentioned this pull request Sep 19, 2023

Roll Flutter Engine from e1c784e3f841 to 87396c00500a (12 revisions) flutter/flutter#134995

Closed

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

8f00243

…r/engine#45657)

engine-flutter-autoroll mentioned this pull request Sep 19, 2023

Roll Flutter Engine from e1c784e3f841 to 3303a3e040b1 (13 revisions) flutter/flutter#134996

Closed

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

ddab731

…r/engine#45657)

engine-flutter-autoroll mentioned this pull request Sep 19, 2023

Roll Flutter Engine from e1c784e3f841 to 64705ac76734 (14 revisions) flutter/flutter#134997

Closed

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

e4521d7

…r/engine#45657)

engine-flutter-autoroll mentioned this pull request Sep 19, 2023

Roll Flutter Engine from e1c784e3f841 to 589bde9a95c9 (16 revisions) flutter/flutter#134998

Merged

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

217824895 [iOS] Remove selectionDidChange call in UndoManager (flutte…

7d8d1c9

…r/engine#45657)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS] Remove selectionDidChange call in UndoManager #45657

[iOS] Remove selectionDidChange call in UndoManager #45657

LongCatIsLooong commented Sep 11, 2023 •

edited

LongCatIsLooong commented Sep 11, 2023

hellohuanlin commented Sep 14, 2023

hellohuanlin Sep 14, 2023

hellohuanlin Sep 14, 2023 •

edited

fbcouch Sep 14, 2023

hellohuanlin Sep 15, 2023

hellohuanlin Sep 15, 2023

LongCatIsLooong Sep 15, 2023

hellohuanlin left a comment

		@@ -104,9 +104,11 @@ - (void)setUndoState:(NSDictionary*)dictionary API_AVAILABLE(ios(9.0)) {
		// This is needed to notify the iPadOS keyboard that it needs to update the

[iOS] Remove selectionDidChange call in UndoManager #45657

[iOS] Remove selectionDidChange call in UndoManager #45657

Conversation

LongCatIsLooong commented Sep 11, 2023 • edited

Pre-launch Checklist

LongCatIsLooong commented Sep 11, 2023

hellohuanlin commented Sep 14, 2023

hellohuanlin Sep 14, 2023

Choose a reason for hiding this comment

hellohuanlin Sep 14, 2023 • edited

Choose a reason for hiding this comment

fbcouch Sep 14, 2023

Choose a reason for hiding this comment

hellohuanlin Sep 15, 2023

Choose a reason for hiding this comment

hellohuanlin Sep 15, 2023

Choose a reason for hiding this comment

LongCatIsLooong Sep 15, 2023

Choose a reason for hiding this comment

hellohuanlin left a comment

Choose a reason for hiding this comment

LongCatIsLooong commented Sep 11, 2023 •

edited

hellohuanlin Sep 14, 2023 •

edited