changwan@, could you take look?

Able to reproduce the issue on Windows 10, mac 10.12.6 and Ubuntu 14.04 using chrome reported version #59.0.3071.115 and latest canary #62.0.3168.0.

Bisect Information:
=====================
Good build: 58.0.3015.0    Revision(451180)
Bad Build : 58.0.3016.0    Revision(451403)

Change Log URL: 
https://chromium.googlesource.com/chromium/src/+log/9d20a4962d5ffabb59953294bb28c6c9799422a8..62f5729e30dc50465f85419949564cb6e786dd49

From the above change log suspecting below change
Review url: https://codereview.chromium.org/2681833006

changwan@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Thanks...!!

rlanday@, could you take a look?

I can repro on Mac but not Android. Interesting.

Reverting the culprit CL on macOS fixes the issue. So we probably need to add an IME reset back in on macOS somewhere.

If I revert it on Linux, there's still another issue causing the character to automatically appear a second time after composition ends (without pressing an arrow key). So there's an extra piece of logic missing there somewhere.

I will set up a Windows build device so I can test there.

Ok, I can provide some more information about the Linux-specific issue.

All of the following is with https://codereview.chromium.org/2681833006 ([Android] Restart input only once on focus change) reverted.

When blur() is called on the contenteditable element while we have an active composition, the renderer sends an InputHostMsg_ImeCancelComposition message to the browser to tell it to cancel the composition. The stack trace for this on Linux looks like:

#0 0x7f3faa2edd99 ui::InputMethodAuraLinux::ResetContext()
#1 0x7f3faa2ee3aa ui::InputMethodAuraLinux::CancelComposition()
#2 0x7f3faf3f35e9 content::RenderWidgetHostViewAura::OnImeCancelComposition()
#3 0x7f3faf404e1e content::TextInputManager::ImeCancelComposition()
#4 0x7f3faf3f73c8 content::RenderWidgetHostViewBase::ImeCancelComposition()
#5 0x7f3faf3ca994 content::RenderWidgetHostImpl::OnImeCancelComposition()

So we're inside ui::InputMethodAuraLinux::ResetContext() and we need to reset the IME state. The code for this is here:
https://chromium.googlesource.com/chromium/src/+/ab1ba1ead2975ce4237cd8ffec8f559d223bbe1e/ui/base/ime/input_method_auralinux.cc#302

We first call context_->Reset() (I haven't yet learned why we also need context_simple_, but I don't think it's immediately relevant), which calls  gtk_im_context_reset() as we'd expect:
https://developer.gnome.org/gtk3/stable/GtkIMContext.html#gtk-im-context-reset

Apparently (from a code comment), "Some input methods may not honour the reset call." So we also call Blur() (gtk_im_context_focus_out()) and Focus() (gtk_im_context_focus_in()), just to be sure.

We have a bug here: there's a flag suppress_next_result_ that gets set to true immediately before we call gtk-im-context-reset() that makes us ignore the next commit signal from the IME. However, gtk_im_context_focus_out() and gtk_im_context_focus_in() can *also* send us a commit signal that we also have to ignore. One of these (gtk_im_context_focus_out(), I believe) is causing the Hangul IME to try to commit text, and we need to ignore it (since we've already saved the pending composition), but we're not. That's where the extra character comes from.

