I cannot repro on my Galaxy S7 with 54.0.2840.34/MMB29M/Google Keyboard (Latin) 5.1.23.127065177-arm64-v8a (nor with 55.0.2868.0).  I wonder why not?

Hmm, I can repro the problem 10% of the time if I am very fast between steps 3 and 4.  Also, tapping on the form field and then quickly typing a letter has the same bug.  The more common failure mode is that the letter I typed quickly after switching focus is simply dropped.

I found the following scenario:

input form #1 [abcdefg]
input form #2 [hij]

if you move from input form #1 to input form #2 quickly,
and type k, the following happens:

1) focusedNodeChanged calls restartInput()
2) restart with initial selection = 7  // this is incorrect
3) getTextBeforeCursor() returns "hij"
4) setComposingRegion(4, 7) // no op
5) setComposingText("hijk")

Now the text becomes "hijhijk".

This reproes quite often even without any javascript, marking it as RBS.
I'm checking if setting (-1, -1) as initial selection can solve this issue.

Google Keyboard app does not work correctly when you set (-1, -1) as initial selection.

Another idea on our end would be to update selection more aggressively. When focus changes, the following happens:
1) cancelComposition restarts input.
2) focusedNodeChanged restarts input.
3) selection state update follows.

Currently, selection update inside ImeEventGuard gets ignored. I can change that and
update selection immediately before the above two issues. What do you think?

Removing RBS as this is preexisting on M53.


As for the fix, I tried changing focusedNodeChanged to forcefully update selection change. It fixes the original problem, but I found that there's another variation of this problem as follows:

1) setComposingText() is called on input form A.
2) Focus changed to input form B.
3) Actually setComposingText() arrives later, so input form B gets affected.

Not sure if this issue is also preexisting or not - it's shadowed by the original problem.

Ultimately, cancelComposition and focusedNodeChanged should wait until restart input takes effect.

Forceful update before focusedNodeChanged seems to reintroduce  issue 484139 .
And the text state is incorrect anyways.

Some ideas:
1) Fix focusedNodeChanged to be called *after* focus node change occurs, not *in the middle of* it.
2) Delay focusedNodeChanged handling.

Also, "waiting until restart input takes effect" --> actually needs to empty the IME thread handler queue.

One idea I've been mulling is to create a different placeholder Android View per text field, and switch between them.  Would that solve this problem?

Re #8, I think that the render widget behavior change is more important here.

There are three things that needs to be done to fix this.

1) Ignore additional IME operations until browser gets notified of CancelComposition or FocusedNodeChanged.

Especially for FocusedNodeChanged, the operations on the old input form do not make any more sense. I haven't thought through CancelComposition case yet,
but I think it makes sense to synchronize the renderer-browser states before restarting input method.

2) Make sure we give timely text input state update before CancelComposition / FocusedNodeChanged is sent out. One thing important to note is that selection values are likely a garbage in the middle of focus change. ( issue 484139 ).

I've come up with two different approaches to handle 2).

The first approach is to defer sending of CancelComposition and FocusedNodeChanged till the end of ImeEventGuard: https://codereview.chromium.org/2470713002/

The second approach is to force-update text input state even in the middle of ImeEventGuard, while trying to fix garbage selection problem in 2):
https://codereview.chromium.org/2466283002/

What do you think about these approaches?

Hmm, I thought about this, and I don't really like either approach that much, unfortunately...  I think this problem is difficult and will require a lot of care.

For 1), I think we should try not to ignore the additional IME operations -- even better would be to apply them on the previously focused text box, right?  Otherwise we introduce a bug where a letter you typed right before tapping on the next textbox is ignored.

For 2), it sounds to me like what's appropriate is not so much more timeliness/ordering (I think there risks having a window of time where IME would see new textinputstate with old textbox?), but rather a more atomic focus change operation which contains the selection update as part of it.


And I'm also worried about the asynchronous channel between IME process and InputConnectionHandlerThread.  That property means that we can get no guarantee about what input box is targeted by an IME call just simply by examining event ordering.  Even if it arrived after restartInput, it may still have been targeted at old box.  We need a reliable method to identify what textbox IME meant to target.  That means using something like Android View per textbox.  In fact, I think I've convinced myself that restartInput() is broken garbage because of this ordering problem and we should *never* use it for anything.


