exo: Do not send text_input::keysym, if it is not IME consumed one.

Currently, some key combinations, such as CTRL+V, is sent twice
from exo to wayland client via wl_keyboard::key and text_input::keysym.
Because, currently Lacros converts both to KeyEvent, so in Lacros
the KeyEvent is dispatched twice, which is unexpected behavior.

The long term solution is not to dispatch KeyEvent converted from
text_input::keysym in Lacros, and instead, call
TextInputClient::InsertChar directly.
However, this approach hits another issue. Currently, exo does
not send wl_keyboard::key event, if the delivered KeyEvent is
being consumed by IME. This is the workaround for ARC, and planned
to be removed in the near future.

So, to workaround consumed-by-ime filtering for the short term,
this CL introduces another filtering into text_input::keysym with
the reversed condition, so that, KeyEvent dispatching in Lacros
will become once.

BUG=1149371
TEST=Ran on DUT. Ran exo_unittests.

Change-Id: I64314dda4838ae611e3643dc0e16eace9cc6fcd9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567283Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832595}

components/exo/keyboard.cc

@@ -54,70 +54,6 @@ bool ProcessAccelerator(Surface* surface, const ui::KeyEvent* event) {
   return false;
 }
 
-bool ConsumedByIme(Surface* focus, const ui::KeyEvent* event) {
-  // When IME is blocked, Exo can handle any key events.
-  if (WMHelper::GetInstance()->IsImeBlocked(focus->window()))
-    return false;
-
-  // Check if IME consumed the event, to avoid it to be doubly processed.
-  // First let us see whether IME is active and is in text input mode.
-  views::Widget* widget =
-      views::Widget::GetTopLevelWidgetForNativeView(focus->window());
-  ui::InputMethod* ime = widget ? widget->GetInputMethod() : nullptr;
-  if (!ime || ime->GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE ||
-      ime->GetTextInputType() == ui::TEXT_INPUT_TYPE_NULL) {
-    return false;
-  }
-
-  // Case 1:
-  // When IME ate a key event but did not emit character insertion event yet
-  // (e.g., when it is still showing a candidate list UI to the user,) the
-  // consumed key event is re-sent after masked |key_code| by VKEY_PROCESSKEY.
-  if (event->key_code() == ui::VKEY_PROCESSKEY)
-    return true;
-
-  // Except for PROCESSKEY, never discard "key-up" events. A keydown not paired
-  // by a keyup can trigger a never-ending key repeat in the client, which can
-  // never be desirable.
-  if (event->type() == ui::ET_KEY_RELEASED)
-    return false;
-
-  // Case 2:
-  // When IME ate a key event and generated a single character input, it leaves
-  // the key event as-is, and in addition calls the active ui::TextInputClient's
-  // InsertChar() method. (In our case, arc::ArcImeService::InsertChar()).
-  //
-  // In Chrome OS (and Web) convention, the two calls won't cause duplicates,
-  // because key-down events do not mean any character inputs there.
-  // (InsertChar issues a DOM "keypress" event, which is distinct from keydown.)
-  // Unfortunately, this is not necessary the case for our clients that may
-  // treat keydown as a trigger of text inputs. We need suppression for keydown.
-  //
-  // Same condition as components/arc/ime/arc_ime_service.cc#InsertChar.
-  const base::char16 ch = event->GetCharacter();
-  const bool is_control_char =
-      (0x00 <= ch && ch <= 0x1f) || (0x7f <= ch && ch <= 0x9f);
-  if (!is_control_char && !ui::IsSystemKeyModifier(event->flags()))
-    return true;
-
-  // Case 3:
-  // Workaround for apps that doesn't handle hardware keyboard events well.
-  // Keys typically on software keyboard and lack of them are fatal, namely,
-  // unmodified enter and backspace keys, are sent through IME.
-  constexpr int kModifierMask = ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN |
-                                ui::EF_ALT_DOWN | ui::EF_COMMAND_DOWN |
-                                ui::EF_ALTGR_DOWN | ui::EF_MOD3_DOWN;
-  // Same condition as components/arc/ime/arc_ime_service.cc#InsertChar.
-  if ((event->flags() & kModifierMask) == 0) {
-    if (event->key_code() == ui::VKEY_RETURN ||
-        event->key_code() == ui::VKEY_BACK) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
 bool IsVirtualKeyboardEnabled() {
   return keyboard::GetAccessibilityKeyboardEnabled() ||
          keyboard::GetTouchKeyboardEnabled() ||
@@ -283,7 +219,7 @@ void Keyboard::OnKeyEvent(ui::KeyEvent* event) {
   // needed.
   const bool consumed_by_ime =
       !focus_->window()->GetProperty(aura::client::kSkipImeProcessing) &&
-      ConsumedByIme(focus_, event);
+      ConsumedByIme(focus_->window(), *event);
 
   // Always update modifiers.
   // XkbTracker must be updated in the Seat, before calling this method.

components/exo/shell_surface_util.cc

@@ -17,6 +17,7 @@
 #include "ui/aura/window.h"
 #include "ui/aura/window_delegate.h"
 #include "ui/aura/window_targeter.h"
+#include "ui/events/base_event_utils.h"
 #include "ui/events/event.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/core/window_util.h"
@@ -260,4 +261,67 @@ bool HasPermissionToActivate(aura::Window* window) {
   return permission && permission->Check(Permission::Capability::kActivate);
 }
 
+bool ConsumedByIme(aura::Window* window, const ui::KeyEvent& event) {
+  // When IME is blocked, Exo can handle any key events.
+  if (WMHelper::GetInstance()->IsImeBlocked(window))
+    return false;
+
+  // Check if IME consumed the event, to avoid it to be doubly processed.
+  // First let us see whether IME is active and is in text input mode.
+  views::Widget* widget = views::Widget::GetTopLevelWidgetForNativeView(window);
+  ui::InputMethod* ime = widget ? widget->GetInputMethod() : nullptr;
+  if (!ime || ime->GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE ||
+      ime->GetTextInputType() == ui::TEXT_INPUT_TYPE_NULL) {
+    return false;
+  }
+
+  // Case 1:
+  // When IME ate a key event but did not emit character insertion event yet
+  // (e.g., when it is still showing a candidate list UI to the user,) the
+  // consumed key event is re-sent after masked |key_code| by VKEY_PROCESSKEY.
+  if (event.key_code() == ui::VKEY_PROCESSKEY)
+    return true;
+
+  // Except for PROCESSKEY, never discard "key-up" events. A keydown not paired
+  // by a keyup can trigger a never-ending key repeat in the client, which can
+  // never be desirable.
+  if (event.type() == ui::ET_KEY_RELEASED)
+    return false;
+
+  // Case 2:
+  // When IME ate a key event and generated a single character input, it leaves
+  // the key event as-is, and in addition calls the active ui::TextInputClient's
+  // InsertChar() method. (In our case, arc::ArcImeService::InsertChar()).
+  //
+  // In Chrome OS (and Web) convention, the two calls won't cause duplicates,
+  // because key-down events do not mean any character inputs there.
+  // (InsertChar issues a DOM "keypress" event, which is distinct from keydown.)
+  // Unfortunately, this is not necessary the case for our clients that may
+  // treat keydown as a trigger of text inputs. We need suppression for keydown.
+  //
+  // Same condition as components/arc/ime/arc_ime_service.cc#InsertChar.
+  const base::char16 ch = event.GetCharacter();
+  const bool is_control_char =
+      (0x00 <= ch && ch <= 0x1f) || (0x7f <= ch && ch <= 0x9f);
+  if (!is_control_char && !ui::IsSystemKeyModifier(event.flags()))
+    return true;
+
+  // Case 3:
+  // Workaround for apps that doesn't handle hardware keyboard events well.
+  // Keys typically on software keyboard and lack of them are fatal, namely,
+  // unmodified enter and backspace keys, are sent through IME.
+  constexpr int kModifierMask = ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN |
+                                ui::EF_ALT_DOWN | ui::EF_COMMAND_DOWN |
+                                ui::EF_ALTGR_DOWN | ui::EF_MOD3_DOWN;
+  // Same condition as components/arc/ime/arc_ime_service.cc#InsertChar.
+  if ((event.flags() & kModifierMask) == 0) {
+    if (event.key_code() == ui::VKEY_RETURN ||
+        event.key_code() == ui::VKEY_BACK) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 }  // namespace exo

components/exo/shell_surface_util.h

@@ -20,6 +20,7 @@ class TimeDelta;
 
 namespace ui {
 class LocatedEvent;
+class KeyEvent;
 }
 
 namespace exo {
@@ -88,6 +89,9 @@ std::unique_ptr<exo::Permission> GrantPermissionToActivate(
 // Returns true if the |window| has permission to activate itself.
 bool HasPermissionToActivate(aura::Window* window);
 
+// Returns true if event is/will be consumed by IME.
+bool ConsumedByIme(aura::Window* window, const ui::KeyEvent& event);
+
 }  // namespace exo
 
 #endif  // COMPONENTS_EXO_SHELL_SURFACE_UTIL_H_

components/exo/text_input.cc

@@ -8,6 +8,7 @@
 
 #include "ash/keyboard/ui/keyboard_ui_controller.h"
 #include "base/strings/string_piece.h"
+#include "components/exo/shell_surface_util.h"
 #include "components/exo/surface.h"
 #include "components/exo/wm_helper.h"
 #include "third_party/icu/source/common/unicode/uchar.h"
@@ -141,7 +142,14 @@ void TextInput::InsertChar(const ui::KeyEvent& event) {
     InsertText(base::string16(1, ch));
     return;
   }
-  delegate_->SendKey(event);
+  // TextInput is currently used only for Lacros, and this is the
+  // short term workaround not to duplicate KeyEvent there.
+  // This is what we do for ARC, which is being removed in the near
+  // future.
+  // TODO(fukino): Get rid of this, too, when the wl_keyboard::key
+  // and text_input::keysym events are handled properly in Lacros.
+  if (window_ && ConsumedByIme(window_, event))
+    delegate_->SendKey(event);
 }
 
 ui::TextInputType TextInput::GetTextInputType() const {

components/exo/text_input_unittest.cc

@@ -270,13 +270,27 @@ TEST_F(TextInputTest, Commit) {
 }
 
 TEST_F(TextInputTest, InsertChar) {
+  text_input()->Activate(surface());
+
   ui::KeyEvent ev(ui::ET_KEY_PRESSED, ui::VKEY_RETURN, 0);
 
   EXPECT_CALL(*delegate(), SendKey(testing::Ref(ev))).Times(1);
   text_input()->InsertChar(ev);
 }
 
+TEST_F(TextInputTest, InsertCharCtrlV) {
+  text_input()->Activate(surface());
+
+  // CTRL+V is interpreted as non-IME consumed KeyEvent, so should
+  // not be sent.
+  ui::KeyEvent ev(ui::ET_KEY_PRESSED, ui::VKEY_V, ui::EF_CONTROL_DOWN);
+  EXPECT_CALL(*delegate(), SendKey(_)).Times(0);
+  text_input()->InsertChar(ev);
+}
+
 TEST_F(TextInputTest, InsertCharNormalKey) {
+  text_input()->Activate(surface());
+
   base::char16 ch = 'x';
   ui::KeyEvent ev(ch, ui::VKEY_X, ui::DomCode::NONE, 0);