(The code for the Hangul IME notes that it's configured to commit the composition text upon focus out: https://github.com/choehwanjin/ibus-hangul/blob/341ba797e5117e1339b97be655617bcc8b51a464/src/engine.c#L1318)

So, really what we need to do here is ignore all commit signals from the IME until the whole "reset, blur, focus" procedure is done. *Problem is*, the commit signals are async, so I don't think there's any good way of knowing how many we're actually going to get. Currently, suppress_next_result_ is never set back to false at the end of ResetContext(), so we always drop exactly one commit signal from "reset, blur, focus." If the IME never sends us a commit signal during this procedure, I assume we drop whatever the next commit would be (but I haven't run into this yet, so idk if it's a problem in practice or not).

Really what we need to do is suppress *any* commit that results from *any of* Reset(), Blur(), or Focus(). However, we receive the commit signals asynchronously, which makes things tricky. If we set suppress_next_result_ back to false at the end of ResetContext(), we won't actually suppress any commits, because we won't handle the commit signal until MessagePumpGlib::Run() runs again, which is well after this method returns. If we leave it as true (as we're doing now), we'll suppress exactly one commit (it gets set back to false after handling one commit signal), which isn't enough for the Hangul IME. We need to suppress all resulting commits from Reset()/Blur()/Focus(), but end our commit suppression once the IME has finished handling reset/blur/focus. Unfortunately, I don't know if there's any way to know this.


One potential idea is to just suppress all commit events until we get the next keypress. That seems to resolve the bug I'm seeing, but I don't know for sure that it wouldn't cause other problems. For example, if the IME receives input via a mouse clicking on an on-screen keyboard, that probably wouldn't be sending us any keypress events. We could also maybe ignore all commit signals for a certain amount of time after reset/blur/focus is called, but that's  obviously not ideal.

Note: we had an Android bug (crbug.com/650204) where typing into one text box and then clicking in another would replicate some of the text into the new text box. We currently have the same bug on Linux when typing Hangul because of this suppression issue I described.

On Android, Changwan's CL (https://codereview.chromium.org/2681833006) makes us defer restarting input until we get the next state update with updated selection values. I am not sure if this deferral is necessary on Linux, since it looks like we never call gtk_im_context_set_surrounding() to tell the IME about the text surrounding the cursor. On Linux (still need to check Mac and Windows), we might just be able to call ResetContext() in response to a focused node change, and call it a day. Or, maybe we can find a way to consolidate the logic and use the Android logic for this (since it seems to be the most flexible/correct) on all platforms.

The tricky part of course is going to be fixing the Linux-specific issue in Comment 9 :(

I can't think of any solution for the Linux issue other than the timer idea. E.g. upon resetting the input, we look at the current time, and ignore all commits until 100 ms has elapsed (since this is enough time that the message loop should've been run again,  but also probably not enough time for someone to click an on-screen keyboard (or perform voice input, or swipe a word on a touchscreen, etc.) after the new field is focused). It might be possible for someone to e.g. quickly tab over to a new form field and start typing again, but that's OK, since we can stop the timer early if we receive a key event that we're about to pass to the IME.

Does this seem reasonable?

I think I have a better idea: the commit signals are handled asynchronously, but I believe they're *generated* synchronously in response to Reset()/Blur()/Focus(). So if we can emit a special signal immediately after these operations, we can ignore all commit signals until we handle this special signal we emitted.

Re #12, this sounds like something we should pursue - we need to apply the same logic to Android, too. Could you write up a design doc / one pager for this? I remember yosin@ once mentioned the idea of 'IME session'. I'm not sure if special signal + ignoring is enough or we really need to introduce the concept of session (or just session ID) for serial focus changes.

changwan@, I'm not sure if we're talking about the same issue or not. Are you thinking of fixing something about the interaction between the renderer and browser? This one seems specific to how the GtkImContext API works.

I don't see any way to do this without using a timer. I tried emitting a signal on the gtk_context_ object in X11InputMethodContextImplGtk2 after reset is finished, but I discovered that GTK signals are actually synchronous, so this doesn't do anything productive. Apparently the asynchronous event that the IME is committing text in response to is a "UpdatePreeditText" signal that IBus is sending us over D-Bus. I'm not sure how we can send a D-Bus signal to ourselves or if that would be productive, but I tried using MessageLoopForUi::current()->task_runner()->PostTask() to try to update the suppress_next_input_ variable the next time the message loop runs (in the hopes that this would be after the UpdatePreeditText signal is handled), but this is incredibly flaky only seems to run before we finish handling all of the D-Bus signals about 50% of the time.

I'll put up a CL for the "ignore commits for 100ms or until the next key event" approach. This seems to work very well.

Hmm... Sorry, I read again carefully. I'm not quite familiar with GTK, so this is purely based on your descriptions. Here are some questions:

1) Are you saying that have preedit --> reset() does not get commit() callback while have preedit --> focus out --> focus in does? How about other callbacks such as preedit_end / preedit_change?

2) In case that there is no callback at all, is it possible to ignore until get_preedit_string becomes empty if it had a preedit?

