exo: Improve keyboard pressed key tracking.

After some testing we have determined that the following
pattern results in a more appropriate behavior when sending
key press/release events to clients:

- Send key press events if they have not been handled and
  are not already pressed.
- Send key release events if they are currently pressed.

This should reduce the chance that a client ends up with
an inappropriate keyboard state where some keys are
incorrectly pressed.

Bug: 820641
Test: exo_unittests --gtest_filter=KeyboardTest.OnKeyboardKey
Change-Id: Ica7b5510928056b6659140cf492e40234a272053
Reviewed-on: https://chromium-review.googlesource.com/1038907Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: David Reveman <reveman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555389}

components/exo/keyboard.cc

@@ -214,60 +214,62 @@ void Keyboard::AckKeyboardKey(uint32_t serial, bool handled) {
 // ui::EventHandler overrides:
 
 void Keyboard::OnKeyEvent(ui::KeyEvent* event) {
+  if (!focus_)
+    return;
+
   // Process reserved accelerators before sending it to client.
-  if (focus_ && ProcessAcceleratorIfReserved(focus_, event)) {
+  if (ProcessAcceleratorIfReserved(focus_, event)) {
     // Discard a key press event if it's a reserved accelerator and it's
     // enabled.
     event->SetHandled();
-    // Send leave/enter event instead of key event, so the client can know the
-    // actual state of the keyboard.
-    SetFocus(focus_);
-    return;
   }
 
   // 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 = focus_ ? ConsumedByIme(focus_, event) : false;
+  bool consumed_by_ime = ConsumedByIme(focus_, event);
 
-  if (focus_ && !consumed_by_ime && !event->handled()) {
-    int modifier_flags = event->flags() & kModifierMask;
-    if (modifier_flags != modifier_flags_) {
-      modifier_flags_ = modifier_flags;
-      delegate_->OnKeyboardModifiers(modifier_flags_);
-    }
+  // Always update modifiers.
+  int modifier_flags = event->flags() & kModifierMask;
+  if (modifier_flags != modifier_flags_) {
+    modifier_flags_ = modifier_flags;
+    delegate_->OnKeyboardModifiers(modifier_flags_);
+  }
 
-    switch (event->type()) {
-      case ui::ET_KEY_PRESSED:
-        if (pressed_keys_.insert(event->code()).second) {
-          uint32_t serial = delegate_->OnKeyboardKey(event->time_stamp(),
-                                                     event->code(), true);
-          if (are_keyboard_key_acks_needed_) {
-            pending_key_acks_.insert(
-                {serial,
-                 {*event, base::TimeTicks::Now() +
-                              expiration_delay_for_pending_key_acks_}});
-            event->SetHandled();
-          }
+  switch (event->type()) {
+    case ui::ET_KEY_PRESSED:
+      // Process key press event if not already handled and not
+      // already pressed.
+      if (!consumed_by_ime && !event->handled() &&
+          pressed_keys_.insert(event->code()).second) {
+        uint32_t serial =
+            delegate_->OnKeyboardKey(event->time_stamp(), event->code(), true);
+        if (are_keyboard_key_acks_needed_) {
+          pending_key_acks_.insert(
+              {serial,
+               {*event, base::TimeTicks::Now() +
+                            expiration_delay_for_pending_key_acks_}});
+          event->SetHandled();
         }
-        break;
-      case ui::ET_KEY_RELEASED:
-        if (pressed_keys_.erase(event->code())) {
-          uint32_t serial = delegate_->OnKeyboardKey(event->time_stamp(),
-                                                     event->code(), false);
-          if (are_keyboard_key_acks_needed_) {
-            pending_key_acks_.insert(
-                {serial,
-                 {*event, base::TimeTicks::Now() +
-                              expiration_delay_for_pending_key_acks_}});
-            event->SetHandled();
-          }
+      }
+      break;
+    case ui::ET_KEY_RELEASED:
+      // Process key release event if currently pressed.
+      if (pressed_keys_.erase(event->code())) {
+        uint32_t serial =
+            delegate_->OnKeyboardKey(event->time_stamp(), event->code(), false);
+        if (are_keyboard_key_acks_needed_) {
+          pending_key_acks_.insert(
+              {serial,
+               {*event, base::TimeTicks::Now() +
+                            expiration_delay_for_pending_key_acks_}});
+          event->SetHandled();
         }
-        break;
-      default:
-        NOTREACHED();
-        break;
-    }
+      }
+      break;
+    default:
+      NOTREACHED();
+      break;
   }
 
   if (pending_key_acks_.empty())

components/exo/keyboard_unittest.cc

@@ -189,10 +189,28 @@ TEST_F(KeyboardTest, OnKeyboardKey) {
   // This should not generate another press event for KEY_A.
   generator.PressKey(ui::VKEY_A, 0);
 
-  // This should only generate a release event for KEY_A.
+  // This should only generate a single release event for KEY_A.
   EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_A, false));
   generator.ReleaseKey(ui::VKEY_A, 0);
 
+  // Press accelerator after surface lost focus.
+  EXPECT_CALL(delegate, OnKeyboardLeave(surface.get()));
+  focus_client->FocusWindow(nullptr);
+  generator.PressKey(ui::VKEY_W, ui::EF_CONTROL_DOWN);
+
+  // Key should be pressed when focus returns.
+  EXPECT_CALL(delegate, CanAcceptKeyboardEventsForSurface(surface.get()))
+      .WillOnce(testing::Return(true));
+  EXPECT_CALL(delegate, OnKeyboardModifiers(ui::EF_CONTROL_DOWN));
+  EXPECT_CALL(delegate, OnKeyboardEnter(
+                            surface.get(),
+                            base::flat_set<ui::DomCode>({ui::DomCode::US_W})));
+  focus_client->FocusWindow(surface->window());
+
+  // Releasing accelerator when surface has focus should generate event.
+  EXPECT_CALL(delegate, OnKeyboardKey(testing::_, ui::DomCode::US_W, false));
+  generator.ReleaseKey(ui::VKEY_W, ui::EF_CONTROL_DOWN);
+
   keyboard.reset();
 }