cros: Window cycle list mouse hover only moves selector.

With the InteractiveWindowCycleList flag enabled, hovering an item in
the window cycle list scrolls the list to that item and moves the
selector over it. This causes an issue if the window cycle list extends
past the screen and a user hovers over an item on one of the ends of
the list.

To address this issue, this CL makes it so hovering an item only moves
the selector.

Test: Manual
Bug: 1067327
Change-Id: Iaba916d7e24391ea8cff6d82c4a405c1e89ccd75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2409604
Commit-Queue: Jeremy Chinsen <chinsenj@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807622}

ash/wm/window_cycle_controller_unittest.cc

@@ -899,39 +899,55 @@ TEST_F(InteractiveWindowCycleControllerTest,
 }
 
 // When a user hovers their mouse over an item, it should cycle to it.
+// The items in the list should not move, only the focus ring.
 // If a user clicks on an item, it should complete cycling and activate
 // the hovered item.
 TEST_F(InteractiveWindowCycleControllerTest, MouseHoverAndSelect) {
   std::unique_ptr<Window> w0 = CreateTestWindow();
   std::unique_ptr<Window> w1 = CreateTestWindow();
   std::unique_ptr<Window> w2 = CreateTestWindow();
+  std::unique_ptr<Window> w3 = CreateTestWindow();
+  std::unique_ptr<Window> w4 = CreateTestWindow();
+  std::unique_ptr<Window> w5 = CreateTestWindow();
+  std::unique_ptr<Window> w6 = CreateTestWindow();
   ui::test::EventGenerator* generator = GetEventGenerator();
   WindowCycleController* controller = Shell::Get()->window_cycle_controller();
 
   // Cycle to the third item, mouse over second item, and release alt-tab.
-  // Starting order of windows in cycle list is [2,1,0].
+  // Starting order of windows in cycle list is [6,5,4,3,2,1,0].
   controller->HandleCycleWindow(WindowCycleController::FORWARD);
   controller->HandleCycleWindow(WindowCycleController::FORWARD);
-  generator->MoveMouseTo(
-      GetWindowCycleItemViews()[1]->GetBoundsInScreen().CenterPoint());
+  gfx::Point target_item_center =
+      GetWindowCycleItemViews()[1]->GetBoundsInScreen().CenterPoint();
+  generator->MoveMouseTo(target_item_center);
+  EXPECT_EQ(target_item_center,
+            GetWindowCycleItemViews()[1]->GetBoundsInScreen().CenterPoint());
   controller->CompleteCycling();
-  EXPECT_TRUE(wm::IsActiveWindow(w1.get()));
+  EXPECT_TRUE(wm::IsActiveWindow(w5.get()));
 
   // Start cycle, mouse over third item, and release alt-tab.
-  // Starting order of windows in cycle list is [1,2,0].
+  // Starting order of windows in cycle list is [5,6,4,3,2,1,0].
   controller->StartCycling();
-  generator->MoveMouseTo(
-      GetWindowCycleItemViews()[2]->GetBoundsInScreen().CenterPoint());
+  target_item_center =
+      GetWindowCycleItemViews()[2]->GetBoundsInScreen().CenterPoint();
+  generator->MoveMouseTo(target_item_center);
+  EXPECT_EQ(target_item_center,
+            GetWindowCycleItemViews()[2]->GetBoundsInScreen().CenterPoint());
   controller->CompleteCycling();
-  EXPECT_TRUE(wm::IsActiveWindow(w0.get()));
+  EXPECT_TRUE(wm::IsActiveWindow(w4.get()));
 
-  // Start cycle, mouse over second item, and click.
-  // Starting order of windows in cycle list is [0,1,2].
+  // Start cycle, cycle to the fifth item, mouse over seventh item, and click.
+  // Starting order of windows in cycle list is [4,5,6,3,2,1,0].
   controller->StartCycling();
-  generator->MoveMouseTo(
-      GetWindowCycleItemViews()[1]->GetBoundsInScreen().CenterPoint());
+  for (int i = 0; i < 5; i++)
+    controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  target_item_center =
+      GetWindowCycleItemViews()[6]->GetBoundsInScreen().CenterPoint();
+  generator->MoveMouseTo(target_item_center);
+  EXPECT_EQ(target_item_center,
+            GetWindowCycleItemViews()[6]->GetBoundsInScreen().CenterPoint());
   generator->PressLeftButton();
-  EXPECT_TRUE(wm::IsActiveWindow(w1.get()));
+  EXPECT_TRUE(wm::IsActiveWindow(w0.get()));
 }
 
 // Tests that the left and right keys cycle after the cycle list has been