3) The other problem I can think of is, since it is asynchronous, that other preedit / commit sequence may arrive before the final commit comes. Is this possible?

4) This sounds like a common problem on Linux, so I think there should be a solution, or we should be able to suggest something. How do firefox and native views handle this?

1) When we do reset, blur, focus, we get the follow signals upon clicking into the other text box:

commit: ㅁ (we currently ignore this one since it's the first commit after a reset)
preedit-changed with string "" and cursor position 0
preedit-end with cursor position 0
commit: ㅁ (this gives us the duplicate character).

If we *only* do reset, we get:
preedit-changed with string "" and cursor position 0
preedit-end with cursor position 0
commit: ㅁ (we currently ignore this one since it's the first commit after a reset).

That is, calling blur() and then focus() *after* reset() seems to add an extra commit message *before* the messages that would've resulted from just reset(). I'm not yet sure why this is.

If we *only* call blur() and focus() (without resetting first), we get:

commit: ㅁ (we currently ignore this one since it's the first commit after a reset)
preedit-changed with string "" and cursor position 0
preedit-end with cursor position 0

2) I don't think this is possible because if you look at the reset, blur, focus case above, the preedit string becomes empty right before a commit that we want to ignore.

3) I do not fully understand the event ordering that's going on (see my comment in 1)). I think we should probably just drop input that comes in while we're switching the node focus anyway, so I'm not too worried about this possibility.

4) Firefox has the same bug where clicking into another text box copies the Hangul character into a new text box. Native GTK text widgets do not seem to have this bug. My guess is that they're resetting input less aggressively than us, or in a better order. Here's the GTK TextView logic for handling focus out:
https://github.com/GNOME/gtk/blob/9bc7581f1c0a26180c3ef1e154cb54ccc6e6104c/gtk/gtktextview.c#L5753

Trying a few ideas (e.g. moving blur() before reset()), I am unable to come up with a sequence of events that doesn't cause the Hangul IME to emit a commit message that we have to ignore to avoid a duplicate character. It might be interesting to determine how GTK's TextView avoids this problem, but I think spending a lot of time to rewrite our input handling to better match GTK is not necessary to fix this particular bug.

We have another bug on Chrome OS where, if you're not at the end of the text in the text box, after the text box gets blurred, the cursor advances forward one character and the text box stays focused/becomes focused again. This is true both with or without my proposed fix for the duplicate character issue in https://chromium-review.googlesource.com/c/chromium/src/+/734644. I'll try to decide tomorrow if there's an easy fix for this or if I should just file a bug for a Chrome OS engineer.

Hmm... How about just partially reverting my change for other OSes and then fix each OS as one off with lower priority? Otherwise, this sounds like a complex project that can take longer.

That's basically what I'm doing. I'm adding the call to CancelComposition() back in for Aura (Windows/Linux/Chrome OS) and Mac in https://chromium-review.googlesource.com/c/chromium/src/+/734644. This is platform-specific code though, so I need to actually test it on all these platforms. Then it turned out there was one related bug on Linux (which I have a fix up for at https://chromium-review.googlesource.com/c/chromium/src/+/734171) and one related bug on Chrome OS (which I think I'm just going to file a bug for, since it turns out to be super slow to build Chrome for Chrome OS in a way that lets the IMEs work properly).

I've filed  crbug.com/778360  for the Chrome OS bug.

Actually, I don't think the Chrome OS bug is real. The test case attached to this bug has some extra code after the focus() that I should've had commented out but didn't. Oops.

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

