overview: Fix using arrow keys in tablet mode.

Arrow keys cause a double selection movement in tablet mode. This was a
regression caused by introducing ctrl + arrow key to move the scrollable
grid. This patch fixes that by handling on key presses. Also stops scrolling
if ctrl is lifted before arrow key.

Test: added regression test
Change-Id: I89595a37880e85cbb903b0039785b20e8f7d0f71
Fixed: 1036140
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1977843
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727167}

ash/wm/overview/overview_highlight_controller_unittest.cc

@@ -19,6 +19,7 @@
 #include "ash/wm/overview/overview_item_view.h"
 #include "ash/wm/overview/overview_test_util.h"
 #include "ash/wm/overview/scoped_overview_transform_window.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller_test_api.h"
 #include "ash/wm/window_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "ui/aura/window.h"
@@ -71,6 +72,30 @@ TEST_F(OverviewHighlightControllerTest, BasicTabKeyNavigation) {
   EXPECT_EQ(overview_windows[1]->GetWindow(), GetOverviewHighlightedWindow());
   SendKeyUntilOverviewItemIsHighlighted(ui::VKEY_TAB);
   EXPECT_EQ(overview_windows[0]->GetWindow(), GetOverviewHighlightedWindow());
+  SendKeyUntilOverviewItemIsHighlighted(ui::VKEY_RIGHT);
+  EXPECT_EQ(overview_windows[1]->GetWindow(), GetOverviewHighlightedWindow());
+  SendKeyUntilOverviewItemIsHighlighted(ui::VKEY_LEFT);
+  EXPECT_EQ(overview_windows[0]->GetWindow(), GetOverviewHighlightedWindow());
+}
+
+// Same as above but for tablet mode. Regression test for crbug.com/1036140.
+TEST_F(OverviewHighlightControllerTest, BasicTabKeyNavigationTablet) {
+  std::unique_ptr<aura::Window> window1(CreateTestWindow());
+  std::unique_ptr<aura::Window> window2(CreateTestWindow());
+  std::unique_ptr<aura::Window> window3(CreateTestWindow());
+
+  TabletModeControllerTestApi().EnterTabletMode();
+  ToggleOverview();
+  const std::vector<std::unique_ptr<OverviewItem>>& overview_windows =
+      GetOverviewItemsForRoot(0);
+  SendKeyUntilOverviewItemIsHighlighted(ui::VKEY_TAB);
+  EXPECT_EQ(overview_windows[0]->GetWindow(), GetOverviewHighlightedWindow());
+  SendKeyUntilOverviewItemIsHighlighted(ui::VKEY_TAB);
+  EXPECT_EQ(overview_windows[1]->GetWindow(), GetOverviewHighlightedWindow());
+  SendKeyUntilOverviewItemIsHighlighted(ui::VKEY_RIGHT);
+  EXPECT_EQ(overview_windows[2]->GetWindow(), GetOverviewHighlightedWindow());
+  SendKeyUntilOverviewItemIsHighlighted(ui::VKEY_LEFT);
+  EXPECT_EQ(overview_windows[1]->GetWindow(), GetOverviewHighlightedWindow());
 }
 
 // Tests that pressing Ctrl+W while a window is selected in overview closes it.

ash/wm/overview/overview_session.cc

@@ -909,11 +909,15 @@ void OverviewSession::OnKeyEvent(ui::KeyEvent* event) {
     return;
   }
 
