From 1134c9e7c557586d0a21a1b758c480532cbd12db Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Tue, 24 Sep 2019 13:36:24 -0700 Subject: [PATCH 1/4] Workaround Samsung keyboard issue 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 don't want to apply this workaround in every case. --- .../plugin/editing/TextInputPlugin.java | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index a3affe66c3a3d..88b608d4dda3c 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -5,6 +5,7 @@ package io.flutter.plugin.editing; import android.content.Context; +import android.os.Build; import android.support.annotation.NonNull; import android.support.annotation.Nullable; import android.text.Editable; @@ -41,6 +42,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 +88,7 @@ public void clearClient() { this.platformViewsController = platformViewsController; this.platformViewsController.attachTextInputPlugin(this); + restartAlwaysRequired = isRestartAlwaysRequired(); } @NonNull @@ -294,7 +297,7 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) { } private void setTextInputEditingState(View view, TextInputChannel.TextEditState state) { - if (!mRestartInputPending && state.text.equals(mEditable.toString())) { + 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 +311,21 @@ private void setTextInputEditingState(View view, TextInputChannel.TextEditState } } + // 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 "ㄴㄴㅇ". https://github.com/flutter/flutter/issues/29341 + // + // 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. + 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 From 2483f5c2281180dccf0bfff98094abb1556c994a Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Tue, 24 Sep 2019 14:45:48 -0700 Subject: [PATCH 2/4] Add unit test. --- shell/platform/android/BUILD.gn | 2 + .../plugin/editing/TextInputPlugin.java | 5 +- .../test/io/flutter/FlutterTestSuite.java | 2 + .../plugin/editing/TextInputPluginTest.java | 102 ++++++++++++++++++ 4 files changed, 109 insertions(+), 2 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 9151457d23100..f2627ebcd112a 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 88b608d4dda3c..f5deb1b0cbdce 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -8,6 +8,7 @@ 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; @@ -264,7 +265,7 @@ private void hideTextInput(View view) { 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(""); @@ -296,7 +297,7 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) { } } - private void setTextInputEditingState(View view, TextInputChannel.TextEditState state) { + @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), diff --git a/shell/platform/android/test/io/flutter/FlutterTestSuite.java b/shell/platform/android/test/io/flutter/FlutterTestSuite.java index 0523a6b78b86a..59980291db489 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.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 @@ 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 0000000000000..100d9c33d465b --- /dev/null +++ b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java @@ -0,0 +1,102 @@ +package io.flutter.plugin.editing; + +import android.content.Context; +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 java.util.HashMap; +import java.util.Map; + +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 Map restartCounter = new HashMap<>(); + + public TestImm() { + } + + @Implementation + public InputMethodSubtype getCurrentInputMethodSubtype() { + return currentInputMethodSubtype; + } + + @Implementation + public void restartInput(View view) { + int count = restartCounter.getOrDefault(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.getOrDefault(view.hashCode(), /*defaultValue=*/0); + } + } +} + From 16ff5bd119c2193b8105265f7551f9990e8edbef Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Tue, 24 Sep 2019 15:28:17 -0700 Subject: [PATCH 3/4] Fix lints. --- .../io/flutter/plugin/editing/TextInputPlugin.java | 2 ++ .../io/flutter/plugin/editing/TextInputPluginTest.java | 10 ++++------ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index f5deb1b0cbdce..0f1821e941b1f 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -4,6 +4,7 @@ package io.flutter.plugin.editing; +import android.annotation.SuppressLint; import android.content.Context; import android.os.Build; import android.support.annotation.NonNull; @@ -320,6 +321,7 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) { // 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() diff --git a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java index 100d9c33d465b..dffc6a6dc0820 100644 --- a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java +++ b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java @@ -1,6 +1,7 @@ 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; @@ -16,9 +17,6 @@ import org.robolectric.shadows.ShadowBuild; import org.robolectric.shadows.ShadowInputMethodManager; -import java.util.HashMap; -import java.util.Map; - import io.flutter.embedding.engine.dart.DartExecutor; import io.flutter.embedding.engine.systemchannels.TextInputChannel; import io.flutter.plugin.platform.PlatformViewsController; @@ -74,7 +72,7 @@ public void setTextInputEditingState_alwaysRestartsOnAffectedDevices() { @Implements(InputMethodManager.class) public static class TestImm extends ShadowInputMethodManager { private InputMethodSubtype currentInputMethodSubtype; - private Map restartCounter = new HashMap<>(); + private SparseIntArray restartCounter = new SparseIntArray(); public TestImm() { } @@ -86,7 +84,7 @@ public InputMethodSubtype getCurrentInputMethodSubtype() { @Implementation public void restartInput(View view) { - int count = restartCounter.getOrDefault(view.hashCode(), /*defaultValue=*/0) + 1; + int count = restartCounter.get(view.hashCode(), /*defaultValue=*/0) + 1; restartCounter.put(view.hashCode(), count); } @@ -95,7 +93,7 @@ public void setCurrentInputMethodSubtype(InputMethodSubtype inputMethodSubtype) } public int getRestartCount(View view) { - return restartCounter.getOrDefault(view.hashCode(), /*defaultValue=*/0); + return restartCounter.get(view.hashCode(), /*defaultValue=*/0); } } } From a4fd71ff9974448026e305e32c5d83cde1b5c5b0 Mon Sep 17 00:00:00 2001 From: Michael Klimushyn Date: Tue, 24 Sep 2019 15:43:26 -0700 Subject: [PATCH 4/4] Remove unicode characters from source code comment to fix CI. --- .../android/io/flutter/plugin/editing/TextInputPlugin.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index 0f1821e941b1f..36669ffdd0ff0 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -314,9 +314,7 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) { } // 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 "ㄴㄴㅇ". https://github.com/flutter/flutter/issues/29341 + // 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