Fix an issue that deleting an emoji may crash the app #34508

yeatse · 2022-07-07T18:12:41Z

An emoji may consist of two or more characters. We should make sure that deleting backward should remove the entire emoji.

Fixes flutter/flutter#106933

This issue was introduced in #32223

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/FlutterTextInputPluginTest.mm

yeatse · 2022-07-18T02:02:33Z

Kindly ping @jmagman for another look😀

dnfield · 2022-07-21T22:36:22Z

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

+                                      options:kNilOptions
+                                        range:charRange
+                               remainingRange:NULL];
+      if (gotCodePoint && u_hasBinaryProperty(codePoint, UCHAR_EMOJI)) {


Why is this getting applied only to emoji?

Aren't there other multi-byte characters that are not emoji that we'd want to apply this to?

From #34508 (comment), languages with diacritical marks will expect a backward deletion should only delete one part of the composed character, such as Thai and Hebrew.

We have two ways to distinguish this behavior from emoji's:

check if the byte is a diacritical mark (and other properties I don't know yet)

check if the byte is not in an emoji

I chose the second one to keep consistency with android (see FlutterTextUtils.getOffsetBefore)

Ahhh ok this makes sense.

jmagman · 2022-07-23T01:33:55Z

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

@@ -7,6 +7,8 @@
 #import <Foundation/Foundation.h>
 #import <UIKit/UIKit.h>

+#include "unicode/uchar.h"


@dnfield since you jumped in, how do you feel about including this library? I've been trying to think of another way to check if it's an emoji without importing this but also reinventing the wheel. I guess it's okay?

This seems fine to me. This is what ICU is for and there's no getting around having ICU in a flutter app

jmagman · 2022-07-23T02:28:59Z

testing/ios/IosUnitTests/IosUnitTests.xcodeproj/xcshareddata/xcschemes/IosUnitTests.xcscheme

+            argument = "--icu-data-file-path=Frameworks/Flutter.framework/icudtl.dat"
+            isEnabled = "YES">


Is this needed?

Yes. Without this the test will fail due to failing to load the ICU data file. Maybe we should file another issue to get rid of this later.

Filed flutter/flutter#108221

I think this is fine - in other tests we use the command line argument to load the ICU data file (see run_tests.py).

…ginTest.mm Co-authored-by: Jenn Magder <[email protected]>

jmagman

LGTM, thanks for the contribution and adding the backfilled tests!

jmagman · 2022-07-25T16:42:33Z

@dnfield can you give the second hacker review?

dnfield

LGTM, as a hacker

…r/engine#34508)

* 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 <[email protected]> Co-authored-by: Chris Yang <[email protected]>

flutter-dashboard bot added platform-android platform-ios platform-linux labels Jul 7, 2022

yeatse marked this pull request as ready for review July 7, 2022 18:57

yeatse mentioned this pull request Jul 8, 2022

[ios]: Trying to delete emoji crashes the app and throws Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Invalid JSON message, encoding failed' error flutter/flutter#106933

Closed

jmagman reviewed Jul 8, 2022

View reviewed changes

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

zanderso requested a review from jmagman July 21, 2022 15:32

dnfield reviewed Jul 21, 2022

View reviewed changes

jmagman reviewed Jul 23, 2022

View reviewed changes

yeatse mentioned this pull request Jul 23, 2022

Flutter engine cannot locate ICU data file correctly in IosUnitTests flutter/flutter#108221

Closed

yeatse and others added 6 commits July 23, 2022 14:53

fix a problem that deleting an emoji crashes the app

6c1d6cc

Add tests

6f604cb

Update shell/platform/darwin/ios/framework/Source/FlutterTextInputPlu…

e8c4a37

…ginTest.mm Co-authored-by: Jenn Magder <[email protected]>

remove composed characters only if they are emojis

a28afd0

update comments

8e59927

update test case to cover the leading thai vowel

1382921

jmagman approved these changes Jul 25, 2022

View reviewed changes

dnfield approved these changes Jul 25, 2022

View reviewed changes

jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 25, 2022

auto-submit bot merged commit 7050725 into flutter:main Jul 25, 2022

yeatse deleted the fix-delete-emoji branch July 25, 2022 17:23

engine-flutter-autoroll mentioned this pull request Jul 25, 2022

Roll Flutter Engine from 54b0ac3059bf to 705072522c00 (4 revisions) flutter/flutter#108308

Merged

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

7050725 Fix an issue that deleting an emoji may crash the app (flutte…

f2f64f7

…r/engine#34508)

jmagman mentioned this pull request Sep 21, 2022

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

Closed

cyanglaz mentioned this pull request Sep 21, 2022

Workaround iOS text input crash for emoji+Korean text #36295

Merged

8 tasks

jmagman mentioned this pull request Oct 11, 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

Fix an issue that deleting an emoji may crash the app (flutter#34508)

cf9b00f

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

Fix an issue that deleting an emoji may crash the app (flutter#34508)

db5d3ef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix an issue that deleting an emoji may crash the app #34508

Fix an issue that deleting an emoji may crash the app #34508

Uh oh!

yeatse commented Jul 7, 2022 •

edited

Loading

Uh oh!

Uh oh!

yeatse commented Jul 18, 2022 •

edited

Loading

Uh oh!

dnfield Jul 21, 2022

Uh oh!

yeatse Jul 22, 2022

Uh oh!

dnfield Jul 22, 2022

Uh oh!

jmagman Jul 23, 2022

Uh oh!

dnfield Jul 23, 2022

Uh oh!

jmagman Jul 23, 2022

Uh oh!

yeatse Jul 23, 2022 •

edited

Loading

Uh oh!

yeatse Jul 23, 2022

Uh oh!

dnfield Jul 25, 2022

Uh oh!

jmagman left a comment

Uh oh!

jmagman commented Jul 25, 2022

Uh oh!

dnfield left a comment

Uh oh!

Uh oh!

		argument = "--icu-data-file-path=Frameworks/Flutter.framework/icudtl.dat"
		isEnabled = "YES">

Fix an issue that deleting an emoji may crash the app #34508

Fix an issue that deleting an emoji may crash the app #34508

Uh oh!

Conversation

yeatse commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

yeatse commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

yeatse Jul 23, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman commented Jul 25, 2022

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yeatse commented Jul 7, 2022 •

edited

Loading

yeatse commented Jul 18, 2022 •

edited

Loading

yeatse Jul 23, 2022 •

edited

Loading