ash/wm/window_cycle_list.cc

@@ -313,7 +313,7 @@ class WindowCycleView : public views::WidgetDelegateView,
     layer()->SetOpacity(1.f);
   }
 
-  void SetTargetWindow(aura::Window* target) {
+  void SetTargetWindow(aura::Window* target, bool should_layout) {
     // Hide the focus border of the previous target window and show the focus
     // border of the new one.
     if (target_window_) {
@@ -326,7 +326,7 @@ class WindowCycleView : public views::WidgetDelegateView,
     if (target_it != window_view_map_.end())
       target_it->second->UpdateBorderState(/*show=*/true);
 
-    if (GetWidget()) {
+    if (GetWidget() && should_layout) {
       Layout();
       if (target_window_)
         window_view_map_[target_window_]->RequestFocus();
@@ -348,7 +348,7 @@ class WindowCycleView : public views::WidgetDelegateView,
     // sure our own Layout() works correctly when it's calculating highlight
     // bounds.
     parent->Layout();
-    SetTargetWindow(new_target);
+    SetTargetWindow(new_target, /*should_layout=*/true);
   }
 
   void DestroyContents() {
@@ -514,45 +514,9 @@ WindowCycleList::~WindowCycleList() {
   Shell::Get()->frame_throttling_controller()->EndThrottling();
 }
 
-void WindowCycleList::Step(int offset) {
-  if (windows_.empty())
-    return;
-
-  // When there is only one window, we should give feedback to the user. If
-  // the window is minimized, we should also show it.
-  if (windows_.size() == 1) {
-    ::wm::AnimateWindow(windows_[0], ::wm::WINDOW_ANIMATION_TYPE_BOUNCE);
-    SelectWindow(windows_[0]);
-    return;
-  }
-
-  DCHECK(static_cast<size_t>(current_index_) < windows_.size());
-
-  if (!cycle_view_ && current_index_ == 0) {
-    // Special case the situation where we're cycling forward but the MRU
-    // window is not active. This occurs when all windows are minimized. The
-    // starting window should be the first one rather than the second.
-    if (offset == 1 && !wm::IsActiveWindow(windows_[0]))
-      current_index_ = -1;
-  }
-
-  current_index_ += offset;
-
-  // Wrap to window list size.
-  current_index_ = (current_index_ + windows_.size()) % windows_.size();
-  DCHECK(windows_[current_index_]);
-
-  if (ShouldShowUi()) {
-    if (current_index_ > 1)
-      InitWindowCycleView();
-
-    if (cycle_view_)
-      cycle_view_->SetTargetWindow(windows_[current_index_]);
-  }
-}
-
 void WindowCycleList::Step(WindowCycleController::Direction direction) {
-  Step(direction == WindowCycleController::FORWARD ? 1 : -1);
+  Step(direction == WindowCycleController::FORWARD ? 1 : -1,
+       /*should_layout=*/true);
 }
 
 void WindowCycleList::StepToWindow(aura::Window* window) {
@@ -560,7 +524,10 @@ void WindowCycleList::StepToWindow(aura::Window* window) {
   DCHECK(target_window != windows_.end());
   int target_index = std::distance(windows_.begin(), target_window);
 
-  Step(target_index - current_index_);
+  // If the user hovers over an item and the window cycle list scrolls,
+  // sometimes it can cause the mouse to not hover over the selected item. To
+  // avoid this, prevent scrolling and only move focus border.
+  Step(target_index - current_index_, /*should_layout=*/false);
 }
 
 bool WindowCycleList::IsEventInCycleView(ui::LocatedEvent* event) {
@@ -627,7 +594,8 @@ void WindowCycleList::InitWindowCycleView() {
     return;
 
   cycle_view_ = new WindowCycleView(windows_);
-  cycle_view_->SetTargetWindow(windows_[current_index_]);
+  cycle_view_->SetTargetWindow(windows_[current_index_],
+                               /*should_layout=*/true);
 
   // We need to activate the widget if ChromeVox is enabled as ChromeVox
   // relies on activation.
@@ -698,6 +666,44 @@ void WindowCycleList::SelectWindow(aura::Window* window) {
   window_selected_ = true;
 }
 
+void WindowCycleList::Step(int offset, bool should_layout) {
+  if (windows_.empty())
+    return;
+
+  // When there is only one window, we should give feedback to the user. If
+  // the window is minimized, we should also show it.
+  if (windows_.size() == 1) {
+    ::wm::AnimateWindow(windows_[0], ::wm::WINDOW_ANIMATION_TYPE_BOUNCE);
+    SelectWindow(windows_[0]);
+    return;
+  }
+
+  DCHECK(static_cast<size_t>(current_index_) < windows_.size());
+
+  if (!cycle_view_ && current_index_ == 0) {
+    // Special case the situation where we're cycling forward but the MRU
+    // window is not active. This occurs when all windows are minimized. The
+    // starting window should be the first one rather than the second.
+    if (offset == 1 && !wm::IsActiveWindow(windows_[0]))
+      current_index_ = -1;
+  }
+
+  current_index_ += offset;
+
+  // Wrap to window list size.
+  current_index_ = (current_index_ + windows_.size()) % windows_.size();
+  DCHECK(windows_[current_index_]);
+
+  if (ShouldShowUi()) {
+    if (current_index_ > 1)
+      InitWindowCycleView();
+
+    if (cycle_view_)
+      cycle_view_->SetTargetWindow(windows_[current_index_],
+                                   /*should_layout=*/should_layout);
+  }
+}
+
 const views::View::Views& WindowCycleList::GetWindowCycleItemViewsForTesting()
     const {
   return cycle_view_->GetPreviewViewsForTesting();

ash/wm/window_cycle_list.h

@@ -42,10 +42,12 @@ class ASH_EXPORT WindowCycleList : public aura::WindowObserver,
   WindowCycleList& operator=(const WindowCycleList&) = delete;
   ~WindowCycleList() override;
 
-  // Cycles to the next or previous window based on |direction|.
+  // Cycles to the next or previous window based on |direction| and re-layouts
+  // the window cycle list, scrolling the list.
   void Step(WindowCycleController::Direction direction);
 
-  // Skip window cycle list directly to |window|.
+  // Skip window cycle list directly to |window| and don't re-layout the window
+  // cycle list, only moving the focus ring.
   void StepToWindow(aura::Window* window);
 
   // Checks whether |event| occurs within the cycle view. Returns false if
@@ -85,8 +87,12 @@ class ASH_EXPORT WindowCycleList : public aura::WindowObserver,
   // PIP.
   void SelectWindow(aura::Window* window);
 
-  // Cycles windows by |offset|.
-  void Step(int offset);
+  // Cycles windows by |offset|. If |should_layout|, layouts the window cycle
+  // list and moves the focus border to the newly selected window. If not
+  // |should_layout|, just moves the focus border to the newly selected window.
+  // Should be called with |should_layout| if we want the Step() call to animate
+  // the window cycle list, scrolling it.
+  void Step(int offset, bool should_layout);
 
   // Returns the views for the window cycle list.
   const views::View::Views& GetWindowCycleItemViewsForTesting() const;