-  const bool process_released_key_event =
-      (event->key_code() == ui::VKEY_LEFT ||
-       event->key_code() == ui::VKEY_RIGHT) &&
-      ShouldUseTabletModeGridLayout();
-  if (event->type() != ui::ET_KEY_PRESSED && !process_released_key_event)
+  // Check if we can scroll with the event first as it can use release events as
+  // well.
+  if (ProcessForScrolling(*event)) {
+    event->SetHandled();
+    event->StopPropagation();
+    return;
+  }
+
+  if (event->type() != ui::ET_KEY_PRESSED)
     return;
 
   switch (event->key_code()) {
@@ -930,10 +934,8 @@ void OverviewSession::OnKeyEvent(ui::KeyEvent* event) {
       Move(/*reverse=*/false);
       break;
     case ui::VKEY_RIGHT:
-      if (!ProcessForScrolling(*event)) {
-        ++num_key_presses_;
-        Move(/*reverse=*/false);
-      }
+      ++num_key_presses_;
+      Move(/*reverse=*/false);
       break;
     case ui::VKEY_TAB: {
       const bool reverse = event->flags() & ui::EF_SHIFT_DOWN;
@@ -942,10 +944,8 @@ void OverviewSession::OnKeyEvent(ui::KeyEvent* event) {
       break;
     }
     case ui::VKEY_LEFT:
-      if (!ProcessForScrolling(*event)) {
-        ++num_key_presses_;
-        Move(/*reverse=*/true);
-      }
+      ++num_key_presses_;
+      Move(/*reverse=*/true);
       break;
     case ui::VKEY_W: {
       if (!(event->flags() & ui::EF_CONTROL_DOWN))
@@ -1004,33 +1004,41 @@ void OverviewSession::Move(bool reverse) {
 }
 
 bool OverviewSession::ProcessForScrolling(const ui::KeyEvent& event) {
-  if (!ShouldUseTabletModeGridLayout() ||
-      !(event.flags() & ui::EF_CONTROL_DOWN)) {
+  if (!ShouldUseTabletModeGridLayout())
     return false;
-  }
-
-  const bool press = (event.type() == ui::ET_KEY_PRESSED);
-  const bool repeat = event.is_repeat();
-  DCHECK(event.key_code() == ui::VKEY_LEFT ||
-         event.key_code() == ui::VKEY_RIGHT);
-  const bool reverse = event.key_code() == ui::VKEY_LEFT;
 
   // TODO(sammiequon): This only works for tablet mode at the moment, so using
   // the primary display works. If this feature is adapted for multi display
   // then this needs to be revisited.
   auto* grid = GetGridWithRootWindow(Shell::GetPrimaryRootWindow());
-  if (press && !repeat) {
-    grid->StartScroll();
-    grid->UpdateScrollOffset(kKeyboardPressScrollingDp * (reverse ? 1 : -1));
-    return true;
+  const bool press = (event.type() == ui::ET_KEY_PRESSED);
+
+  if (!press) {
+    if (is_keyboard_scrolling_grid_) {
+      is_keyboard_scrolling_grid_ = false;
+      grid->EndScroll();
+      return true;
+    }
+    return false;
   }
 
-  if (press && repeat) {
-    grid->UpdateScrollOffset(kKeyboardHoldScrollingDp * (reverse ? 1 : -1));
+  // Presses only at this point.
+  if (event.key_code() != ui::VKEY_LEFT && event.key_code() != ui::VKEY_RIGHT)
+    return false;
+
+  if (!event.IsControlDown())
+    return false;
+
+  const bool repeat = event.is_repeat();
+  const bool reverse = event.key_code() == ui::VKEY_LEFT;
+  if (!repeat) {
+    is_keyboard_scrolling_grid_ = true;
+    grid->StartScroll();
+    grid->UpdateScrollOffset(kKeyboardPressScrollingDp * (reverse ? 1 : -1));
     return true;
   }
 
-  grid->EndScroll();
+  grid->UpdateScrollOffset(kKeyboardHoldScrollingDp * (reverse ? 1 : -1));
   return true;
 }
 

ash/wm/overview/overview_session.h

@@ -410,6 +410,10 @@ class ASH_EXPORT OverviewSession : public display::DisplayObserver,
   // The number of items in the overview.
   size_t num_items_ = 0;
 
+  // True if we are currently using keyboard (control + left/right) to scroll
+  // through the grid.
+  bool is_keyboard_scrolling_grid_ = false;
+
   // Stores the overview enter/exit type. See the enum declaration for
   // information on how these types affect overview mode.
   EnterExitOverviewType enter_exit_overview_type_ =