Fix deleting text when the last character is some special characters on IOS #7982

HelloCore · 2019-02-27T09:29:33Z

On iOS, we cannot delete last character if it is some special characters, for example, ่, ี, ุ, or ឹ

As you can see here.
flutter/flutter#24203
flutter/flutter#21745

After I debugged it, I found that _selectedTextRange.isEmpty happened to be true for this specific case.
Morover, I found that it called - (void)setSelectedTextRange:(UITextRange*)selectedTextRange before - (void)deleteBackward on the normal case, but it wasn't called on this case.

… > 0

…rd-special-character

sittipongsaladaeng · 2019-02-28T07:07:43Z

@GaryQian Please solve this issue.

SupakornTeam · 2019-02-28T08:08:42Z

@GaryQian
We want to edit this part. Please solve this issue.

…racter Rewrite logic

googlebot · 2019-02-28T15:01:40Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-02-28T15:01:43Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

HelloCore · 2019-02-28T15:07:23Z

She’s signed it.

googlebot · 2019-02-28T15:07:36Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-02-28T15:07:36Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

MechinMaru · 2019-02-28T15:13:27Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

I'm OK.

GaryQian · 2019-02-28T22:21:50Z

@MechinMaru Hey, to satisfy the CLA bots, could you make sure you have signed (https://cla.developers.google.com/clas) and comment I signed it! on this PR? Thanks!

MechinMaru · 2019-03-01T01:19:15Z

I signed it!

MechinMaru · 2019-03-01T01:44:48Z

I signed it at Feb 28, 2019 07:03 PST.
I think bot confused about this.

Update FlutterTextInputPlugin.mm

GaryQian · 2019-03-01T18:30:35Z

Ok, have verified everything is signed, and you have commented that you are ok with contributing this code to Flutter.

I'll take a look at this soon too. cc @cbracken for some Obj-C magic review

googlebot · 2019-03-01T18:36:52Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-01T18:36:52Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

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

googlebot · 2019-03-06T22:19:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-06T22:19:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-06T22:20:14Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-06T22:20:15Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-07T04:59:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-07T04:59:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

HelloCore · 2019-03-07T12:09:01Z

Thank you for reviewing.

As I investigated, I found that UIKeyboardImpl called

- (UITextPosition*)positionFromPosition:(UITextPosition*)position offset:(NSInteger)offset with offset is -1
- (UITextRange*)textRangeFromPosition:(UITextPosition*)fromPosition toPosition:(UITextPosition*)toPosition
- (void)setSelectedTextRange:(UITextRange*)selectedTextRange with new textRange
- (void)deleteBackward

However, in case of Thai vowel, it skipped - (void)setSelectedTextRange:, so _selectedTextRange is going to be empty.
Moreover, when I modified - (UITextPosition*)positionFromPosition to return previous codepoint, I found that it happened to call - (void)setSelectedTextRange: correctly.

So, I think UIKeyboardImpl did some magical things that made them be able to distinguish between grapheme cluster that had to be completely deleted, and partly deleted.

I think it's not going to break current behavior because it called - (void)setSelectedTextRange:, so _selectedTextRange wouldn't be empty for other cases.

GaryQian

Well, this LGTM!

Thanks for researching and fixing this bug!

I'll leave this open for a bit to see if @cbracken has any final concerns, and ill look for an opportunity to land it!

(I'll manually change the CLA tag again just before landing to avoid more clabot spam)

googlebot · 2019-03-08T18:46:49Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-08T18:46:51Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

…racters on IOS (flutter/engine#7982)

flutter/engine@6031407...87edd94 git log 6031407..87edd94 --no-merges --oneline 87edd94 Add read-only persistent cache (flutter/engine#8049) 4c94049 Move android_sdk_downloader so I can more easily deprecate it (flutter/engine#8084) 7cbd9d8 Roll src/third_party/skia 3d1b941f3a7d..bea1f94f341e (7 commits) (flutter/engine#8083) 260669c Fix deleting text when the last character is some special characters on IOS (flutter/engine#7982) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (mklim@google.com), and stop the roller if necessary.

HelloCore added 3 commits February 26, 2019 18:18

Handle deleteBackward when _selectedTextRange isEmpty but text.length…

7daf6a0

… > 0

Format file

6616cc1

Merge remote-tracking branch 'upstream/master' into fix/delete-backwa…

66220ea

…rd-special-character

googlebot added the cla: yes label Feb 27, 2019

MechinMaru and others added 2 commits February 28, 2019 21:54

Rewrite logic

ba85b8f

Merge pull request #3 from MechinMaru/fix/delete-backward-special-cha…

117ca27

…racter Rewrite logic

googlebot added cla: no and removed cla: yes labels Feb 28, 2019

HelloCore added 4 commits March 1, 2019 10:42

Update FlutterTextInputPlugin.mm

d80e968

Merge pull request #4 from HelloCore/HelloCore-patch-1

34ab641

Update FlutterTextInputPlugin.mm

Optimize logic

96b9516

Merge condition and simplify logic

7315a77

GaryQian requested a review from cbracken March 1, 2019 18:30

GaryQian added cla: yes and removed cla: no labels Mar 1, 2019

GaryQian reviewed Mar 4, 2019

View reviewed changes

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

Add details about Thai input to backspace handling

42f4ef8

googlebot added cla: no and removed cla: yes labels Mar 6, 2019

cbracken added cla: yes and removed cla: no labels Mar 6, 2019

Add a comment and links.

70816ec

googlebot added cla: no and removed cla: yes labels Mar 7, 2019

GaryQian approved these changes Mar 7, 2019

View reviewed changes

GaryQian added cla: yes and removed cla: no labels Mar 8, 2019

GaryQian approved these changes Mar 8, 2019

View reviewed changes

GaryQian merged commit 260669c into flutter:master Mar 8, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2019

260669c Fix deleting text when the last character is some special cha…

fc03286

…racters on IOS (flutter/engine#7982)

engine-flutter-autoroll mentioned this pull request Mar 8, 2019

Roll engine 60314077846c..260669c65e9e (1 commits) flutter/flutter#29060

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 8, 2019

260669c Fix deleting text when the last character is some special cha…

6994fc1

…racters on IOS (flutter/engine#7982)

engine-flutter-autoroll mentioned this pull request Mar 8, 2019

Roll engine 60314077846c..87edd94e9cc7 (4 commits) flutter/flutter#29067

Merged

HelloCore mentioned this pull request Aug 31, 2019

Fix deleting Thai vowel bug on iOS #11807

Merged

jmagman mentioned this pull request Jul 8, 2022

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

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deleting text when the last character is some special characters on IOS #7982

Fix deleting text when the last character is some special characters on IOS #7982

HelloCore commented Feb 27, 2019

sittipongsaladaeng commented Feb 28, 2019

SupakornTeam commented Feb 28, 2019

googlebot commented Feb 28, 2019

googlebot commented Feb 28, 2019

HelloCore commented Feb 28, 2019

googlebot commented Feb 28, 2019

googlebot commented Feb 28, 2019

MechinMaru commented Feb 28, 2019

GaryQian commented Feb 28, 2019 •

edited

Loading

MechinMaru commented Mar 1, 2019

MechinMaru commented Mar 1, 2019

GaryQian commented Mar 1, 2019

googlebot commented Mar 1, 2019

googlebot commented Mar 1, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 7, 2019

googlebot commented Mar 7, 2019

HelloCore commented Mar 7, 2019

GaryQian left a comment •

edited

Loading

googlebot commented Mar 8, 2019

googlebot commented Mar 8, 2019

Fix deleting text when the last character is some special characters on IOS #7982

Fix deleting text when the last character is some special characters on IOS #7982

Conversation

HelloCore commented Feb 27, 2019

sittipongsaladaeng commented Feb 28, 2019

SupakornTeam commented Feb 28, 2019

googlebot commented Feb 28, 2019

googlebot commented Feb 28, 2019

HelloCore commented Feb 28, 2019

googlebot commented Feb 28, 2019

googlebot commented Feb 28, 2019

MechinMaru commented Feb 28, 2019

GaryQian commented Feb 28, 2019 • edited Loading

MechinMaru commented Mar 1, 2019

MechinMaru commented Mar 1, 2019

GaryQian commented Mar 1, 2019

googlebot commented Mar 1, 2019

googlebot commented Mar 1, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 6, 2019

googlebot commented Mar 7, 2019

googlebot commented Mar 7, 2019

HelloCore commented Mar 7, 2019

GaryQian left a comment • edited Loading

Choose a reason for hiding this comment

googlebot commented Mar 8, 2019

googlebot commented Mar 8, 2019

GaryQian commented Feb 28, 2019 •

edited

Loading

GaryQian left a comment •

edited

Loading