Work around Samsung keyboard issue #12432

mklim · 2019-09-24T22:02:17Z

Samsung's Korean keyboard has a bug where it always attempts to combine
characters based on its internal state, ignoring if and when the cursor
is moved programmatically. EG typing "ㄴㅇ" and then moving the cursor
back to the front of the text and typing "ㄴ" again would result in
"ㄴㅇㄴ", not "ㄴㄴㅇ".

Fully restarting the IMM works around this because it flushes the
keyboard's internal state and stops it from trying to incorrectly
combine characters. However this also has some negative performance
implications, so we only apply the workaround on Samsung devices set
to use Korean input.

This also effectively disables the feature on Samsung keyboards that
allowed users to re-open a composing region for previously typed
characters. See flutter/flutter#29341 (comment).

Fixes flutter/flutter#29341.

Samsung's Korean keyboard has a bug where it always attempts to combine characters based on its internal state, ignoring if and when the cursor is moved programmatically. EG typing "ㄴㅇ" and then moving the cursor back to the front of the text and typing "ㄴ" again would result in "ㄴㅇㄴ", not "ㄴㄴㅇ". Fully restarting the IMM works around this because it flushes the keyboard's internal state and stops it from trying to incorrectly combine characters. However this also has some negative performance implications, so we don't want to apply this workaround in every case.

justinmc

Thank you so much for this fix, it has been such a pain 😭

I think this is a pretty reasonable compromise. I was afraid I might have ended up hacking something much more sketchy!

It's important to note that with this fix, Flutter will not be able to support Samsung's ability to reopen composing regions. This is the feature that Samsung added to their keyboard to cause this bug in the first place. I explained in detail what the feature is in flutter/flutter#29341 (comment). It strikes me as a subtle thing that many users won't notice, but we'll have to wait and see if it ends up being a big deal for Korean users.

justinmc · 2019-09-24T22:58:59Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

