New issue
Advanced search Search tips

Issue 888740 link

Starred by 1 user
Status: Fixed
Owner:
sky@chromium.org
Closed: Sep 27
Cc:
shuchen@chromium.org
kinaba@chromium.org
xiy...@chromium.org
msw@chromium.org
jamescook@chromium.org
sky@chromium.org
yhanada@chromium.org
dxie@chromium.org
mukai@chromium.org
mustash-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

ws: Key events are sent to the underlying window while a menu is open.

Project Member Reported by msw@chromium.org, Sep 24 
ws: Key events are sent to the underlying window while a menu is open.

This affects the KSV app and the omnibox in SingleProcessMash
(1) Open the KSV (Ctrl-Alt-/) and focus the textfield
    (or the Omnibox with --enable-features=SingleProcessMash)
(2) Type some text, make a selection, etc.
(3) Open a context menu (right-click) in the textfield/omnibox (or on Chrome's tab/titlebar for omnibox)
(4) Type or press a mnemonic key (a key that invokes a menu item, eg. 'c' for copy, 'u' for undo)
Expected: Mnemonic keys invoke the menu item and close the menu, other keys do nothing.
Actual: Mnemonic keys still take action, but then all keys are sent to the textfield/omnibox as input.

This affects the KSV app in M-70... probably not a blocker, though? I'll take a look.

Comment 1 by dxie@chromium.org, Sep 24

Cc: dxie@chromium.org

Comment 2 by mukai@chromium.org, Sep 24

Cc: yhanada@chromium.org kinaba@chromium.org

Comment 3 by mukai@chromium.org, Sep 24

Cc: shuchen@chromium.org

Comment 4 by mukai@chromium.org, Sep 24 

note that
- menu keyevents are handled by views::MenuPreTargetHandlerAura::OnKeyEvent()
- it is called through aura::TextInputClientImpl::DispatchKeyEventPostIME(), called from ws::mojom::TextInputClient
- textfield is edited through aura::TextInputClientImpl::InsertChar() -- this is also a mojo method in ws::mojom::TextInputClient

Comment 5 by xiy...@chromium.org, Sep 25 

Good finding.

MenuPreTargetHandlerAura installs itself as aura::Env's pre target handler [1]. This probably does not work in a mojo app like KSV since the key event seems dispatched to IMEMus directly [2].

[1]: https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_pre_target_handler_aura.cc?rcl=a01342d0f40473b50cf47e7cf3c34f1b2062fb1d&l=28

[2] https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?rcl=81ac726681d099fb940643290274074fb9eda11c&l=1357

Comment 6 by sky@chromium.org, Sep 25 

At least part of the problem is TextInputClientImpl was dropping the ack on the floor. https://chromium-review.googlesource.com/c/chromium/src/+/1243671 gets us closer. I need to run it in a debugger to ensure it's the right thing. Mike, if you want to take this over you are welcome to. Otherwise pass it my way as I think the patch is pretty close.

Comment 7 by msw@chromium.org, Sep 25

Cc: msw@chromium.org
Owner: sky@chromium.org
If you're pretty close to a [partial] fix, then I'll pass this issue to you.
Send it back my way if there are other aspects you'd like me to investigate.
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 27 

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

commit ad6366d7ec456a4c1c493e37d578bd836ba29830
Author: Scott Violet <sky@chromium.org>
Date: Thu Sep 27 21:33:19 2018

ime: route ack to InputMethodDelegate

When IME runs remotely there is a callback that is used to signal
if the event was handled. This callback is passed around number places.
InputMethodChromeOS injects its own callback that does some processing.
InputMethodBase was running the callback immediately, not passing it through.
This meant some processing was happening that should have been conditional
based on the response from the remote client.

BUG= 888740 
TEST=none

Change-Id: I51804626cf5bce3bec9690ef33b75d94229228a4
Reviewed-on: https://chromium-review.googlesource.com/1243671
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594875}
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ash/display/window_tree_host_manager.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ash/display/window_tree_host_manager.h
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ash/display/window_tree_host_manager_unittest.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/chrome/browser/ui/views/ime_driver/remote_text_input_client.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/chrome/browser/ui/views/ime_driver/remote_text_input_client.h
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/extensions/shell/browser/shell_desktop_controller_aura.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/extensions/shell/browser/shell_desktop_controller_aura.h
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/aura/mus/input_method_mus.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/aura/mus/input_method_mus_unittest.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/aura/mus/text_input_client_impl.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/aura/window_tree_host.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/aura/window_tree_host.h
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/BUILD.gn
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_auralinux.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_auralinux_unittest.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_base.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_base.h
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_chromeos.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_chromeos_unittest.cc
[add] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_delegate.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_delegate.h
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_fuchsia.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_mac.mm
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_minimal.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_minimal_unittest.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/input_method_win_base.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/base/ime/mock_input_method.cc
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/views/cocoa/bridged_native_widget_host_impl.h
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/views/cocoa/bridged_native_widget_host_impl.mm
[modify] https://crrev.com/ad6366d7ec456a4c1c493e37d578bd836ba29830/ui/views/controls/textfield/textfield_unittest.cc

Comment 9 by sky@chromium.org, Sep 27

Status: Fixed (was: Started)
This should be fixed now. This is a slightly meaty change that I'm a bit hesitant to merge back, at least without some bake time.
►

Sign in to add a comment