Bento: AltTabMode: Update highlighted window after switching mode.

Regardless of where the highlight position is, after switching
alt-tab mode, the highlight will be reset to the first or second
window. This is consistent with highlighting when users first
enter alt-tab mode:
- In general case, highlight the second (next) most recently used
window after the current one.
- In the special case that the most recently used is minimized,
highlight itself (the MRU window), so tabbing into it causes it
to unminimize rather than opening up the next window in the list.

- Track mode switching state in the controller to differentiate
between normal tab pressing and mode switching.
- Add Ash unit tests for both cases.

`ash_unittests --gtest_filter=ModeSelectionWindowCycleControllerTest.SwitchingModeUpdates*WindowHighlight`

Bug: 1157100
Test: Manual test (a video uploaded to crbug) and ash unit tests
Change-Id: Id85b389ba9d59eb1815b8250d640e5aa9128240b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2604907Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Min Chen <minch@chromium.org>
Commit-Queue: Cattalyya Nuengsigkapian <cattalyya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840348}

ash/wm/window_cycle_controller.cc

@@ -89,8 +89,6 @@ void WindowCycleController::HandleCycleWindow(Direction direction) {
   if (!IsCycling())
     StartCycling();
 
-  // TODO(crbug.com/1157100): Handle window highlighting after switching
-  // alt-tab mode.
   Step(direction);
 }
 
