Workaround iOS text input crash for emoji+Korean text by cyanglaz · Pull Request #36295 · flutter/engine

cyanglaz · 2022-09-20T20:14:53Z

This is what iOS 16 does when transforming 2 Korean characters into 1 (for example ㅇ ㅏ) right after a unicode composed character (emoji etc):

The keyboard deletes the emoji.
The keyboard API inserts the composed character back.
The keyboard API inserts the transformed Korean character.

However, in iOS 16.0, there is a bug that in step 2, only the first unicode is inserted, which creates an incomplete or different composed character.

To workaround this issue, this PR:

Cache the deleted composed character.
Detects when iOS only adds the first unicode of the composed character, then replaced the first unicode with the cached input.

The issue is caused by an iOS bug and this PR only provides a workaround, we should revert this workaround when the issue is fixed by iOS.

fixes flutter/flutter#111494

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.

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

+  if (emoji) {
+    self.lastEmoji = emoji;
+  }
+  if (text.length == 1 && !text.UTF8String &&


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

+  dispatch_once(&onceToken, ^{
+    set = CFCharacterSetCreateMutableCopy(
+        kCFAllocatorDefault,
+        CTFontCopyCharacterSet(CTFontCreateWithName(CFSTR("AppleColorEmoji"), 0.0, NULL)));


cyanglaz · 2022-09-21T22:38:59Z

#34508 seems similar and we might be able to use some of the tests in the PR.

jmagman · 2022-09-21T22:41:23Z

engine/shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.mm

Lines 421 to 439 in a7accef

    
           - (void)testDeletingBackward { 
        
             NSDictionary* config = self.mutableTemplateCopy; 
        
             [self setClientId:123 configuration:config]; 
        
             NSArray<FlutterTextInputView*>* inputFields = self.installedInputViews; 
        
             FlutterTextInputView* inputView = inputFields[0]; 
        
             [inputView insertText:@"ឹ😀 text 🥰👨‍👩‍👧‍👦🇺🇳ดี "]; 
        
             [inputView deleteBackward]; 
        
             [inputView deleteBackward]; 
        
             // Thai vowel is removed. 
        
             XCTAssertEqualObjects(inputView.text, @"ឹ😀 text 🥰👨‍👩‍👧‍👦🇺🇳ด"); 
        
             [inputView deleteBackward]; 
        
             XCTAssertEqualObjects(inputView.text, @"ឹ😀 text 🥰👨‍👩‍👧‍👦🇺🇳"); 
        
             [inputView deleteBackward]; 
        
             XCTAssertEqualObjects(inputView.text, @"ឹ😀 text 🥰👨‍👩‍👧‍👦"); 
        
             [inputView deleteBackward]; 
        
             XCTAssertEqualObjects(inputView.text, @"ឹ😀 text 🥰"); 
        
             [inputView deleteBackward];

LongCatIsLooong

This approach LGTM. I think the keyboard also handles grapheme clusters wrong. I'll check what the exact behavior is.

LongCatIsLooong · 2022-09-21T23:07:15Z

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

+  }
+  // Only need to use lastComposedCharacters for the one insertion right after deletion.
+  // Clear it even it is not used in the above if block.
+  self.lastComposedCharacters = nil;


also maybe set the stored value to nil at the end of the runloop?

LongCatIsLooong · 2022-09-21T23:09:47Z

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

+    // Workaround for https://github.com/flutter/flutter/issues/111494
+    // TODO(cyanglaz): revert this workaround if when flutter supports a minimum iOS version which
+    // this bug is fixed by Apple.
+    text = self.lastComposedCharacters;


So this step adds the emoji back, then the IME will call insertText: again to insert "어"?

why the text.length == 1 check? Shouldn't !text.UTF8String be enough (I'm assuming this checks the string is not malformed)?

nvm. I tried some grapheme clusters, the keyboard always inserts the first code unit.

So this step adds the emoji back, then the IME will call insertText: again to insert "어"?

Yes. I'm going to add what the "bugged" behavior actually is in the PR description.

LongCatIsLooong · 2022-09-23T23:02:38Z

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

    // TODO(cyanglaz): revert this workaround if when flutter supports a minimum iOS version which
    // this bug is fixed by Apple.
-    text = self.lastComposedCharacters;
+    text = self.temporarilyDeletedComposedCharacter;


nit: also clear the stored string here since it's consumed?

LongCatIsLooong · 2022-09-23T23:03:19Z

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

+  [inputView deleteBackward];
+  [inputView shouldChangeTextInRange:OCMClassMock([UITextRange class]) replacementText:@""];
+
+  // Insert the first unicode in the emoji.


nit: first unicode -> first unichar

LongCatIsLooong · 2022-09-23T23:08:16Z

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

@@ -1956,9 +1961,8 @@ - (void)deleteBackward {
    NSString* deletedText = [self.text substringWithRange:_selectedTextRange.range];
    CFRange range =


nit: consider using fml::RangeForCharacterAtIndex

jmagman · 2022-09-28T20:25:18Z

@cyanglaz is out this week, he will pick the nits for next week.

format fix fix fix delete multiple remove unused code add comments format fix comments fix clang tidy fix autorelease tests release temporarilyDeletedComposedCharacter after use, use more accurate verbose

jmagman · 2022-10-04T18:28:24Z

@cyanglaz is this ready to merge?

cyanglaz · 2022-10-04T19:55:30Z

@LongCatIsLooong Updated to use fml::RangeForCharacterAtIndex, also refactored out an "IsEmoji" method, ptal

LongCatIsLooong · 2022-10-04T20:09:57Z

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

-                                        range:charRange
-                               remainingRange:NULL];
-      if (gotCodePoint && u_hasBinaryProperty(codePoint, UCHAR_EMOJI)) {
+      if (IsEmoji(self.text, charRange)) {


nit: some emojis are in plane 0 and require only one code point to encode, e.g.: https://www.compart.com/en/unicode/U+2764

LongCatIsLooong

still LGTM

…r/engine#36295)

* Fix an issue that deleting an emoji may crash the app (#34508) * Workaround iOS text input crash for emoji+Korean text (#36295) Co-authored-by: Yang Chao <6036532+Yeatse@users.noreply.github.com> Co-authored-by: Chris Yang <ychris@google.com>

flutter-dashboard bot added the platform-ios label Sep 20, 2022

cyanglaz commented Sep 20, 2022

View reviewed changes

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

if (emoji) {

self.lastEmoji = emoji;

}

if (text.length == 1 && !text.UTF8String &&

This comment was marked as outdated.

Sign in to view

cyanglaz commented Sep 20, 2022

View reviewed changes

cyanglaz force-pushed the textfield_crash branch from 99125e9 to 12dff9a Compare September 21, 2022 17:53

cyanglaz requested a review from LongCatIsLooong September 21, 2022 18:02

cyanglaz force-pushed the textfield_crash branch from 12dff9a to ad3c308 Compare September 21, 2022 18:02

LongCatIsLooong reviewed Sep 21, 2022

View reviewed changes

cyanglaz marked this pull request as ready for review September 23, 2022 20:10

LongCatIsLooong approved these changes Sep 23, 2022

View reviewed changes

draft fix

b5856e7

format fix fix fix delete multiple remove unused code add comments format fix comments fix clang tidy fix autorelease tests release temporarilyDeletedComposedCharacter after use, use more accurate verbose

cyanglaz force-pushed the textfield_crash branch from b2806a2 to b5856e7 Compare October 3, 2022 17:19

jmagman mentioned this pull request Oct 3, 2022

[iOS 16] Flutter apps crash when emojis and text are combined. flutter/flutter#111494

Closed

review

9afb4e1

LongCatIsLooong reviewed Oct 4, 2022

View reviewed changes

LongCatIsLooong approved these changes Oct 4, 2022

View reviewed changes

cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2022

auto-submit bot merged commit ba1c87a into flutter:main Oct 4, 2022

cyanglaz deleted the textfield_crash branch October 4, 2022 22:57

thor-ola added a commit to olaparty/engine-builds that referenced this pull request Oct 5, 2022

patch flutter/engine#36295 to fix ios emoji text input

3366f46

engine-flutter-autoroll mentioned this pull request Oct 5, 2022

Roll Flutter Engine from 9692f09afd67 to 4d4914799b9a (16 revisions) flutter/flutter#112914

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 5, 2022

ba1c87a Workaround iOS text input crash for emoji+Korean text (flutte…

9787467

…r/engine#36295)

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Oct 5, 2022

Workaround iOS text input crash for emoji+Korean text (flutter#36295)

8ae7afd

cyanglaz mentioned this pull request Oct 5, 2022

[CP] Workaround iOS text input crash for emoji+Korean text #36621

Closed

8 tasks

jmagman pushed a commit to jmagman/engine that referenced this pull request Oct 17, 2022

Workaround iOS text input crash for emoji+Korean text (flutter#36295)

c0f0e36

jmagman mentioned this pull request Oct 17, 2022

[CP] Workaround iOS text input crash for emoji+Korean text #36807

Merged

8 tasks

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Oct 17, 2022

Workaround iOS text input crash for emoji+Korean text (flutter#36295)

c92f1ca

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Oct 17, 2022

Workaround iOS text input crash for emoji+Korean text (flutter#36295)

df7aa33

		@@ -1956,9 +1961,8 @@ - (void)deleteBackward {
		NSString* deletedText = [self.text substringWithRange:_selectedTextRange.range];
		CFRange range =

Conversation

cyanglaz commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

cyanglaz commented Sep 21, 2022

Uh oh!

jmagman commented Sep 21, 2022

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!

jmagman commented Sep 28, 2022

Uh oh!

jmagman commented Oct 4, 2022

Uh oh!

cyanglaz commented Oct 4, 2022

Uh oh!

LongCatIsLooong Oct 4, 2022 • 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

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyanglaz commented Sep 20, 2022 •

edited

Loading

LongCatIsLooong Oct 4, 2022 •

edited

Loading