Send a key event to a client always if the key event skips IME.

Key event dispatch order is changed for ARC apps window to support
View.onKeyPreIme() API on ARC apps. While the ARC apps are focused,
a key event is sent to exo::Keyboard before it's sent to IME.
In this case, exo::Keyboard should send a key event to a client always.
Please see go/arc-key-event-dispatch for the details.

Bug: b:148193316
Test: exo_unittests
Change-Id: I65cf1c2eb20eb20bc7767710b7c23e9269404428
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2215532
Commit-Queue: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774544}

chrome/browser/ui/ash/launcher/app_service/app_service_app_window_arc_tracker.cc

@@ -10,6 +10,7 @@
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/public/cpp/shelf_types.h"
 #include "ash/public/cpp/window_properties.h"
+#include "base/feature_list.h"
 #include "base/time/time.h"
 #include "chrome/browser/apps/app_service/app_service_proxy.h"
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
@@ -25,6 +26,7 @@
 #include "chrome/browser/ui/ash/launcher/arc_app_window_info.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager_helper.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/views/widget/widget.h"
 
@@ -305,6 +307,10 @@ void AppServiceAppWindowArcTracker::AttachControllerToWindow(
   window->SetProperty(ash::kArcPackageNameKey,
                       new std::string(info->package_name()));
   window->SetProperty(ash::kAppIDKey, new std::string(shelf_id.app_id));
+  if (base::FeatureList::IsEnabled(
+          chromeos::features::kArcPreImeKeyEventSupport)) {
+    window->SetProperty(aura::client::kSkipImeProcessing, true);
+  }
 
   if (info->app_shelf_id().app_id() == arc::kPlayStoreAppId)
     HandlePlayStoreLaunch(info);

components/arc/ime/arc_ime_service.cc

@@ -289,10 +289,6 @@ void ArcImeService::OnWindowFocused(aura::Window* gained_focus,
     DCHECK_EQ(nullptr, focused_arc_window_);
     focused_arc_window_ = gained_focus;
     focused_arc_window_->AddObserver(this);
-    if (base::FeatureList::IsEnabled(
-            chromeos::features::kArcPreImeKeyEventSupport)) {
-      focused_arc_window_->SetProperty(aura::client::kSkipImeProcessing, true);
-    }
     // The focused window and the toplevel window are different in production,
     // but in tests they can be the same, so avoid adding the observer twice.
     if (focused_arc_window_ != focused_arc_window_->GetToplevelWindow())

components/arc/ime/arc_ime_service_unittest.cc

@@ -17,7 +17,6 @@
 #include "components/arc/mojom/ime.mojom.h"
 #include "components/arc/session/arc_bridge_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/test/test_window_delegate.h"
 #include "ui/aura/test/test_windows.h"
 #include "ui/aura/window.h"
@@ -513,26 +512,4 @@ TEST_F(ArcImeServiceTest, DoNothingIfArcWindowIsNotFocused) {
   EXPECT_EQ(0, fake_input_method_->count_cancel_composition());
 }
 
-TEST_F(ArcImeServiceTest, PutSkipImeProcessingProperty) {
-  ASSERT_FALSE(arc_win_->GetProperty(aura::client::kSkipImeProcessing));
-
-  {
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitAndDisableFeature(
-        chromeos::features::kArcPreImeKeyEventSupport);
-    instance_->OnWindowFocused(arc_win_.get(), nullptr);
-    EXPECT_FALSE(arc_win_->GetProperty(aura::client::kSkipImeProcessing));
-    instance_->OnWindowFocused(nullptr, arc_win_.get());
-  }
-
-  {
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitAndEnableFeature(
-        chromeos::features::kArcPreImeKeyEventSupport);
-    instance_->OnWindowFocused(arc_win_.get(), nullptr);
-    EXPECT_TRUE(arc_win_->GetProperty(aura::client::kSkipImeProcessing));
-    instance_->OnWindowFocused(nullptr, arc_win_.get());
-  }
-}
-
 }  // namespace arc

components/exo/BUILD.gn

@@ -260,6 +260,7 @@ source_set("unit_tests") {
       "//ash/keyboard/ui",
       "//ash/public/cpp",
       "//chromeos/constants",
+      "//ui/base:test_support",
       "//ui/base/cursor/mojom:cursor_type",
     ]
   }