-    private void setTextInputEditingState(View view, TextInputChannel.TextEditState state) {
-        if (!mRestartInputPending && state.text.equals(mEditable.toString())) {
+    @VisibleForTesting void setTextInputEditingState(View view, TextInputChannel.TextEditState state) {
+        if (!restartAlwaysRequired && !mRestartInputPending && state.text.equals(mEditable.toString())) {


Are there any cases where this will cause the IMM to restart besides tapping to move the cursor? I wonder if there are any optimizations we can make by not restarting in those cases if so. Not sure though, I'll have to play around with the device tomorrow.

Good question. From reading through the code and manually testing it out it looked like just tapping to move, but I'm not totally convinced that I'm seeing the whole situation here. @jason-simmons do you know?

The engine asks the IMM to restart input when the app switches to a new text editing client or when the app sends an update to editor state originating from the Dart side.

In the latter case it's possible that we could diff the current and new state and do something less expensive than a full restart if the diff is simple enough. But AFAIK this hasn't been noticeable.

It seems fine to me as-is after playing around with it. I only see it restart when the user pastes into the input, but that may be necessary, and it shouldn't be a performance problem anyway.

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

GaryQian · 2019-09-24T23:13:07Z

I'll ask @jayoung-lee for an opinion on if the lack of capability to reopen composing regions is significant or not.

justinmc

LGTM

After playing around with it, it seems to still work with the "reopening" feature! It seems identical to the native input now as far as I can tell. I tried it on an old unaffected Samsung device, and the behavior was the same as native on that device (no composing region reopening). When I tried it on the new affected Samsung device, the behavior was also the same as native, and composing reopening worked.

Seems perfect to me!

justinmc · 2019-09-25T17:18:50Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

-    private void setTextInputEditingState(View view, TextInputChannel.TextEditState state) {
-        if (!mRestartInputPending && state.text.equals(mEditable.toString())) {
+    @VisibleForTesting void setTextInputEditingState(View view, TextInputChannel.TextEditState state) {
+        if (!restartAlwaysRequired && !mRestartInputPending && state.text.equals(mEditable.toString())) {


It seems fine to me as-is after playing around with it. I only see it restart when the user pastes into the input, but that may be necessary, and it shouldn't be a performance problem anyway.

GaryQian

LGTM from my end. We should keep an eye out to see if that regression we saw shows itself again, but it looks like it may have been an artifact of a different version. Thanks!

jayoung-lee · 2019-09-25T18:16:38Z

I helped @justinmc play around with the new keyboard, and yes, it seemed to work perfectly fine!

Please let me know if anything new comes up in the future, I will be more than happy to help you test it :)

mklim · 2019-09-25T18:16:57Z

Thank you @jayoung-lee!

…1318) git@github.com:flutter/engine.git/compare/0bfca375b3f5...efb7bf4 git log 0bfca37..efb7bf4 --no-merges --oneline 2019-09-25 mklim@google.com Work around Samsung keyboard issue (flutter/engine#12432) 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 aaclarke@google.com on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

futterer · 2019-09-25T21:23:32Z

Might this be related to flutter/flutter#41315 ?

markmoskalenko · 2019-09-26T08:07:13Z

Included in the assembly of the Russian language!
Similar problem

…utter#41318) git@github.com:flutter/engine.git/compare/0bfca375b3f5...efb7bf4 git log 0bfca37..efb7bf4 --no-merges --oneline 2019-09-25 mklim@google.com Work around Samsung keyboard issue (flutter/engine#12432) 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 aaclarke@google.com on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

Michael Klimushyn added 2 commits September 24, 2019 13:52

Add unit test.

2483f5c

mklim requested review from GaryQian, jason-simmons and justinmc September 24, 2019 22:02

googlebot added the cla: yes label Sep 24, 2019

mklim changed the title ~~Workaround Samsung keyboard issue~~ Work around Samsung keyboard issue Sep 24, 2019

Michael Klimushyn added 2 commits September 24, 2019 15:28

Fix lints.

16ff5bd

Remove unicode characters from source code comment to fix CI.

a4fd71f

justinmc reviewed Sep 24, 2019

View reviewed changes

GaryQian reviewed Sep 24, 2019

View reviewed changes

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java Show resolved Hide resolved

justinmc approved these changes Sep 25, 2019

View reviewed changes

GaryQian mentioned this pull request Sep 25, 2019

Samsung Keyboard duplicates text when restoring composing regions (restart text input, punctuation) flutter/flutter#31512

Closed

GaryQian approved these changes Sep 25, 2019

View reviewed changes

mklim merged commit efb7bf4 into flutter:master Sep 25, 2019

mklim deleted the korean_text_bugfix branch September 25, 2019 18:16

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

efb7bf4 Work around Samsung keyboard issue (flutter/engine#12432)

9cedca3

engine-flutter-autoroll mentioned this pull request Sep 25, 2019

Roll engine 0bfca375b3f5..efb7bf434f2c (1 commits) flutter/flutter#41318

Merged

mklim mentioned this pull request Sep 25, 2019

Textinput bug on Samsung device flutter/flutter#41315

Closed

yutae pushed a commit to yutae/engine that referenced this pull request Sep 26, 2019

Work around Samsung keyboard issue (flutter#12432)

b2a9fa2

seritaApp mentioned this pull request Sep 28, 2019

Not reproducible on Tab S4 justinmc/android-samsung-keyboard-bug-demo#1

Open

GaryQian mentioned this pull request Oct 3, 2019

Restart all modern Samsung keyboard IMM #12780

Merged

justinmc mentioned this pull request Jan 28, 2021

Holding backspace in textfield stops sporadically on Samsung devices flutter/flutter#73433

Closed

Work around Samsung keyboard issue #12432

Work around Samsung keyboard issue #12432

Uh oh!

Conversation

mklim commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Sep 24, 2019 • 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!

jason-simmons Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GaryQian commented Sep 24, 2019

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

GaryQian left a comment

Choose a reason for hiding this comment

Uh oh!

jayoung-lee commented Sep 25, 2019

Uh oh!

mklim commented Sep 25, 2019

Uh oh!

futterer commented Sep 25, 2019

Uh oh!

markmoskalenko commented Sep 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mklim commented Sep 24, 2019 •

edited

Loading

mklim Sep 24, 2019 •

edited

Loading