How about this proposal we introduce a focus change "handshake":
A) Touch event sent to change focus to textbox 2.
B) By coincidence, IME calls setComposingText("What") for textbox 1 after this touch event.
C) Touch event arrives at renderer.  Renderer sends up ViewHostMsg_RequestFocusChange but does *not* change focus yet.
D) setComposingText("What") arrives at renderer.  It still applies to textbox 1 as intended.
E) Browser receives ViewHostMsg_RequestFocusChange.
F) (Cross-OOPIF focus change only) Browser sends ViewMsg_ClearFocus to previously focused render process, and asynchronously awaits reply.
G) Browser tells IME to attach to a different shim textbox Android View; within the initialization of this, calls ViewMsg_ApplyRequestedFocusChange and blocks the IME thread on the reply.
H) Renderer receives ViewMsg_ApplyFocusChange and finally applies focus change.  Renderer sends back a FROM_IME TextInputState.
I) Browser unblocks IME thread.
J) Browser receives a delayed IME message labeled for the textbox 1 shim Android View.  This one is ignored (it's too late to try to apply it to textbox 1).


Complex, but I *think* all of it is needed for an optimal solution to the races here.  By the way, I think  http://crbug.com/660645  is higher priority than this and the fix for that one will be easier, so marking this one for M-56.

Hmm, reading what restartInput does in the IMM code (https://github.com/android/platform_frameworks_base/blob/master/core/java/android/view/inputmethod/InputMethodManager.java#L1128 ), it seems really similar to what IMM does when Views are swapped out, so I guess there's probably no point in the shim Android View part of my proposal, and we can probably stick with restartInput for focus change.  I assume one of those unbinding things probably results in future IME Binder messages getting dropped (though it would be good to confirm for sure).

cc'ing yosin@ and kochi@ who might be interested in what's proposed at #10.

I drew a diagram of the plan in #10 (step "F" omitted for simplicity), to help visualize it.  Note that the main goal of the design is that Blink never needs to ignore any IME message because IME thread is blocked during the critical period when focus is changing.

Deleted: ime_focuschange_redesign.jpg 1.1 MB

	ime_focuschange_redesign.jpg 1.1 MB View Download

Talked to Alex offline, but there is an issue with the above design in the following scenario:

1) change focus from input #1 to #2
2) update display
3) change selection or text

Then either display update should wait for the focus sync to complete or display may become inconsistent with the IME changes.

I'm thinking of writing up alternative design ideas, but the first step would be to make initial selection start / end to be correct. It can be done once 
https://codereview.chromium.org/2370663002/ lands
and then we add selection values as the FocusedNodeChanged message's parameters. I know that this only partially fixes the issue, but I think it could be a good start.

aelias@, what do you think?

To clarify, the mentioned CL removes additional restartInput caused by CancelComposition, so there will be only one-time restartInput when moving focus, where we fix selection values such that IME does not get confused.

Yeah, my proposal above doesn't work in the case where Javascript changes the focus and also immediately writes text into the box without releasing the message loop, so back to the drawing board unfortunately.

Sounds OK to pass in the extra information in FocusedNodeChanged.  That will probably be needed in any design we end up with.

Hmm... Unfortunately, fixing selection values is not simple.

In the case of switching inputs using touch events,
MouseEventManager::handleMouseFocus() initiates setFocusedElement()
and
MouseEventManager::handleMousePressEvent() initiates setSelection()

And focusedNodeChanged() is initiated by setFocusedElement() and selection is not prepared at that time.

One idea here is as follows:

1) RenderFrameImpl::FocusedNodeChanged() resets RenderWidget::text_input_* variables, such that the same state on the next input can still cause text input state update.
2) ImeAdapter does not restartInput() on FocusedNodeChanged. (Unless input type is NONE for which we should hide keyboard)
3) ImeAdapter restarts input on the next state update.

SGTM to make restartInput a boolean on TextInputState update similar to show_virtual_keyboard.  That guarantees that we'll always have the latest information to answer the question "restart with what?" even in the case of JS-change-based restarts.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62f5729e30dc50465f85419949564cb6e786dd49

commit 62f5729e30dc50465f85419949564cb6e786dd49
Author: changwan <changwan@chromium.org>
Date: Fri Feb 17 08:28:25 2017

[Android] Restart input only once on focus change

Even when finishComposingText() returns false in
InputMethodController::willChangeFocus(), we do not need to reset input
because RenderFrameImpl::focusedNodeChanged() will reset input in the
later part of focus change process.

Also, currently updateKeyboardVisibility() and updateState() in ImeAdapter
may each restart input which is another source of redundant restart input.
By merging the code path and changing some order of execution, we can
reduce additional restart input.

Lastly, we defer restartInput until the next state update. By the time we
send FocusedNodeChanged, selection is not created for the newly focused
input because of the reason mentioned in http://crbug.com/650204#c17,
so we want to defer it in order to provide correct initial sel values for
View#onCreateInputConnection().

BUG=650204

