From efb7bf434f2c993dc0f44a83fa13efcda127526a Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Wed, 25 Sep 2019 11:16:32 -0700 Subject: [PATCH] Work around Samsung keyboard issue (#12432) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 https://github.com/flutter/flutter/issues/29341#issuecomment-531283508. Fixes flutter/flutter#29341. --- shell/platform/android/BUILD.gn | 2 + .../plugin/editing/TextInputPlugin.java | 25 ++++- .../test/io/flutter/FlutterTestSuite.java | 2 + .../plugin/editing/TextInputPluginTest.java | 100 ++++++++++++++++++ 4 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java diff --git a/shell/platform/android/BUILD.gn b/shell/platform/android/BUILD.gn index 9151457d231..f2627ebcd11 100644 --- a/shell/platform/android/BUILD.gn +++ b/shell/platform/android/BUILD.gn @@ -419,6 +419,7 @@ action("robolectric_tests") { "test/io/flutter/embedding/engine/renderer/FlutterRendererTest.java", "test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java", "test/io/flutter/plugin/common/StandardMessageCodecTest.java", + "test/io/flutter/plugin/editing/TextInputPluginTest.java", "test/io/flutter/plugin/platform/SingleViewPresentationTest.java", "test/io/flutter/util/PreconditionsTest.java", ] @@ -437,6 +438,7 @@ action("robolectric_tests") { "//third_party/robolectric/lib/robolectric-3.8.jar", "//third_party/robolectric/lib/shadows-framework-3.8.jar", "//third_party/robolectric/lib/annotations-3.8.jar", + "//third_party/robolectric/lib/shadowapi-3.8.jar", "//third_party/robolectric/lib/runtime-1.1.1.jar", "//third_party/robolectric/lib/common-1.1.1.jar", "//third_party/robolectric/lib/common-java8-1.1.1.jar", diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index a3affe66c3a..36669ffdd0f 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -4,9 +4,12 @@ package io.flutter.plugin.editing; +import android.annotation.SuppressLint; import android.content.Context; +import android.os.Build; import android.support.annotation.NonNull; import android.support.annotation.Nullable; +import android.support.annotation.VisibleForTesting; import android.text.Editable; import android.text.InputType; import android.text.Selection; @@ -41,6 +44,7 @@ public class TextInputPlugin { private InputConnection lastInputConnection; @NonNull private PlatformViewsController platformViewsController; + private final boolean restartAlwaysRequired; // When true following calls to createInputConnection will return the cached lastInputConnection if the input // target is a platform view. See the comments on lockPlatformViewInputConnection for more details. @@ -86,6 +90,7 @@ public class TextInputPlugin { this.platformViewsController = platformViewsController; this.platformViewsController.attachTextInputPlugin(this); + restartAlwaysRequired = isRestartAlwaysRequired(); } @NonNull @@ -261,7 +266,7 @@ public class TextInputPlugin { mImm.hideSoftInputFromWindow(view.getApplicationWindowToken(), 0); } - private void setTextInputClient(int client, TextInputChannel.Configuration configuration) { + @VisibleForTesting void setTextInputClient(int client, TextInputChannel.Configuration configuration) { inputTarget = new InputTarget(InputTarget.Type.FRAMEWORK_CLIENT, client); this.configuration = configuration; mEditable = Editable.Factory.getInstance().newEditable(""); @@ -293,8 +298,8 @@ public class TextInputPlugin { } } - 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())) { applyStateToSelection(state); mImm.updateSelection(mView, Math.max(Selection.getSelectionStart(mEditable), 0), Math.max(Selection.getSelectionEnd(mEditable), 0), @@ -308,6 +313,20 @@ public class TextInputPlugin { } } + // 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. + // + // 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. + @SuppressLint("NewApi") // New API guard is inline, the linter can't see it. + private boolean isRestartAlwaysRequired() { + String language = (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) + ? mImm.getCurrentInputMethodSubtype().getLanguageTag() + : mImm.getCurrentInputMethodSubtype().getLocale(); + return Build.MANUFACTURER.equals("samsung") && language.equals("ko"); + } + private void clearTextInputClient() { if (inputTarget.type == InputTarget.Type.PLATFORM_VIEW) { // Focus changes in the framework tree have no guarantees on the order focus nodes are notified. A node diff --git a/shell/platform/android/test/io/flutter/FlutterTestSuite.java b/shell/platform/android/test/io/flutter/FlutterTestSuite.java index 0523a6b78b8..59980291db4 100644 --- a/shell/platform/android/test/io/flutter/FlutterTestSuite.java +++ b/shell/platform/android/test/io/flutter/FlutterTestSuite.java @@ -16,6 +16,7 @@ import io.flutter.embedding.engine.RenderingComponentTest; import io.flutter.embedding.engine.renderer.FlutterRendererTest; import io.flutter.embedding.engine.systemchannels.PlatformChannelTest; import io.flutter.plugin.common.StandardMessageCodecTest; +import io.flutter.plugin.editing.TextInputPluginTest; import io.flutter.plugin.platform.SingleViewPresentationTest; import io.flutter.util.PreconditionsTest; @@ -33,6 +34,7 @@ import io.flutter.util.PreconditionsTest; StandardMessageCodecTest.class, SingleViewPresentationTest.class, SmokeTest.class, + TextInputPluginTest.class, }) /** Runs all of the unit tests listed in the {@code @SuiteClasses} annotation. */ public class FlutterTestSuite { } diff --git a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java new file mode 100644 index 00000000000..dffc6a6dc08 --- /dev/null +++ b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java @@ -0,0 +1,100 @@ +package io.flutter.plugin.editing; + +import android.content.Context; +import android.util.SparseIntArray; +import android.view.View; +import android.view.inputmethod.InputMethodManager; +import android.view.inputmethod.InputMethodSubtype; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; +import org.robolectric.annotation.Config; +import org.robolectric.annotation.Implementation; +import org.robolectric.annotation.Implements; +import org.robolectric.shadow.api.Shadow; +import org.robolectric.shadows.ShadowBuild; +import org.robolectric.shadows.ShadowInputMethodManager; + +import io.flutter.embedding.engine.dart.DartExecutor; +import io.flutter.embedding.engine.systemchannels.TextInputChannel; +import io.flutter.plugin.platform.PlatformViewsController; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; + +@Config(manifest = Config.NONE, shadows = TextInputPluginTest.TestImm.class) +@RunWith(RobolectricTestRunner.class) +public class TextInputPluginTest { + @Test + public void setTextInputEditingState_doesNotRestartWhenTextIsIdentical() { + // Initialize a general TextInputPlugin. + InputMethodSubtype inputMethodSubtype = mock(InputMethodSubtype.class); + TestImm testImm = Shadow.extract(RuntimeEnvironment.application.getSystemService(Context.INPUT_METHOD_SERVICE)); + testImm.setCurrentInputMethodSubtype(inputMethodSubtype); + View testView = new View(RuntimeEnvironment.application); + TextInputPlugin textInputPlugin = new TextInputPlugin(testView, mock(DartExecutor.class), mock(PlatformViewsController.class)); + textInputPlugin.setTextInputClient(0, new TextInputChannel.Configuration(false, false, TextInputChannel.TextCapitalization.NONE, null, null, null)); + // There's a pending restart since we initialized the text input client. Flush that now. + textInputPlugin.setTextInputEditingState(testView, new TextInputChannel.TextEditState("", 0, 0)); + + // Move the cursor. + assertEquals(1, testImm.getRestartCount(testView)); + textInputPlugin.setTextInputEditingState(testView, new TextInputChannel.TextEditState("", 0, 0)); + + // Verify that we haven't restarted the input. + assertEquals(1, testImm.getRestartCount(testView)); + } + + // See https://github.com/flutter/flutter/issues/29341 + @Test + public void setTextInputEditingState_alwaysRestartsOnAffectedDevices() { + // Initialize a TextInputPlugin that needs to be always restarted. + ShadowBuild.setManufacturer("samsung"); + InputMethodSubtype inputMethodSubtype = new InputMethodSubtype(0, 0, /*locale=*/"ko", "", "", false, false); + TestImm testImm = Shadow.extract(RuntimeEnvironment.application.getSystemService(Context.INPUT_METHOD_SERVICE)); + testImm.setCurrentInputMethodSubtype(inputMethodSubtype); + View testView = new View(RuntimeEnvironment.application); + TextInputPlugin textInputPlugin = new TextInputPlugin(testView, mock(DartExecutor.class), mock(PlatformViewsController.class)); + textInputPlugin.setTextInputClient(0, new TextInputChannel.Configuration(false, false, TextInputChannel.TextCapitalization.NONE, null, null, null)); + // There's a pending restart since we initialized the text input client. Flush that now. + textInputPlugin.setTextInputEditingState(testView, new TextInputChannel.TextEditState("", 0, 0)); + + // Move the cursor. + assertEquals(1, testImm.getRestartCount(testView)); + textInputPlugin.setTextInputEditingState(testView, new TextInputChannel.TextEditState("", 0, 0)); + + // Verify that we've restarted the input. + assertEquals(2, testImm.getRestartCount(testView)); + } + + @Implements(InputMethodManager.class) + public static class TestImm extends ShadowInputMethodManager { + private InputMethodSubtype currentInputMethodSubtype; + private SparseIntArray restartCounter = new SparseIntArray(); + + public TestImm() { + } + + @Implementation + public InputMethodSubtype getCurrentInputMethodSubtype() { + return currentInputMethodSubtype; + } + + @Implementation + public void restartInput(View view) { + int count = restartCounter.get(view.hashCode(), /*defaultValue=*/0) + 1; + restartCounter.put(view.hashCode(), count); + } + + public void setCurrentInputMethodSubtype(InputMethodSubtype inputMethodSubtype) { + this.currentInputMethodSubtype = inputMethodSubtype; + } + + public int getRestartCount(View view) { + return restartCounter.get(view.hashCode(), /*defaultValue=*/0); + } + } +} +