cros: Refactor window cycle list to decouple scrolling and focus ring.

Currently the logic that handles the focus ring and scrolling the
window cycle list are coupled together. However, there are cases where
the focus ring should move and the list should not scroll and cases
where the focus ring shouldn't move and the list should scroll.

To handle the aforementioned cases, this CL separates the handling for
scrolling and the focus ring.

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

ash/wm/window_cycle_controller.cc

@@ -91,6 +91,17 @@ void WindowCycleController::HandleCycleWindow(Direction direction) {
   Step(direction);
 }
 
+void WindowCycleController::Scroll(Direction direction) {
+  if (!CanCycle())
+    return;
+
+  if (!IsCycling())
+    StartCycling();
+
+  DCHECK(window_cycle_list_);
+  window_cycle_list_->ScrollInDirection(direction);
+}
+
 void WindowCycleController::StartCycling() {
   // Close the wallpaper preview if it is open to prevent visual glitches where
   // the window view item for the preview is transparent
@@ -127,9 +138,12 @@ void WindowCycleController::MaybeResetCycleList() {
   window_cycle_list_->ReplaceWindows(window_list);
 }
 
-void WindowCycleController::StepToWindow(aura::Window* window) {
+void WindowCycleController::SetFocusedWindow(aura::Window* window) {
+  if (!IsCycling())
+    return;
+
   DCHECK(window_cycle_list_);
-  window_cycle_list_->StepToWindow(window);
+  window_cycle_list_->SetFocusedWindow(window);
 }
 
 bool WindowCycleController::IsEventInCycleView(ui::LocatedEvent* event) {

ash/wm/window_cycle_controller.h

@@ -45,9 +45,14 @@ class ASH_EXPORT WindowCycleController {
   // certain times, such as when the lock screen is visible.
   static bool CanCycle();
 
-  // Cycles between windows in the given |direction|.
+  // Cycles between windows in the given |direction|. This moves the focus ring
+  // to the window in the given |direction| and also scrolls the list.
   void HandleCycleWindow(Direction direction);
 
+  // Scrolls the window in the given |direction|. This does not move the focus
+  // ring.
+  void Scroll(Direction direction);
+
   // Returns true if we are in the middle of a window cycling gesture.
   bool IsCycling() const { return window_cycle_list_.get() != NULL; }
 
@@ -66,8 +71,9 @@ class ASH_EXPORT WindowCycleController {
   // cycling.
   void MaybeResetCycleList();
 
-  // Skip window cycle list directly to |window|.
-  void StepToWindow(aura::Window* window);
+  // Moves the focus ring to |window|. Does not scroll the list. Do nothing if
+  // not cycling.
+  void SetFocusedWindow(aura::Window* window);
 
   // Checks whether |event| occurs within the cycle view.
   bool IsEventInCycleView(ui::LocatedEvent* event);

ash/wm/window_cycle_controller_unittest.cc

@@ -130,6 +130,13 @@ class WindowCycleControllerTest : public AshTestBase {
         ->cycle_view_for_testing();
   }
 
+  int GetCurrentIndex() const {
+    return Shell::Get()
+        ->window_cycle_controller()
+        ->window_cycle_list()
+        ->current_index_for_testing();
+  }
+
  private:
   std::unique_ptr<ShelfViewTestAPI> shelf_view_test_;
 
@@ -288,6 +295,94 @@ TEST_F(WindowCycleControllerTest, HandleCycleWindow) {
   EXPECT_FALSE(wm::IsActiveWindow(window1.get()));
 }
 
+TEST_F(WindowCycleControllerTest, Scroll) {
+  WindowCycleController* controller = Shell::Get()->window_cycle_controller();
+
+  // Doesn't crash if there are no windows.
+  controller->Scroll(WindowCycleController::FORWARD);
+
+  // Create test windows.
+  std::unique_ptr<Window> w5 = CreateTestWindow(gfx::Rect(0, 0, 200, 200));
+  std::unique_ptr<Window> w4 = CreateTestWindow(gfx::Rect(0, 0, 200, 200));
+  std::unique_ptr<Window> w3 = CreateTestWindow(gfx::Rect(0, 0, 200, 200));
+  std::unique_ptr<Window> w2 = CreateTestWindow(gfx::Rect(0, 0, 200, 200));
+  std::unique_ptr<Window> w1 = CreateTestWindow(gfx::Rect(0, 0, 200, 200));
+  std::unique_ptr<Window> w0 = CreateTestWindow(gfx::Rect(0, 0, 200, 200));
+
+  auto ScrollAndReturnCurrentIndex =
+      [this](WindowCycleController::Direction direction, int num_of_scrolls) {
+        WindowCycleController* controller =
+            Shell::Get()->window_cycle_controller();
+        for (int i = 0; i < num_of_scrolls; i++)
+          controller->Scroll(direction);
+
+        return GetCurrentIndex();
+      };
+
+  auto GetXOfCycleListCenterPoint = [this]() {
+    return GetWindowCycleListWidget()
+        ->GetWindowBoundsInScreen()
+        .CenterPoint()
+        .x();
+  };
+
+  auto GetXOfWindowCycleItemViewCenterPoint = [this](int index) {
+    return GetWindowCycleItemViews()[index]
+        ->GetBoundsInScreen()
+        .CenterPoint()
+        .x();
+  };
+
+  // Start cycling and scroll forward. The list should be not be centered around
+  // w1. Since w1 is so close to the beginning of the list.
+  controller->StartCycling();
+  int current_index =
+      ScrollAndReturnCurrentIndex(WindowCycleController::FORWARD, 1);
+  EXPECT_EQ(1, current_index);
+  EXPECT_GT(GetXOfCycleListCenterPoint(),
+            GetXOfWindowCycleItemViewCenterPoint(current_index));
+
+  // Scroll forward twice. The list should be centered around w3.
+  current_index =
+      ScrollAndReturnCurrentIndex(WindowCycleController::FORWARD, 2);
+  EXPECT_EQ(3, current_index);
+  EXPECT_EQ(GetXOfCycleListCenterPoint(),
+            GetXOfWindowCycleItemViewCenterPoint(current_index));
+
+  // Scroll backward once. The list should be centered around w2.
+  current_index =
+      ScrollAndReturnCurrentIndex(WindowCycleController::BACKWARD, 1);
+  EXPECT_EQ(2, current_index);
+  EXPECT_EQ(GetXOfCycleListCenterPoint(),
+            GetXOfWindowCycleItemViewCenterPoint(current_index));
+
+  // Scroll backward three times. The list should not be centered around w5.
+  current_index =
+      ScrollAndReturnCurrentIndex(WindowCycleController::BACKWARD, 3);
+  EXPECT_EQ(5, current_index);
+  EXPECT_LT(GetXOfCycleListCenterPoint(),
+            GetXOfWindowCycleItemViewCenterPoint(current_index));
+
+  // Cycle forward. Since the target window != current window, it should scroll
+  // to target window then cycle. The target_window was w0 prior to cycling.
+  controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  current_index = GetCurrentIndex();
+  EXPECT_EQ(1, current_index);
+  EXPECT_GT(GetXOfCycleListCenterPoint(),
+            GetXOfWindowCycleItemViewCenterPoint(current_index));
+  controller->CompleteCycling();
+  EXPECT_TRUE(wm::IsActiveWindow(w1.get()));
+
+  // Start cycling, scroll backward once and complete cycling. Scroll should not
+  // affect the selected window.
+  controller->StartCycling();
+  current_index =
+      ScrollAndReturnCurrentIndex(WindowCycleController::BACKWARD, 1);
+  EXPECT_EQ(5, current_index);
+  controller->CompleteCycling();
+  EXPECT_TRUE(wm::IsActiveWindow(w1.get()));
+}
+
 // Cycles between a maximized and normal window.
 TEST_F(WindowCycleControllerTest, MaximizedWindow) {
   // Create a couple of test windows.

ash/wm/window_cycle_list.cc

@@ -141,10 +141,11 @@ class WindowCycleItemView : public WindowMiniView {
 
   // views::View:
   void OnMouseEntered(const ui::MouseEvent& event) override {
-    Shell::Get()->window_cycle_controller()->StepToWindow(source_window());
+    Shell::Get()->window_cycle_controller()->SetFocusedWindow(source_window());
   }
 
   bool OnMousePressed(const ui::MouseEvent& event) override {
+    Shell::Get()->window_cycle_controller()->SetFocusedWindow(source_window());
     Shell::Get()->window_cycle_controller()->CompleteCycling();
     return true;
   }
@@ -157,7 +158,7 @@ class WindowCycleItemView : public WindowMiniView {
       case ui::ET_GESTURE_TWO_FINGER_TAP: {
         WindowCycleController* controller =
             Shell::Get()->window_cycle_controller();
-        controller->StepToWindow(source_window());
+        controller->SetFocusedWindow(source_window());
         controller->CompleteCycling();
         break;
       }
@@ -311,7 +312,8 @@ class WindowCycleView : public views::WidgetDelegateView,
     widget_rect.ClampToCenteredSize(GetPreferredSize());
     GetWidget()->SetBounds(widget_rect);
 
-    SetTargetWindow(windows[0], /*should_layout=*/true);
+    SetTargetWindow(windows[0]);
+    ScrollToWindow(windows[0]);
   }
 
   void FadeInLayer() {
@@ -331,7 +333,15 @@ class WindowCycleView : public views::WidgetDelegateView,
     layer()->SetOpacity(1.f);
   }
 
-  void SetTargetWindow(aura::Window* target, bool should_layout) {
+  void ScrollToWindow(aura::Window* target) {
+    current_window_ = target;
+
+    if (GetWidget()) {
+      Layout();
+    }
+  }
+
+  void SetTargetWindow(aura::Window* target) {
     // Hide the focus border of the previous target window and show the focus
     // border of the new one.
     if (target_window_) {
@@ -344,12 +354,9 @@ class WindowCycleView : public views::WidgetDelegateView,
     if (target_it != window_view_map_.end())
       target_it->second->UpdateBorderState(/*show=*/true);
 
-    if (GetWidget() && should_layout) {
-      Layout();
-      if (target_window_)
+    if (GetWidget() && target_window_)
       window_view_map_[target_window_]->RequestFocus();
   }
-  }
 
   void HandleWindowDestruction(aura::Window* destroying_window,
                                aura::Window* new_target) {
@@ -362,17 +369,19 @@ class WindowCycleView : public views::WidgetDelegateView,
     delete preview;
 
     // With one of its children now gone, we must re-layout
-    // |mirror_container_|. This must happen before SetTargetWindow() to make
+    // |mirror_container_|. This must happen before ScrollToWindow() to make
     // sure our own Layout() works correctly when it's calculating highlight
     // bounds.
     parent->Layout();
-    SetTargetWindow(new_target, /*should_layout=*/true);
+    SetTargetWindow(new_target);
+    ScrollToWindow(new_target);
   }
 
   void DestroyContents() {
     window_view_map_.clear();
     no_previews_set_.clear();
     target_window_ = nullptr;
+    current_window_ = nullptr;
     RemoveAllChildViews(true);
   }
 
@@ -382,7 +391,7 @@ class WindowCycleView : public views::WidgetDelegateView,
   }
 
   void Layout() override {
-    if (!target_window_ || bounds().IsEmpty())
+    if (!target_window_ || !current_window_ || bounds().IsEmpty())
       return;
 
     const bool first_layout = mirror_container_->bounds().IsEmpty();
@@ -398,7 +407,7 @@ class WindowCycleView : public views::WidgetDelegateView,
         layer()->SetRoundedCornerRadius(kBackgroundCornerRadius);
     }
 
-    views::View* target_view = window_view_map_[target_window_];
+    views::View* target_view = window_view_map_[current_window_];
     gfx::RectF target_bounds(target_view->GetLocalBounds());
     views::View::ConvertRectToTarget(target_view, mirror_container_,
                                      &target_bounds);
@@ -456,6 +465,8 @@ class WindowCycleView : public views::WidgetDelegateView,
     }
   }
 
+  aura::Window* GetTargetWindow() { return target_window_; }
+
   View* GetInitiallyFocusedView() override {
     return window_view_map_[target_window_];
   }
@@ -472,8 +483,17 @@ class WindowCycleView : public views::WidgetDelegateView,
  private:
   std::map<aura::Window*, WindowCycleItemView*> window_view_map_;
   views::View* mirror_container_ = nullptr;
+
+  // The |target_window_| is the window that has the focus ring. When the user
+  // completes cycling the |target_window_| is activated.
   aura::Window* target_window_ = nullptr;
 
+  // The |current_window_| is the window that the window cycle list uses to
+  // determine the layout and positioning of the list's items. If this window's
+  // preview can equally divide the list it is centered, otherwise it is
+  // off-center.
+  aura::Window* current_window_ = nullptr;
+
   // Set which contains items which have been created but have some of their
   // performance heavy elements not created yet. These elements will be created
   // once onscreen to improve fade in performance, then removed from this set.
@@ -513,20 +533,26 @@ WindowCycleList::~WindowCycleList() {
   if (cycle_ui_widget_)
     cycle_ui_widget_->Close();
 
+  // Store the target window before |cycle_view_| is destroyed.
+  aura::Window* target_window = nullptr;
+
   // |this| is responsible for notifying |cycle_view_| when windows are
   // destroyed. Since |this| is going away, clobber |cycle_view_|. Otherwise
   // there will be a race where a window closes after now but before the
   // Widget::Close() call above actually destroys |cycle_view_|. See
   // crbug.com/681207
-  if (cycle_view_)
+  if (cycle_view_) {
+    target_window = cycle_view_->GetTargetWindow();
     cycle_view_->DestroyContents();
+  }
 
   // While the cycler widget is shown, the windows listed in the cycler is
   // marked as force-visible and don't contribute to occlusion. In order to
   // work occlusion calculation properly, we need to activate a window after
   // the widget has been destroyed. See b/138914552.
   if (!windows_.empty() && user_did_accept_) {
-    auto* target_window = windows_[current_index_];
+    if (!target_window)
+      target_window = windows_[current_index_];
     SelectWindow(target_window);
   }
   Shell::Get()->frame_throttling_controller()->EndThrottling();
@@ -547,19 +573,36 @@ void WindowCycleList::ReplaceWindows(const WindowList& windows) {
 }
 
 void WindowCycleList::Step(WindowCycleController::Direction direction) {
-  Step(direction == WindowCycleController::FORWARD ? 1 : -1,
-       /*should_layout=*/true);
+  if (windows_.empty())
+    return;
+
+  // If the position of the window cycle list is out-of-sync with the currently
+  // selected item, scroll to the selected item and then step.
+  if (cycle_view_) {
+    aura::Window* selected_window = cycle_view_->GetTargetWindow();
+    Scroll(GetIndexOfWindow(selected_window) - current_index_);
+  }
+
+  const int offset = direction == WindowCycleController::FORWARD ? 1 : -1;
+  SetFocusedWindow(windows_[GetOffsettedWindowIndex(offset)]);
+  Scroll(offset);
 }
 
-void WindowCycleList::StepToWindow(aura::Window* window) {
-  auto target_window = std::find(windows_.begin(), windows_.end(), window);
-  DCHECK(target_window != windows_.end());
-  int target_index = std::distance(windows_.begin(), target_window);
+void WindowCycleList::ScrollInDirection(
+    WindowCycleController::Direction direction) {
+  if (windows_.empty())
+    return;
+
+  const int offset = direction == WindowCycleController::FORWARD ? 1 : -1;
+  Scroll(offset);
+}
 
-  // 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);
+void WindowCycleList::SetFocusedWindow(aura::Window* window) {
+  if (windows_.empty())
+    return;
+
+  if (ShouldShowUi() && cycle_view_)
+    cycle_view_->SetTargetWindow(windows_[GetIndexOfWindow(window)]);
 }
 
 bool WindowCycleList::IsEventInCycleView(ui::LocatedEvent* event) {
@@ -632,6 +675,7 @@ void WindowCycleList::RemoveAllWindows() {
 
   windows_.clear();
   current_index_ = 0;
+  window_selected_ = false;
 }
 
 void WindowCycleList::InitWindowCycleView() {
@@ -639,8 +683,8 @@ void WindowCycleList::InitWindowCycleView() {
     return;
 
   cycle_view_ = new WindowCycleView(windows_);
-  cycle_view_->SetTargetWindow(windows_[current_index_],
-                               /*should_layout=*/true);
+  cycle_view_->SetTargetWindow(windows_[current_index_]);
+  cycle_view_->ScrollToWindow(windows_[current_index_]);
 
   // We need to activate the widget if ChromeVox is enabled as ChromeVox
   // relies on activation.
@@ -696,7 +740,7 @@ void WindowCycleList::InitWindowCycleView() {
 
 void WindowCycleList::SelectWindow(aura::Window* window) {
   // If the list has only one window, the window can be selected twice (in
-  // Step() and the destructor). This causes ARC PIP windows to be restored
+  // Scroll() and the destructor). This causes ARC PIP windows to be restored
   // twice, which leads to a wrong window state.
   if (window_selected_)
     return;
@@ -711,7 +755,7 @@ void WindowCycleList::SelectWindow(aura::Window* window) {
   window_selected_ = true;
 }
 
-void WindowCycleList::Step(int offset, bool should_layout) {
+void WindowCycleList::Scroll(int offset) {
   if (windows_.empty())
     return;
 
@@ -733,22 +777,33 @@ void WindowCycleList::Step(int offset, bool should_layout) {
       current_index_ = -1;
   }
 
-  current_index_ += offset;
-
-  // Wrap to window list size.
-  current_index_ = (current_index_ + windows_.size()) % windows_.size();
-  DCHECK(windows_[current_index_]);
+  current_index_ = GetOffsettedWindowIndex(offset);
 
   if (ShouldShowUi()) {
     if (current_index_ > 1)
       InitWindowCycleView();
 
     if (cycle_view_)
-      cycle_view_->SetTargetWindow(windows_[current_index_],
-                                   /*should_layout=*/should_layout);
+      cycle_view_->ScrollToWindow(windows_[current_index_]);
   }
 }
 
+int WindowCycleList::GetIndexOfWindow(aura::Window* window) const {
+  auto target_window = std::find(windows_.begin(), windows_.end(), window);
+  DCHECK(target_window != windows_.end());
+  return std::distance(windows_.begin(), target_window);
+}
+
+int WindowCycleList::GetOffsettedWindowIndex(int offset) const {
+  DCHECK(!windows_.empty());
+
+  const int offsetted_index =
+      (current_index_ + offset + windows_.size()) % windows_.size();
+  DCHECK(windows_[offsetted_index]);
+
+  return offsetted_index;
+}
+
 const views::View::Views& WindowCycleList::GetWindowCycleItemViewsForTesting()
     const {
   return cycle_view_->GetPreviewViewsForTesting();

ash/wm/window_cycle_list.h

@@ -46,13 +46,16 @@ class ASH_EXPORT WindowCycleList : public aura::WindowObserver,
   // |windows| is empty, cancels cycling.
   void ReplaceWindows(const WindowList& windows);
 
-  // Cycles to the next or previous window based on |direction| and re-layouts
-  // the window cycle list, scrolling the list.
+  // Cycles to the next or previous window based on |direction|. This moves the
+  // focus ring to the next/previous window and also scrolls the list.
   void Step(WindowCycleController::Direction direction);
 
-  // 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);
+  // Scrolls windows in given |direction|. Does not move the focus ring.
+  void ScrollInDirection(WindowCycleController::Direction direction);
+
+  // Moves the focus ring to the respective preview for |window|. Does not
+  // scroll the window cycle list.
+  void SetFocusedWindow(aura::Window* window);
 
   // Checks whether |event| occurs within the cycle view. Returns false if
   // |cycle_view_| does not exist.
@@ -95,18 +98,27 @@ class ASH_EXPORT WindowCycleList : public aura::WindowObserver,
   // PIP.
   void SelectWindow(aura::Window* window);
 
-  // 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);
+  // Scrolls windows by |offset|. Does not move the focus ring. If you want to
+  // scroll the list and move the focus ring in one animation, call
+  // SetFocusedWindow() before this.
+  void Scroll(int offset);
+
+  // Returns the index for the window |offset| away from |current_index_|. Can
+  // only be called if |windows_| is not empty. Also checks that the window for
+  // the returned index exists.
+  int GetOffsettedWindowIndex(int offset) const;
+
+  // Returns the index for |window| in |windows_|. |window| must be in
+  // |windows_|.
+  int GetIndexOfWindow(aura::Window* window) const;
 
   // Returns the views for the window cycle list.
   const views::View::Views& GetWindowCycleItemViewsForTesting() const;
 
   WindowCycleView* cycle_view_for_testing() const { return cycle_view_; }
 
+  int current_index_for_testing() const { return current_index_; }
+
   // List of weak pointers to windows to use while cycling with the keyboard.
   // List is built when the user initiates the gesture (i.e. hits alt-tab the
   // first time) and is emptied when the gesture is complete (i.e. releases the