@@ -175,10 +173,23 @@ WindowCycleController::WindowList WindowCycleController::CreateWindowList() {
 }
 
 void WindowCycleController::SetAltTabMode(DesksMruType alt_tab_mode) {
+  DCHECK(features::IsBentoEnabled());
   if (alt_tab_mode_ == alt_tab_mode)
     return;
   alt_tab_mode_ = alt_tab_mode;
   MaybeResetCycleList();
+
+  is_switching_mode_ = true;
+  // When user first press alt + tab, `HandleCycleForwardMRU` triggers
+  // `HandleCycleWindow(WindowCycleController::FORWARD)` since it considers
+  // the initial tab as forward cycling. Therefore, switching the mode
+  // should imitate the same forward cycling behavior after the cycle is reset.
+  HandleCycleWindow(WindowCycleController::FORWARD);
+  is_switching_mode_ = false;
+}
+
+bool WindowCycleController::IsSwitchingMode() {
+  return features::IsBentoEnabled() && is_switching_mode_;
 }
 
 bool WindowCycleController::IsAltTabPerActiveDesk() {

ash/wm/window_cycle_controller.h

@@ -95,6 +95,12 @@ class ASH_EXPORT WindowCycleController {
   // to all desk unless LimitAltTabToActiveDesk flag is explicitly enabled.
   bool IsAltTabPerActiveDesk();
 
+  // Returns true while switching the alt-tab mode and Bento flag is enabled.
+  // This helps `Scroll()` and `Step()` distinguish between pressing tabs and
+  // switching mode, so they refresh |current_index_| and the highlighted
+  // window correctly.
+  bool IsSwitchingMode();
+
  private:
   // Gets a list of windows from the currently open windows, removing windows
   // with transient roots already in the list. The returned list of windows
@@ -130,6 +136,9 @@ class ASH_EXPORT WindowCycleController {
   // TODO(crbug.com/1157105): Store per-desk mode in primary user pref.
   DesksMruType alt_tab_mode_ = DesksMruType::kAllDesks;
 
+  // Tracks whether alt-tab mode is currently switching or not.
+  bool is_switching_mode_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(WindowCycleController);
 };
 

ash/wm/window_cycle_controller_unittest.cc

@@ -1538,4 +1538,132 @@ TEST_F(ModeSelectionWindowCycleControllerTest, NoWindowInActiveDesk) {
   waiter.Wait();
 }
 
+// Tests that switching between modes correctly reset the alt-tab-highlighted
+// window to the second most recently used window, i.e. the next window to tab
+// into from the currently used window. Since the window cycle list is ordered
+// by MRU, such window is therefore the second window in the MRU list.
+TEST_F(ModeSelectionWindowCycleControllerTest,
+       SwitchingModeUpdatesWindowHighlight) {
+  WindowCycleController* cycle_controller =
+      Shell::Get()->window_cycle_controller();
+
+  // Create two windows for desk1 and three windows for desk2 in the reversed
+  // order of the most recently active window.
+  auto win4 = CreateAppWindow(gfx::Rect(0, 0, 250, 100));
+  auto win3 = CreateAppWindow(gfx::Rect(50, 50, 200, 200));
+  auto* desks_controller = DesksController::Get();
+  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
+  ASSERT_EQ(2u, desks_controller->desks().size());
+  const Desk* desk_2 = desks_controller->desks()[1].get();
+  ActivateDesk(desk_2);
+  EXPECT_EQ(desk_2, desks_controller->active_desk());
+  auto win2 = CreateAppWindow(gfx::Rect(0, 0, 300, 200));
+  auto win1 = CreateAppWindow(gfx::Rect(10, 30, 400, 200));
+  auto win0 = CreateAppWindow(gfx::Rect(10, 30, 400, 200));
+
+  // Enter the all-desk mode by default with the window order [0, 1, 2, 3 ,4].
+  cycle_controller->StartCycling();
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+
+  EXPECT_FALSE(cycle_controller->IsAltTabPerActiveDesk());
+  auto cycle_windows = GetWindows(cycle_controller);
+  // The window list is MRU ordered.
+  EXPECT_EQ(win0.get(), cycle_windows[0]);
+  EXPECT_EQ(win1.get(), cycle_windows[1]);
+  EXPECT_EQ(win2.get(), cycle_windows[2]);
+  EXPECT_EQ(win3.get(), cycle_windows[3]);
+  EXPECT_EQ(win4.get(), cycle_windows[4]);
+  // Alt-Tab should highlight the second most recently used window, which is
+  // the second window in the MRU list, win1.
+  EXPECT_EQ(win1.get(), GetTargetWindow());
+
+  // Step to win2 and win3, so we are now select a window in a non-active desk.
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  EXPECT_EQ(win2.get(), GetTargetWindow());
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  EXPECT_EQ(win3.get(), GetTargetWindow());
+
+  // Switching from the all-desks mode, which highlights a non-current-desk
+  // window to the current-desk mode [0, 1, 2] should resolve the highlight
+  // correctly to win1, the second window in the cycle list.
+  SwitchPerDeskAltTabMode(true);
+  EXPECT_EQ(win1.get(), GetTargetWindow());
+  EXPECT_EQ(win1.get(), cycle_windows[1]);
+  // Step to win2.
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  EXPECT_EQ(win2.get(), GetTargetWindow());
+
+  // Switching back to the all-desk mode should reset highlight to win1 again.
+  SwitchPerDeskAltTabMode(false);
+  EXPECT_EQ(win1.get(), GetTargetWindow());
+  cycle_controller->CompleteCycling();
+}
+
+// Similar to `SwitchingModeUpdatesWindowHighlight`, tests that switching the
+// alt-tab mode updates the highlighted window to the first window (most
+// recently used) in the special case where all windows are minimized.
+// When they are minimized, cycling forward should help unminimize the most
+// recently used window rather than trying to open the second most recently
+// used window.
+TEST_F(ModeSelectionWindowCycleControllerTest,
+       SwitchingModeUpdatesMinimizedWindowHighlight) {
+  WindowCycleController* cycle_controller =
+      Shell::Get()->window_cycle_controller();
+
+  // Create two windows for desk1 and three windows for desk2 in the reversed
+  // order of the most recently active window.
+  auto win4 = CreateAppWindow(gfx::Rect(0, 0, 250, 100));
+  auto win3 = CreateAppWindow(gfx::Rect(50, 50, 200, 200));
+  auto* desks_controller = DesksController::Get();
+  desks_controller->NewDesk(DesksCreationRemovalSource::kButton);
+  ASSERT_EQ(2u, desks_controller->desks().size());
+  const Desk* desk_2 = desks_controller->desks()[1].get();
+  ActivateDesk(desk_2);
+  EXPECT_EQ(desk_2, desks_controller->active_desk());
+  auto win2 = CreateAppWindow(gfx::Rect(0, 0, 300, 200));
+  auto win1 = CreateAppWindow(gfx::Rect(10, 30, 400, 200));
+  auto win0 = CreateAppWindow(gfx::Rect(10, 30, 400, 200));
+
+  // Minimize all windows to test this special case.
+  WindowState::Get(win4.get())->Minimize();
+  WindowState::Get(win3.get())->Minimize();
+  WindowState::Get(win2.get())->Minimize();
+  WindowState::Get(win1.get())->Minimize();
+  WindowState::Get(win0.get())->Minimize();
+  EXPECT_FALSE(WindowState::Get(win0.get())->IsActive());
+  EXPECT_FALSE(WindowState::Get(win1.get())->IsActive());
+  EXPECT_FALSE(WindowState::Get(win2.get())->IsActive());
+  EXPECT_FALSE(WindowState::Get(win3.get())->IsActive());
+  EXPECT_FALSE(WindowState::Get(win4.get())->IsActive());
+
+  // Enter the all-desk mode by default with the window order [0, 1, 2, 3 ,4].
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  EXPECT_FALSE(cycle_controller->IsAltTabPerActiveDesk());
+  auto cycle_windows = GetWindows(cycle_controller);
+  EXPECT_EQ(5u, GetWindowCycleItemViews().size());
+  // The window list is MRU ordered.
+  EXPECT_EQ(win0.get(), cycle_windows[0]);
+  EXPECT_EQ(win1.get(), cycle_windows[1]);
+  EXPECT_EQ(win2.get(), cycle_windows[2]);
+  EXPECT_EQ(win3.get(), cycle_windows[3]);
+  EXPECT_EQ(win4.get(), cycle_windows[4]);
+  // Step forward a few times and switch to all-desks mode. This should
+  // highlight win0, the first window in the current-desk cycle list.
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  SwitchPerDeskAltTabMode(true);
+  EXPECT_EQ(3u, GetWindowCycleItemViews().size());
+  EXPECT_EQ(win0.get(), GetTargetWindow());
+  EXPECT_EQ(win0.get(), cycle_windows[0]);
+
+  // Stepping to win1 and switching back to the all-desk mode should reset
+  // a highlight to win0 again.
+  cycle_controller->HandleCycleWindow(WindowCycleController::FORWARD);
+  EXPECT_EQ(win1.get(), GetTargetWindow());
+  SwitchPerDeskAltTabMode(false);
+  EXPECT_EQ(5u, GetWindowCycleItemViews().size());
+  EXPECT_EQ(win0.get(), GetTargetWindow());
+  cycle_controller->CompleteCycling();
+}
+
 }  // namespace ash

ash/wm/window_cycle_list.cc

@@ -691,7 +691,16 @@ void WindowCycleList::Step(WindowCycleController::Direction direction) {
   }
 
   const int offset = direction == WindowCycleController::FORWARD ? 1 : -1;
-  SetFocusedWindow(windows_[GetOffsettedWindowIndex(offset)]);
+  if (offset == 1 && !wm::IsActiveWindow(windows_[0]) &&
+      Shell::Get()->window_cycle_controller()->IsSwitchingMode()) {
+    // Similar to `WindowCycleList::Scroll()`, when switching to alt-tab mode,
+    // if all windows are minimized, the starting window should be the first
+    // one rather than the second. Note that during entering alt-tab mode,
+    // `SetFocusedWindow()` does nothing, so we don't need to prevent it here.
+    SetFocusedWindow(windows_[0]);
+  } else {
+    SetFocusedWindow(windows_[GetOffsettedWindowIndex(offset)]);
+  }
   Scroll(offset);
 }
 
@@ -876,16 +885,17 @@ void WindowCycleList::Scroll(int offset) {
 
   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;
+  // If alt-tab is entered or switched to the other mode, check the following
+  // special case: user is 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 ((!cycle_view_ ||
+       Shell::Get()->window_cycle_controller()->IsSwitchingMode()) &&
+      current_index_ == 0 && offset == 1 && !wm::IsActiveWindow(windows_[0])) {
+    current_index_ = -1;
   }
 
   current_index_ = GetOffsettedWindowIndex(offset);
-
   if (ShouldShowUi()) {
     if (current_index_ > 1)
       InitWindowCycleView();