Cancel Overview first when activation changes

* WindowSelectorObserver listens the activation change first so that
  it can cancel the overview before resuming minimized window.
* This also fixes a crash that happens if you click the overview button
 while activating a window in overview.
* This also fixes the bug in WindowSelectorController was calling wrong
  "Starting/EndingAnimation". This happens to work fine
  because the end results are the same.

Bug: 905813
Test: covered by unit test. WindowSelectorControllerTest::AnimationCallbacks
Change-Id: I2b3d3fdc2e68bb41c0d037d46552cf6cfda98746
Reviewed-on: https://chromium-review.googlesource.com/c/1338175
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608930}

ash/shell.cc

@@ -675,7 +675,6 @@ Shell::Shell(std::unique_ptr<ShellDelegate> shell_delegate,
       system_tray_notifier_(std::make_unique<SystemTrayNotifier>()),
       vpn_list_(std::make_unique<VpnList>()),
       window_cycle_controller_(std::make_unique<WindowCycleController>()),
-      window_selector_controller_(std::make_unique<WindowSelectorController>()),
       display_configurator_(std::make_unique<display::DisplayConfigurator>()),
       display_output_protection_(std::make_unique<DisplayOutputProtection>(
           display_configurator_.get())),
@@ -1063,7 +1062,9 @@ void Shell::Init(
       std::make_unique<::wm::FocusController>(new wm::AshFocusRules());
   focus_controller_->AddObserver(this);
 
-  screen_position_controller_.reset(new ScreenPositionController);
+  window_selector_controller_ = std::make_unique<WindowSelectorController>();
+
+  screen_position_controller_ = std::make_unique<ScreenPositionController>();
 
   // Must be before CreatePrimaryHost().
   window_service_owner_ =

ash/system/overview/overview_button_tray.cc

@@ -75,15 +75,13 @@ bool OverviewButtonTray::PerformAction(const ui::Event& event) {
   DCHECK(event.type() == ui::ET_MOUSE_RELEASED ||
          event.type() == ui::ET_GESTURE_TAP);
 
-  if (last_press_event_time_ &&
+  WindowSelectorController* window_selector_controller =
+      Shell::Get()->window_selector_controller();
+  // Skip if the second tap happened outside of overview. This can happen if a
+  // window gets activated in between, which cancels overview mode.
+  if (window_selector_controller->IsSelecting() && last_press_event_time_ &&
       event.time_stamp() - last_press_event_time_.value() <
           kDoubleTapThresholdMs) {
-    // Second taps should not be processed outside of overview mode. (First
-    // taps should be outside of overview).
-    WindowSelectorController* window_selector_controller =
-        Shell::Get()->window_selector_controller();
-    DCHECK(window_selector_controller->IsSelecting());
-
     base::RecordAction(base::UserMetricsAction("Tablet_QuickSwitch"));
 
     // Build mru window list. Use cycle as it excludes some windows we are not

ash/wm/overview/window_selector.cc

@@ -53,7 +53,6 @@
 #include "ui/views/widget/widget_delegate.h"
 #include "ui/wm/core/coordinate_conversion.h"
 #include "ui/wm/core/window_util.h"
-#include "ui/wm/public/activation_client.h"
 
 namespace ash {
 
@@ -379,7 +378,6 @@ void WindowSelector::Init(const WindowList& windows,
 
   UMA_HISTOGRAM_COUNTS_100("Ash.WindowSelector.Items", num_items_);
 
-  Shell::Get()->activation_client()->AddObserver(this);
   Shell::Get()->split_view_controller()->AddObserver(this);
 
   display::Screen::GetScreen()->AddObserver(this);
@@ -389,6 +387,8 @@ void WindowSelector::Init(const WindowList& windows,
       mojom::AccessibilityAlert::WINDOW_OVERVIEW_MODE_ENTERED);
 
   UpdateShelfVisibility();
+
+  ignore_activations_ = false;
 }
 
 // NOTE: The work done in Shutdown() is not done in the destructor because it
@@ -769,6 +769,48 @@ bool WindowSelector::IsWindowGridAnimating() {
   return false;
 }
 
+void WindowSelector::OnWindowActivating(
+    ::wm::ActivationChangeObserver::ActivationReason reason,
+    aura::Window* gained_active,
+    aura::Window* lost_active) {
+  if (ignore_activations_ || !gained_active ||
+      gained_active == GetTextFilterWidgetWindow()) {
+    return;
+  }
+
+  auto* grid = GetGridWithRootWindow(gained_active->GetRootWindow());
+  if (!grid)
+    return;
+  const auto& windows = grid->window_list();
+
+  auto iter = std::find_if(
+      windows.begin(), windows.end(),
+      [gained_active](const std::unique_ptr<WindowSelectorItem>& window) {
+        return window->Contains(gained_active);
+      });
+
+  if (iter == windows.end() && showing_text_filter_ &&
+      lost_active == GetTextFilterWidgetWindow()) {
+    return;
+  }
+
+  // Do not cancel overview mode if the window activation was caused by
+  // snapping window to one side of the screen.
+  if (Shell::Get()->IsSplitViewModeActive())
+    return;
+
+  // Do not cancel overview mode if the window activation was caused while
+  // dragging overview mode offscreen.
+  if (IsSlidingOutOverviewFromShelf())
+    return;
+
+  // Don't restore focus on exit if a window was just activated.
+  ResetFocusRestoreWindow(false);
+  if (iter != windows.end())
+    selected_item_ = iter->get();
+  CancelSelection();
+}
+
 void WindowSelector::OnDisplayRemoved(const display::Display& display) {
   // TODO(flackr): Keep window selection active on remaining displays.
   CancelSelection();
@@ -823,53 +865,6 @@ void WindowSelector::OnWindowDestroying(aura::Window* window) {
     restore_focus_window_ = nullptr;
 }
 
-void WindowSelector::OnWindowActivated(ActivationReason reason,
-                                       aura::Window* gained_active,
-                                       aura::Window* lost_active) {
-  if (ignore_activations_ || !gained_active ||
-      gained_active == GetTextFilterWidgetWindow()) {
-    return;
-  }
-
-  auto* grid = GetGridWithRootWindow(gained_active->GetRootWindow());
-  if (!grid)
-    return;
-  const auto& windows = grid->window_list();
-
-  auto iter = std::find_if(
-      windows.begin(), windows.end(),
-      [gained_active](const std::unique_ptr<WindowSelectorItem>& window) {
-        return window->Contains(gained_active);
-      });
-
-  if (iter == windows.end() && showing_text_filter_ &&
-      lost_active == GetTextFilterWidgetWindow()) {
-    return;
-  }
-
-  // Do not cancel overview mode if the window activation was caused by
-  // snapping window to one side of the screen.
-  if (Shell::Get()->IsSplitViewModeActive())
-    return;
-
-  // Do not cancel overview mode if the window activation was caused while
-  // dragging overview mode offscreen.
-  if (IsSlidingOutOverviewFromShelf())
-    return;
-
-  // Don't restore focus on exit if a window was just activated.
-  ResetFocusRestoreWindow(false);
-  if (iter != windows.end())
-    selected_item_ = iter->get();
-  CancelSelection();
-}
-
-void WindowSelector::OnAttemptToReactivateWindow(aura::Window* request_active,
-                                                 aura::Window* actual_active) {
-  OnWindowActivated(ActivationReason::ACTIVATION_CLIENT, request_active,
-                    actual_active);
-}
-
 void WindowSelector::ContentsChanged(views::Textfield* sender,
                                      const base::string16& new_contents) {
   // If the user enters underline mode via CTRL+SHIFT+U, ContentsChanged
@@ -1083,7 +1078,6 @@ void WindowSelector::RemoveAllObservers() {
     window->RemoveObserver(this);
   observed_windows_.clear();
 
-  Shell::Get()->activation_client()->RemoveObserver(this);
   display::Screen::GetScreen()->RemoveObserver(this);
   if (restore_focus_window_)
     restore_focus_window_->RemoveObserver(this);

ash/wm/overview/window_selector.h

@@ -47,7 +47,6 @@ enum class IndicatorState;
 // one by clicking or tapping on it.
 class ASH_EXPORT WindowSelector : public display::DisplayObserver,
                                   public aura::WindowObserver,
-                                  public ::wm::ActivationChangeObserver,
                                   public views::TextfieldController,
                                   public SplitViewController::Observer {
  public:
@@ -224,6 +223,13 @@ class ASH_EXPORT WindowSelector : public display::DisplayObserver,
   // animating.
   bool IsWindowGridAnimating();
 
+  // Called when windows are being activated/deactivated during
+  // overview mode.
+  void OnWindowActivating(
+      ::wm::ActivationChangeObserver::ActivationReason reason,
+      aura::Window* gained_active,
+      aura::Window* lost_active);
+
   WindowSelectorDelegate* delegate() { return delegate_; }
 
   SplitViewDragIndicators* split_view_drag_indicators() {
@@ -257,13 +263,6 @@ class ASH_EXPORT WindowSelector : public display::DisplayObserver,
   void OnWindowHierarchyChanged(const HierarchyChangeParams& params) override;
   void OnWindowDestroying(aura::Window* window) override;
 
-  // wm::ActivationChangeObserver:
-  void OnWindowActivated(ActivationReason reason,
-                         aura::Window* gained_active,
-                         aura::Window* lost_active) override;
-  void OnAttemptToReactivateWindow(aura::Window* request_active,
-                                   aura::Window* actual_active) override;
-
   // views::TextfieldController:
   void ContentsChanged(views::Textfield* sender,
                        const base::string16& new_contents) override;
@@ -315,8 +314,9 @@ class ASH_EXPORT WindowSelector : public display::DisplayObserver,
   aura::Window* restore_focus_window_;
 
   // True when performing operations that may cause window activations. This is
-  // used to prevent handling the resulting expected activation.
-  bool ignore_activations_ = false;
+  // used to prevent handling the resulting expected activation. This is
+  // initially true until this is initialized.
+  bool ignore_activations_ = true;
 
   // List of all the window overview grids, one for each root window.
   std::vector<std::unique_ptr<WindowGrid>> grid_list_;

ash/wm/overview/window_selector_controller.cc

@@ -31,6 +31,7 @@
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/animation/slide_animation.h"
 #include "ui/wm/core/window_util.h"
+#include "ui/wm/public/activation_client.h"
 
 namespace ash {
 
@@ -251,9 +252,12 @@ class WindowSelectorController::OverviewBlurController
 
 WindowSelectorController::WindowSelectorController()
     : overview_blur_controller_(std::make_unique<OverviewBlurController>()),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  Shell::Get()->activation_client()->AddObserver(this);
+}
 
 WindowSelectorController::~WindowSelectorController() {
+  Shell::Get()->activation_client()->RemoveObserver(this);
   overview_blur_controller_.reset();
 
   // Destroy widgets that may be still animating if shell shuts down soon after
@@ -318,7 +322,7 @@ bool WindowSelectorController::ToggleOverview(
       return true;
     }
 
-    // Suspend occlusion tracker until the enter animation is complete.
+    // Suspend occlusion tracker until the exit animation is complete.
     reset_pauser_task_.Cancel();
     occlusion_tracker_pauser_ =
         std::make_unique<aura::WindowOcclusionTracker::ScopedPause>(
@@ -352,10 +356,10 @@ bool WindowSelectorController::ToggleOverview(
     for (const auto& animation : delayed_animations_)
       animation->Shutdown();
     if (!delayed_animations_.empty())
-      OnStartingAnimationComplete(/*canceled=*/true);
+      OnEndingAnimationComplete(/*canceled=*/true);
     delayed_animations_.clear();
 
-    // Suspend occlusion tracker until the exit animation is complete.
+    // Suspend occlusion tracker until the enter animation is complete.
     reset_pauser_task_.Cancel();
     occlusion_tracker_pauser_ =
         std::make_unique<aura::WindowOcclusionTracker::ScopedPause>(
@@ -537,7 +541,7 @@ void WindowSelectorController::OnSelectionEnded() {
     return;
 
   if (!start_animations_.empty())
-    OnEndingAnimationComplete(/*canceled=*/true);
+    OnStartingAnimationComplete(/*canceled=*/true);
   start_animations_.clear();
 
   window_selector_->UpdateMaskAndShadow(/*show=*/false);
@@ -545,14 +549,17 @@ void WindowSelectorController::OnSelectionEnded() {
   Shell::Get()->NotifyOverviewModeEnding();
   auto* window_selector = window_selector_.release();
   window_selector->Shutdown();
-  // There may be no delayed animations in tests, so unblur right away.
-  if (delayed_animations_.empty() && IsBlurAllowed())
-    overview_blur_controller_->Unblur();
   // Don't delete |window_selector_| yet since the stack is still using it.
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, window_selector);
   last_selection_time_ = base::Time::Now();
   Shell::Get()->NotifyOverviewModeEnded();
   is_shutting_down_ = false;
+  // There may be no delayed animations in tests, so unblur right away.
+  if (delayed_animations_.empty()) {
+    if (IsBlurAllowed())
+      overview_blur_controller_->Unblur();
+    OnEndingAnimationComplete(/*canceled=*/false);
+  }
 }
 
 void WindowSelectorController::AddDelayedAnimationObserver(
@@ -579,6 +586,23 @@ void WindowSelectorController::RemoveAndDestroyAnimationObserver(
   }
 }
 
+void WindowSelectorController::OnWindowActivating(ActivationReason reason,
+                                                  aura::Window* gained_active,
+                                                  aura::Window* lost_active) {
+  if (window_selector_)
+    window_selector_->OnWindowActivating(reason, gained_active, lost_active);
+}
+
+void WindowSelectorController::OnAttemptToReactivateWindow(
+    aura::Window* request_active,
+    aura::Window* actual_active) {
+  if (window_selector_) {
+    window_selector_->OnWindowActivating(
+        ::wm::ActivationChangeObserver::ActivationReason::ACTIVATION_CLIENT,
+        request_active, actual_active);
+  }
+}
+
 void WindowSelectorController::AddStartAnimationObserver(
     std::unique_ptr<DelayedAnimationObserver> animation_observer) {
   animation_observer->SetOwner(this);

ash/wm/overview/window_selector_controller.h

@@ -22,7 +22,9 @@ class WindowSelectorTest;
 
 // Manages a window selector which displays an overview of all windows and
 // allows selecting a window to activate it.
-class ASH_EXPORT WindowSelectorController : public WindowSelectorDelegate {
+class ASH_EXPORT WindowSelectorController
+    : public WindowSelectorDelegate,
+      public ::wm::ActivationChangeObserver {
  public:
   enum class AnimationCompleteReason {
     kCompleted,
@@ -84,6 +86,16 @@ class ASH_EXPORT WindowSelectorController : public WindowSelectorDelegate {
   void RemoveAndDestroyStartAnimationObserver(
       DelayedAnimationObserver* animation_observer) override;
 
+  // ::wm::ActivationChangeObserver:
+  void OnWindowActivating(ActivationReason reason,
+                          aura::Window* gained_active,
+                          aura::Window* lost_active) override;
+  void OnWindowActivated(ActivationReason reason,
+                         aura::Window* gained_active,
+                         aura::Window* lost_active) override {}
+  void OnAttemptToReactivateWindow(aura::Window* request_active,
+                                   aura::Window* actual_active) override;
+
   WindowSelector* window_selector() { return window_selector_.get(); }
 
  private:

ash/wm/overview/window_selector_controller_unittest.cc

@@ -5,20 +5,24 @@
 #include "ash/wm/overview/window_selector_controller.h"
 
 #include "ash/shell.h"
+#include "ash/shell_observer.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller_test_api.h"
 #include "ash/wm/window_resizer.h"
+#include "ash/wm/window_util.h"
 #include "base/command_line.h"
 #include "ui/base/hit_test.h"
+#include "ui/compositor/scoped_animation_duration_scale_mode.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/keyboard/keyboard_controller.h"
 #include "ui/keyboard/keyboard_util.h"
 #include "ui/keyboard/public/keyboard_switches.h"
 #include "ui/keyboard/test/keyboard_test_util.h"
 
+namespace ash {
 namespace {
 
-gfx::Point CalculateDragPoint(const ash::WindowResizer& resizer,
+gfx::Point CalculateDragPoint(const WindowResizer& resizer,
                               int delta_x,
                               int delta_y) {
   gfx::Point location = resizer.GetInitialLocation();
@@ -27,9 +31,50 @@ gfx::Point CalculateDragPoint(const ash::WindowResizer& resizer,
   return location;
 }
 
-}  // namespace
+// TODO(sammiequan): consolidate this and TestOverviewObserver
+// in window_selector_unittest.
+class TestShellObserver : public ShellObserver {
+ public:
+  enum AnimationState {
+    UNKNOWN,
+    COMPLETED,
+    CANCELED,
+  };
+
+  TestShellObserver() { Shell::Get()->AddShellObserver(this); }
+  ~TestShellObserver() override { Shell::Get()->RemoveShellObserver(this); }
+
+  // ShellObserver:
+  void OnOverviewModeStartingAnimationComplete(bool canceled) override {
+    EXPECT_EQ(UNKNOWN, starting_animation_state_);
+    starting_animation_state_ = canceled ? CANCELED : COMPLETED;
+  }
+  void OnOverviewModeEndingAnimationComplete(bool canceled) override {
+    EXPECT_EQ(UNKNOWN, ending_animation_state_);
+    ending_animation_state_ = canceled ? CANCELED : COMPLETED;
+  }
 
-namespace ash {
+  void Reset() {
+    starting_animation_state_ = UNKNOWN;
+    ending_animation_state_ = UNKNOWN;
+  }
+
+  bool is_ended() const { return ending_animation_state_ != UNKNOWN; }
+  bool is_started() const { return starting_animation_state_ != UNKNOWN; }
+  AnimationState starting_animation_state() const {
+    return starting_animation_state_;
+  }
+  AnimationState ending_animation_state() const {
+    return ending_animation_state_;
+  }
+
+ private:
+  AnimationState starting_animation_state_ = UNKNOWN;
+  AnimationState ending_animation_state_ = UNKNOWN;
+  DISALLOW_COPY_AND_ASSIGN(TestShellObserver);
+};
+
+}  // namespace
 
 using WindowSelectorControllerTest = AshTestBase;
 
@@ -49,6 +94,59 @@ TEST_F(WindowSelectorControllerTest,
   resizer->CompleteDrag();
 }
 
+TEST_F(WindowSelectorControllerTest, AnimationCallbacks) {
+  ui::ScopedAnimationDurationScaleMode non_zero(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+  TestShellObserver observer;
+  // Enter without windows.
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_TRUE(Shell::Get()->window_selector_controller()->IsSelecting());
+  EXPECT_EQ(TestShellObserver::COMPLETED, observer.starting_animation_state());
+
+  // Exit winhtout windows still creates an animation.
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_FALSE(Shell::Get()->window_selector_controller()->IsSelecting());
+  EXPECT_EQ(TestShellObserver::UNKNOWN, observer.ending_animation_state());
+
+  while (!observer.is_ended())
+    base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(TestShellObserver::COMPLETED, observer.ending_animation_state());
+
+  gfx::Rect bounds(0, 0, 100, 100);
+  std::unique_ptr<aura::Window> window1(
+      CreateTestWindowInShellWithBounds(bounds));
+  std::unique_ptr<aura::Window> window2(
+      CreateTestWindowInShellWithBounds(bounds));
+
+  observer.Reset();
+  ASSERT_EQ(TestShellObserver::UNKNOWN, observer.starting_animation_state());
+  ASSERT_EQ(TestShellObserver::UNKNOWN, observer.ending_animation_state());
+
+  // Enter with windows.
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_TRUE(Shell::Get()->window_selector_controller()->IsSelecting());
+  EXPECT_EQ(TestShellObserver::UNKNOWN, observer.starting_animation_state());
+
+  // Exit with windows.
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_FALSE(Shell::Get()->window_selector_controller()->IsSelecting());
+  EXPECT_EQ(TestShellObserver::CANCELED, observer.starting_animation_state());
+  EXPECT_EQ(TestShellObserver::UNKNOWN, observer.ending_animation_state());
+
+  observer.Reset();
+
+  // Enter again.
+  Shell::Get()->window_selector_controller()->ToggleOverview();
+  EXPECT_TRUE(Shell::Get()->window_selector_controller()->IsSelecting());
+  EXPECT_EQ(TestShellObserver::CANCELED, observer.ending_animation_state());
+  EXPECT_EQ(TestShellObserver::UNKNOWN, observer.starting_animation_state());
+
+  // Activating window while entering animation should cancel the overview.
+  wm::ActivateWindow(window1.get());
+  EXPECT_FALSE(Shell::Get()->window_selector_controller()->IsSelecting());
+  EXPECT_EQ(TestShellObserver::CANCELED, observer.starting_animation_state());
+}
+
 class OverviewVirtualKeyboardTest : public WindowSelectorControllerTest {
  protected:
   void SetUp() override {