Review-Url: https://codereview.chromium.org/2681833006
Cr-Commit-Position: refs/heads/master@{#451269}

[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/renderer/input/render_widget_input_handler_delegate.h
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/renderer/render_view_impl.cc
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/renderer/render_view_impl.h
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/renderer/render_widget.cc
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/content/renderer/render_widget.h
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/third_party/WebKit/Source/web/ChromeClientImpl.h
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/third_party/WebKit/public/web/WebViewClient.h
[modify] https://crrev.com/62f5729e30dc50465f85419949564cb6e786dd49/third_party/WebKit/public/web/WebWidgetClient.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9353655a861dd23ab239f5f400650bc04050ca2e

commit 9353655a861dd23ab239f5f400650bc04050ca2e
Author: Alexandre Elias <aelias@chromium.org>
Date: Thu Mar 09 19:58:32 2017

[Android] Restart input only once on focus change

Even when finishComposingText() returns false in
InputMethodController::willChangeFocus(), we do not need to reset input
because RenderFrameImpl::focusedNodeChanged() will reset input in the
later part of focus change process.

Also, currently updateKeyboardVisibility() and updateState() in ImeAdapter
may each restart input which is another source of redundant restart input.
By merging the code path and changing some order of execution, we can
reduce additional restart input.

Lastly, we defer restartInput until the next state update. By the time we
send FocusedNodeChanged, selection is not created for the newly focused
input because of the reason mentioned in http://crbug.com/650204#c17,
so we want to defer it in order to provide correct initial sel values for
View#onCreateInputConnection().

BUG=650204

Review-Url: https://codereview.chromium.org/2681833006
Cr-Commit-Position: refs/heads/master@{#451269}
(cherry picked from commit 62f5729e30dc50465f85419949564cb6e786dd49)

Review-Url: https://codereview.chromium.org/2736413002 .
Cr-Commit-Position: refs/branch-heads/2987@{#807}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/renderer/input/render_widget_input_handler_delegate.h
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/renderer/render_view_impl.cc
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/renderer/render_view_impl.h
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/renderer/render_widget.cc
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/content/renderer/render_widget.h
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/third_party/WebKit/Source/web/ChromeClientImpl.h
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/third_party/WebKit/public/web/WebViewClient.h
[modify] https://crrev.com/9353655a861dd23ab239f5f400650bc04050ca2e/third_party/WebKit/public/web/WebWidgetClient.h

Note I cherry-picked this to 57 to fix http://crbug.com/699699

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec5e87b1835fd460bc175897024a44f567b32e22

commit ec5e87b1835fd460bc175897024a44f567b32e22
Author: Alexandre Elias <aelias@chromium.org>
Date: Thu Mar 09 20:45:37 2017

Fix Windows compile error with cherry-pick.

M57 branch had desktop-specific RenderWidgetInputHandlerDelegate derived
class apparently not present on ToT, causing http://crrev.com/2736413002
to break compile.  Switch to an empty default implementation to cover
it.

BUG=700133,650204,699699

Review-Url: https://codereview.chromium.org/2740133003 .
Cr-Commit-Position: refs/branch-heads/2987@{#808}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/ec5e87b1835fd460bc175897024a44f567b32e22/content/renderer/input/render_widget_input_handler_delegate.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7026b266c93ccd4ad36cc3a543381ff6b8b78baf

commit 7026b266c93ccd4ad36cc3a543381ff6b8b78baf
Author: Alexandre Elias <aelias@chromium.org>
Date: Fri Mar 10 19:17:05 2017

Revert restartInput change off the M57 release branch.

Although there are no known problems with this patch, this should
minimize risk given that we are landing another fix for the root cause
http://crrev.com/456116.

Revert "[Android] Restart input only once on focus change"

This reverts commit 9353655a861dd23ab239f5f400650bc04050ca2e.

Revert "Fix Windows compile error with cherry-pick."

This reverts commit ec5e87b1835fd460bc175897024a44f567b32e22.

BUG=650204,699699

Review-Url: https://codereview.chromium.org/2741993003 .
Cr-Commit-Position: refs/branch-heads/2987@{#811}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/renderer/input/render_widget_input_handler_delegate.h
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/renderer/render_view_impl.cc
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/renderer/render_view_impl.h
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/renderer/render_widget.cc
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/content/renderer/render_widget.h
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/third_party/WebKit/Source/core/editing/InputMethodController.cpp
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/third_party/WebKit/Source/web/ChromeClientImpl.h
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/third_party/WebKit/public/web/WebViewClient.h
[modify] https://crrev.com/7026b266c93ccd4ad36cc3a543381ff6b8b78baf/third_party/WebKit/public/web/WebWidgetClient.h