New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 630137 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 683387



Sign in to add a comment

check if we can avoid restarting input when tapping outside composition

Project Member Reported by changwan@chromium.org, Jul 21 2016

Issue description

Version: 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.

 
b/30601476 mentions this issue as well.
Cc: yosin@chromium.org
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();
            }
        }

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.

FYI, this is now blocked by a test failure which seems to have the same root cause as issue 664317.
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Comment 6 by aelias@chromium.org, Jan 24 2017

Blocking: 683387
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 8 2017

Project Member

Comment 8 by bugdroid1@chromium.org, 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

Labels: M-58
Status: Fixed (was: Assigned)
Blocking: -683387
Blocking: 683387
Cc: rlanday@chromium.org changwan@chromium.org alek...@chromium.org aluo@chromium.org
 Issue 683387  has been merged into this issue.

Sign in to add a comment