commit 1a08079972f7319fb586f3cbe4e087f1fbb6095b
Author: Ryan Landay <rlanday@chromium.org>
Date: Fri Oct 27 22:00:22 2017

Fix IME not being reset on focused node change on non-Android platforms

In https://codereview.chromium.org/2681833006, we changed our logic for
restarting input on focus change on Android, and inadvertently broke the logic
on other platforms (Mac/Windows/Linux/Chrome OS).

We were previously resetting the IME in code shared across all platforms, in
InputMethodController::willChangeFocus(). We were calling
frame().chromeClient().resetInputMethod() to send an IPC message to the browser
process. We removed that code and replaced it with Android-specific logic in
ImeAdapter. On Android, we wait until the next TextInputState update to actually
reset the composition, so we can give the right selection offsets to the IME at
the same time, but there's apparently no need to do this on other platforms (at
least, we've never had logic to do it this way), so in this CL, I'm fixing the
issue by just canceling the composition immediately upon focus change for these
platforms in RenderWidgetHostView{Aura,Mac}.

This CL fixes the logic for these platforms. 

Bug:  748438 
Change-Id: I841cd6cbf0aa30f4c986c865ed313462cda1b03b
Reviewed-on: https://chromium-review.googlesource.com/734644
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Shu Chen <shuchen@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512299}
[modify] https://crrev.com/1a08079972f7319fb586f3cbe4e087f1fbb6095b/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/1a08079972f7319fb586f3cbe4e087f1fbb6095b/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/1a08079972f7319fb586f3cbe4e087f1fbb6095b/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/1a08079972f7319fb586f3cbe4e087f1fbb6095b/content/browser/renderer_host/render_widget_host_view_mac.mm
[modify] https://crrev.com/1a08079972f7319fb586f3cbe4e087f1fbb6095b/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm

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

commit 1fc2ad5779eb6e4bf79ad467f73b00fdc3689550
Author: Ryan Landay <rlanday@chromium.org>
Date: Fri Nov 03 19:54:47 2017

Fix logic on Linux that ignores IME commits during state reset

We have a bug typing Hangul on Linux where if you start typing in one text field
and click to focus another text field without pressing enter to commit the
current character first, the character is replicated into the new text box. This
is because the Hangul IME sends us commands while we're trying to reset it, and
our logic for ignoring these commands is currently to ignore exactly one, but
this IME emits more than command as part of this sequence.

I tried to find an approach where we could ignore IME events until we're done
handling all signals/events posted to us during the reset sequence, but
unfortunately I was unable to find a way that worked (see my notes on
 crbug.com/748438  for details). The best approach I was able to come up with is
to use a timer to ignore all events for about 100 ms after the reset sequence,
or until we receive a key event. I believe this will be quite robust in
practice, as we will never ignore IME events resulting from key presses, and
it's unlikely users will be able to enter text through other potential input
mechanisms (on-screen keyboard, speech, swiping, etc.) within 100 ms of focusing
on the new text box.

Bug:  748438 
Change-Id: I7d8cf2fbacf588f1477658d3517682e183fa5b6c
Reviewed-on: https://chromium-review.googlesource.com/734171
Commit-Queue: Ryan Landay <rlanday@chromium.org>
Reviewed-by: Shu Chen <shuchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513883}
[modify] https://crrev.com/1fc2ad5779eb6e4bf79ad467f73b00fdc3689550/ui/base/ime/input_method_auralinux.cc
[modify] https://crrev.com/1fc2ad5779eb6e4bf79ad467f73b00fdc3689550/ui/base/ime/input_method_auralinux.h

Verified the fix on Mac 10.12.6, Win-10 and Ubuntu 14.04 using latest chrome version #64.0.3260.0 as per the comment #0.
Attaching screen cast for reference.
Observed that letter did not display one more time as expected.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!

Deleted: 748438.webm 1.7 MB

	748438.webm 1.7 MB View Download