components/exo/keyboard.cc

@@ -272,7 +272,12 @@ void Keyboard::OnKeyEvent(ui::KeyEvent* event) {
   // When IME ate a key event, we use the event only for tracking key states and
   // ignore for further processing. Otherwise it is handled in two places (IME
   // and client) and causes undesired behavior.
-  bool consumed_by_ime = ConsumedByIme(focus_, event);
+  // If the window should receive a key event before IME, Exo should send any
+  // key events to a client. The client will send back the events to IME if
+  // needed.
+  const bool consumed_by_ime =
+      !focus_->window()->GetProperty(aura::client::kSkipImeProcessing) &&
+      ConsumedByIme(focus_, event);
 
   // Always update modifiers.
   int modifier_flags = event->flags() & kModifierMask;

components/exo/keyboard_unittest.cc

@@ -23,7 +23,9 @@
 #include "components/exo/test/exo_test_base.h"
 #include "components/exo/test/exo_test_helper.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/focus_client.h"
+#include "ui/base/ime/dummy_text_input_client.h"
 #include "ui/events/devices/device_data_manager.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/keycodes/dom/dom_code.h"
@@ -329,6 +331,77 @@ TEST_F(KeyboardTest, OnKeyboardKey) {
   keyboard.reset();
 }
 
+TEST_F(KeyboardTest, OnKeyboardKey_NotSendKeyIfConsumedByIme) {
+  std::unique_ptr<Surface> surface(new Surface);
+  std::unique_ptr<ShellSurface> shell_surface(new ShellSurface(surface.get()));
+  gfx::Size buffer_size(10, 10);
+  std::unique_ptr<Buffer> buffer(
+      new Buffer(exo_test_helper()->CreateGpuMemoryBuffer(buffer_size)));
+  surface->Attach(buffer.get());
+  surface->Commit();
+
+  aura::client::FocusClient* focus_client =
+      aura::client::GetFocusClient(ash::Shell::GetPrimaryRootWindow());
+  focus_client->FocusWindow(nullptr);
+
+  NiceMockKeyboardDelegate delegate;
+  Seat seat;
+  auto keyboard = std::make_unique<Keyboard>(&delegate, &seat);
+
+  EXPECT_CALL(delegate, CanAcceptKeyboardEventsForSurface(surface.get()))
+      .WillOnce(testing::Return(true));
+  EXPECT_CALL(delegate, OnKeyboardModifiers(0));
+  EXPECT_CALL(delegate,
+              OnKeyboardEnter(surface.get(),
+                              base::flat_map<ui::DomCode, ui::DomCode>()));
+  focus_client->FocusWindow(surface->window());
+
+  ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow());
+  views::Widget* widget =
+      views::Widget::GetTopLevelWidgetForNativeView(surface->window());
+  ui::InputMethod* input_method = widget->GetInputMethod();
+  ui::DummyTextInputClient client{ui::TEXT_INPUT_TYPE_TEXT};
+  input_method->SetFocusedTextInputClient(&client);
+
+  // If a text field is focused, a pressed key event is not sent to a client
+  // because a key event should be consumed by the IME.
+  EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_A, true))
+      .Times(0);
+  seat.set_physical_code_for_currently_processing_event_for_testing(
+      ui::DomCode::US_A);
+  generator.PressKey(ui::VKEY_A, 0);
+  // TODO(yhanada): The below EXPECT_CALL fails because exo::Keyboard currently
+  // sends a key release event for the keys which exo::Keyboard sent a pressed
+  // event for. It might causes a never-ending key repeat in the client.
+  // EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_A, false));
+  generator.ReleaseKey(ui::VKEY_A, 0);
+
+  // Any key event should be sent to a client if the focused window is marked as
+  // ImeBlocking.
+  WMHelper::GetInstance()->SetImeBlocked(surface->window()->GetToplevelWindow(),
+                                         true);
+  EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_B, true));
+  seat.set_physical_code_for_currently_processing_event_for_testing(
+      ui::DomCode::US_B);
+  generator.PressKey(ui::VKEY_B, 0);
+  EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_B, false));
+  generator.ReleaseKey(ui::VKEY_B, 0);
+  WMHelper::GetInstance()->SetImeBlocked(surface->window()->GetToplevelWindow(),
+                                         false);
+
+  // Any key event should be sent to a client if a key event skips IME.
+  surface->window()->SetProperty(aura::client::kSkipImeProcessing, true);
+  EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_C, true));
+  seat.set_physical_code_for_currently_processing_event_for_testing(
+      ui::DomCode::US_C);
+  generator.PressKey(ui::VKEY_C, 0);
+  EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_C, false));
+  generator.ReleaseKey(ui::VKEY_C, 0);
+
+  input_method->SetFocusedTextInputClient(nullptr);
+  keyboard.reset();
+}
+
 TEST_F(KeyboardTest, OnKeyboardModifiers) {
   std::unique_ptr<Surface> surface(new Surface);
   std::unique_ptr<ShellSurface> shell_surface(new ShellSurface(surface.get()));

components/exo/shell_surface_base.cc

@@ -58,6 +58,14 @@
 namespace exo {
 namespace {
 
+// Set aura::client::kSkipImeProcessing to all Surface descendants.
+void SetSkipImeProcessingToDescendentSurfaces(aura::Window* window) {
+  if (Surface::AsSurface(window))
+    window->SetProperty(aura::client::kSkipImeProcessing, true);
+  for (aura::Window* child : window->children())
+    SetSkipImeProcessingToDescendentSurfaces(child);
+}
+
 // The accelerator keys used to close ShellSurfaces.
 const struct {
   ui::KeyboardCode keycode;
@@ -833,6 +841,15 @@ void ShellSurfaceBase::OnWindowDestroying(aura::Window* window) {
   window->RemoveObserver(this);
 }
 
+void ShellSurfaceBase::OnWindowPropertyChanged(aura::Window* window,
+                                               const void* key,
+                                               intptr_t old_value) {
+  if (widget_ && window == widget_->GetNativeWindow() &&
+      key == aura::client::kSkipImeProcessing) {
+    SetSkipImeProcessingToDescendentSurfaces(window);
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // wm::ActivationChangeObserver overrides:
 

components/exo/shell_surface_base.h

@@ -181,6 +181,9 @@ class ShellSurfaceBase : public SurfaceTreeHost,
 
   // aura::WindowObserver:
   void OnWindowDestroying(aura::Window* window) override;
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old_value) override;
 
   // wm::ActivationChangeObserver:
   void OnWindowActivated(ActivationReason reason,

components/exo/shell_surface_unittest.cc

@@ -24,6 +24,7 @@
 #include "components/exo/test/exo_test_helper.h"
 #include "components/exo/wm_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/capture_client.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_delegate.h"
@@ -1076,4 +1077,26 @@ TEST_F(ShellSurfaceTest, CaptionWithPopup) {
   }
 }
 
+TEST_F(ShellSurfaceTest, SkipImeProcessingPropagateToSurface) {
+  gfx::Size buffer_size(256, 256);
+  auto buffer = std::make_unique<Buffer>(
+      exo_test_helper()->CreateGpuMemoryBuffer(buffer_size));
+  auto surface = std::make_unique<Surface>();
+  auto shell_surface = std::make_unique<ShellSurface>(surface.get());
+
+  surface->Attach(buffer.get());
+  surface->Commit();
+  shell_surface->GetWidget()->SetBounds(gfx::Rect(0, 0, 256, 256));
+  shell_surface->OnSetFrame(SurfaceFrameType::NORMAL);
+
+  aura::Window* window = shell_surface->GetWidget()->GetNativeWindow();
+  ASSERT_FALSE(window->GetProperty(aura::client::kSkipImeProcessing));
+  ASSERT_FALSE(
+      surface->window()->GetProperty(aura::client::kSkipImeProcessing));
+
+  window->SetProperty(aura::client::kSkipImeProcessing, true);
+  EXPECT_TRUE(window->GetProperty(aura::client::kSkipImeProcessing));
+  EXPECT_TRUE(surface->window()->GetProperty(aura::client::kSkipImeProcessing));
+}
+
 }  // namespace exo

components/exo/sub_surface.cc

@@ -8,6 +8,7 @@
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/traced_value.h"
 #include "components/exo/surface.h"
+#include "ui/aura/client/aura_constants.h"
 
 namespace exo {
 
@@ -113,6 +114,11 @@ bool SubSurface::IsInputEnabled(Surface* surface) const {
   return !parent_ || parent_->IsInputEnabled(surface);
 }
 
+void SubSurface::OnSetParent(Surface* parent, const gfx::Point&) {
+  if (parent->window()->GetProperty(aura::client::kSkipImeProcessing))
+    surface_->window()->SetProperty(aura::client::kSkipImeProcessing, true);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // SurfaceObserver overrides:
 

components/exo/sub_surface.h

@@ -55,7 +55,7 @@ class SubSurface : public SurfaceDelegate, public SurfaceObserver {
   void OnSetFrame(SurfaceFrameType type) override {}
   void OnSetFrameColors(SkColor active_color, SkColor inactive_color) override {
   }
-  void OnSetParent(Surface* parent, const gfx::Point& position) override {}
+  void OnSetParent(Surface* parent, const gfx::Point& position) override;
   void OnSetStartupId(const char* startup_id) override {}
   void OnSetApplicationId(const char* application_id) override {}
   void OnActivationRequested() override {}

components/exo/sub_surface_unittest.cc

@@ -4,11 +4,13 @@
 
 #include "components/exo/sub_surface.h"
 
+#include "components/exo/buffer.h"
 #include "components/exo/shell_surface.h"
 #include "components/exo/surface.h"
 #include "components/exo/test/exo_test_base.h"
 #include "components/exo/test/exo_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/client/aura_constants.h"
 
 namespace exo {
 namespace {
@@ -158,5 +160,25 @@ TEST_F(SubSurfaceTest, SetCommitBehavior) {
             grandchild->window()->bounds().origin().ToString());
 }
 
+TEST_F(SubSurfaceTest, SetOnParent) {
+  gfx::Size buffer_size(32, 32);
+  std::unique_ptr<Buffer> buffer(
+      new Buffer(exo_test_helper()->CreateGpuMemoryBuffer(buffer_size)));
+  auto parent = std::make_unique<Surface>();
+  auto shell_surface = std::make_unique<ShellSurface>(parent.get());
+  parent->Attach(buffer.get());
+  parent->Commit();
+
+  shell_surface->GetWidget()->GetNativeWindow()->SetProperty(
+      aura::client::kSkipImeProcessing, true);
+  ASSERT_TRUE(parent->window()->GetProperty(aura::client::kSkipImeProcessing));
+
+  // SkipImeProcessing property is propagated to SubSurface.
+  auto surface = std::make_unique<Surface>();
+  auto sub_surface = std::make_unique<SubSurface>(surface.get(), parent.get());
+  surface->SetParent(parent.get(), gfx::Point(10, 10));
+  EXPECT_TRUE(surface->window()->GetProperty(aura::client::kSkipImeProcessing));
+}
+
 }  // namespace
 }  // namespace exo