check if we can avoid restarting input when tapping outside composition |
||||||
Issue descriptionVersion: 51 OS: Android What steps will reproduce the problem? (1) Compose some text in multi line input (e.g. textarea) (2) Tap outside the current composition. What is the expected output? Do not call restartInput if possible as it is quite a costly operation. What do you see instead? Chrome/WebView calls InputMethodManager#restartInput(). (Originally filed as b/30025594 and b/30181268.) This logic was originally introduced more than three years ago in issue 164427 and https://bugs.webkit.org/show_bug.cgi?id=107737 . In some versions of Google IME, we found that restartInput() causes word duplication and sudden jumps, and restartInput() is quite an expensive operation so we want to avoid it as much as possible. The current logic leaves some room for corner cases, because it allows subsequent changes from keyboard apps to take effect before restartInput() kicks in, although it is very difficult to reproduce such a problem. Most keyboard apps finishes the current composition when there is an unexpected selection change from the editor (e.g. user taps on some other area). If that is the case, then we don't need to cancel/finish composition ourselves, and just leave conflict handling to the keyboard app. I need to investigate more about its impact on other platforms.
,
Sep 27 2016
Although it is not explicitly stated in the spec, it seems that Android input method apps should finish composition when selection changes unexpectedly. Android's SoftKeyboard sample app has such logic as well: http://androidxref.com/6.0.1_r10/xref/development/samples /SoftKeyboard/src/com/example/android/softkeyboard/ SoftKeyboard.java @Override public void onUpdateSelection(int oldSelStart, int oldSelEnd, int newSelStart, int newSelEnd, int candidatesStart, int candidatesEnd) { super.onUpdateSelection(oldSelStart, oldSelEnd, newSelStart, newSelEnd, candidatesStart, candidatesEnd); // If the current selection in the text view changes, we should // clear whatever candidate text we have. if (mComposing.length() > 0 && (newSelStart != candidatesEnd || newSelEnd != candidatesEnd)) { mComposing.setLength(0); updateCandidates(); InputConnection ic = getCurrentInputConnection(); if (ic != null) { ic.finishComposingText(); } }
,
Sep 27 2016
I just went ahead and built SoftKeyboard without overriding onUpdateSelection, and tested on EditText (e.g. Google Search App). If you tap outside the current composition, the composition remains on the screen. This proves my suspicion in comment #2. FYI, I have a working CL that removes input restarting behavior: https://codereview.chromium.org/2370663002/ Please feel free to review and comment.
,
Nov 21 2016
FYI, this is now blocked by a test failure which seems to have the same root cause as issue 664317.
,
Dec 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2418e1b631787f3a9fed9a6aad0d53e11e467e5b commit 2418e1b631787f3a9fed9a6aad0d53e11e467e5b Author: changwan <changwan@chromium.org> Date: Mon Dec 12 12:43:08 2016 Move call path for resetInputMethod This is mostly about mechanical changes such as: - Move InputMethodController-related logic inside InputMethodController and call it first - such that we can expand the logic there. - Rename method name for better readability. Also, there are subtle logical changes: 1) Try finishComposingText() even when the current input type is NONE. This should not have any effect. 2) When finishComposingText() returns false, we no longer call UpdateTextInputState(). This should not have any effect. 3) When finishComposingText() returns false, we no long call UpdateCompositionInfo(). This is used by Android and Mac. Previously, there was a chance that we propagate InvalidRange() all the way up to the keyboard app. Now ShouldUpdateCompositionInfo() returns false to make it clear. This is originally found in resolving crbug.com/664317, but is also needed for crbug.com/630137 because if we no longer cancel composition on selection change, then WebViewTest may fail because it uses TestWebViewClient which does not implement resetInputMethod(). BUG=664317, 630137 Review-Url: https://codereview.chromium.org/2561543003 Cr-Commit-Position: refs/heads/master@{#437854} [modify] https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b/content/renderer/render_widget.cc [modify] https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b/third_party/WebKit/Source/core/editing/InputMethodController.cpp [modify] https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b/third_party/WebKit/Source/core/editing/InputMethodController.h [modify] https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b/third_party/WebKit/Source/core/page/ChromeClient.h [modify] https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b/third_party/WebKit/Source/core/page/FocusController.cpp [modify] https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b/third_party/WebKit/Source/web/ChromeClientImpl.cpp [modify] https://crrev.com/2418e1b631787f3a9fed9a6aad0d53e11e467e5b/third_party/WebKit/Source/web/ChromeClientImpl.h
,
Jan 24 2017
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29810350d256c855e8d1af4c715b28da3a992336 commit 29810350d256c855e8d1af4c715b28da3a992336 Author: changwan <changwan@chromium.org> Date: Wed Feb 08 02:51:24 2017 Stop dismissing selection handles when selection is kept When we finish composing text and keep selection, selection handles get dismissed because of intermediate selection changes, even though the final selection will not change. BUG= 630137 Review-Url: https://codereview.chromium.org/2646963002 Cr-Commit-Position: refs/heads/master@{#448875} [modify] https://crrev.com/29810350d256c855e8d1af4c715b28da3a992336/third_party/WebKit/Source/core/editing/InputMethodController.cpp [modify] https://crrev.com/29810350d256c855e8d1af4c715b28da3a992336/third_party/WebKit/Source/core/editing/InputMethodController.h [modify] https://crrev.com/29810350d256c855e8d1af4c715b28da3a992336/third_party/WebKit/Source/web/tests/WebViewTest.cpp [modify] https://crrev.com/29810350d256c855e8d1af4c715b28da3a992336/third_party/WebKit/Source/web/tests/data/longpress_selection.html
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50a098f28262dca3de4fa9ff3b26f7a14f5d582a commit 50a098f28262dca3de4fa9ff3b26f7a14f5d582a Author: changwan <changwan@chromium.org> Date: Wed Feb 08 06:54:03 2017 Remove logic to reset input method more than needed The current logic is to reset input method whenever selection is outside the composition. For example, if you touch outside the composition, or JavaScript removes the composition and text preceding it, then the composition will be finished and then browser will reset input method. This logic was originally added to fix an Android bug ( http://crbug.com/164427 ), but the bug is not reproducible any more without the fix. Also, the comment http://crbug.com/164427#c16 mentions another Linux bug, but the current logic is a half-baked solution. It fixes this case: http://jsfiddle.net/eEpQz/2/ but it doesn't fix this case: https://jsfiddle.net/galmacky/vct0Lk5y/ On Android, this reset causes unnecessary delay, and keyboard app loses the context and ceases to produce suggestions. Therefore, I think it makes sense to remove the logic to simplify the overall flow. Although it is not explicitly stated in the Android spec, Android's input method apps should finish composition when selection changes unexpectedly, therefore this case should be covered by the IME apps. I have manually tested against numerous apps, and could not find a case where it does not do it. Android's sample input method covers it as well. The changes here include: 1) InputMethodController not to handle selection change notification - not finish composition nor restart input. Now the keyboard app should handle this case. 2) ImeTest's TestInputMethodManagerWrapper has a logic to finish composition when there is an external selection change. Note that this does not fix the general problem of editor-keyboard conflict in any way, but we didn't see a case where the issue worsens with this change. BUG= 630137 Review-Url: https://codereview.chromium.org/2370663002 Cr-Commit-Position: refs/heads/master@{#448924} [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/content/public/android/java/src/org/chromium/content/browser/input/Range.java [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/content/public/android/junit/src/org/chromium/content/browser/input/RangeTest.java [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestInputMethodManagerWrapper.java [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/content/renderer/render_view_impl.cc [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/content/renderer/render_view_impl.h [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/core/editing/Editor.cpp [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/core/editing/FrameSelection.cpp [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/core/editing/InputMethodController.cpp [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/core/editing/InputMethodController.h [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/core/page/ChromeClient.h [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/web/ChromeClientImpl.cpp [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/web/ChromeClientImpl.h [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/Source/web/tests/WebViewTest.cpp [modify] https://crrev.com/50a098f28262dca3de4fa9ff3b26f7a14f5d582a/third_party/WebKit/public/web/WebViewClient.h
,
Feb 8 2017
,
Feb 22 2017
,
Feb 22 2017
Issue 683387 has been merged into this issue. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by changwan@chromium.org
, Aug 3 2016