Desks: Hotseat should remain visible after dropping a window to another desk

Dropped windows to other desks should not animate while restoring their
transform. This ensures that the hotseat state will be calculated
correctly.

BUG=1063536
TEST=Added a new test.

Change-Id: Idbb525b8380b1e326db31d79256ab7e22d24c032
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2113805
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#752548}

ash/wm/desks/desks_controller.cc

@@ -610,6 +610,21 @@ bool DesksController::MoveWindowFromActiveDeskTo(
   auto* overview_controller = Shell::Get()->overview_controller();
   const bool in_overview = overview_controller->InOverviewSession();
 
+  // The below order matters:
+  // If in overview, remove the item from overview first, before calling
+  // MoveWindowToDesk(), since MoveWindowToDesk() unminimizes the window (if it
+  // was minimized) before updating the mini views. We shouldn't change the
+  // window's minimized state before removing it from overview, since overview
+  // handles minimized windows differently.
+  if (in_overview) {
+    auto* overview_session = overview_controller->overview_session();
+    auto* item = overview_session->GetOverviewItemForWindow(window);
+    DCHECK(item);
+    item->OnMovingWindowToAnotherDesk();
+    // The item no longer needs to be in the overview grid.
+    overview_session->RemoveItem(item);
+  }
+
   active_desk_->MoveWindowToDesk(window, target_desk, target_root);
 
   Shell::Get()
@@ -623,24 +638,12 @@ bool DesksController::MoveWindowFromActiveDeskTo(
   UMA_HISTOGRAM_ENUMERATION(kMoveWindowFromActiveDeskHistogramName, source);
   ReportNumberOfWindowsPerDeskHistogram();
 
-  if (in_overview) {
-    DCHECK(overview_controller->InOverviewSession());
-    auto* overview_session = overview_controller->overview_session();
-    auto* item = overview_session->GetOverviewItemForWindow(window);
-    DCHECK(item);
-    // Restore the dragged item window, so that its transform is reset to
-    // identity.
-    item->RestoreWindow(/*reset_transform=*/true);
-    // The item no longer needs to be in the overview grid.
-    overview_session->RemoveItem(item);
-    // When in overview, we should return immediately and not change the window
-    // activation as we do below, since the dummy "OverviewModeFocusedWidget"
-    // should remain active while overview mode is active..
-    return true;
-  }
-
   // A window moving out of the active desk cannot be active.
-  wm::DeactivateWindow(window);
+  // If we are in overview, we should not change the window activation as we do
+  // below, since the dummy "OverviewModeFocusedWidget" should remain active
+  // while overview mode is active.
+  if (!in_overview)
+    wm::DeactivateWindow(window);
   return true;
 }
 

ash/wm/desks/desks_unittests.cc

@@ -16,6 +16,7 @@
 #include "ash/public/cpp/shelf_types.h"
 #include "ash/screen_util.h"
 #include "ash/session/session_controller_impl.h"
+#include "ash/shelf/hotseat_widget.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_layout_manager.h"
 #include "ash/shell.h"
@@ -2330,6 +2331,68 @@ TEST_F(TabletModeDesksTest, BackdropsStacking) {
   EXPECT_TRUE(IsStackedBelow(desk_2_backdrop, win4.get()));
 }
 
+TEST_F(TabletModeDesksTest, HotSeatStateAfterMovingAWindowToAnotherDesk) {
+  // Create 2 desks, and 2 windows, one of them is minimized and the other is
+  // normal. Test that dragging and dropping them to another desk does not hide
+  // the hotseat. https://crbug.com/1063536.
+  auto* controller = DesksController::Get();
+  NewDesk();
+  ASSERT_EQ(2u, controller->desks().size());
+  auto win0 = CreateAppWindow(gfx::Rect(0, 0, 250, 100));
+  auto win1 = CreateAppWindow(gfx::Rect(0, 0, 250, 100));
+  auto* window_state = WindowState::Get(win0.get());
+  window_state->Minimize();
+  EXPECT_TRUE(window_state->IsMinimized());
+  wm::ActivateWindow(win1.get());
+  EXPECT_EQ(win1.get(), window_util::GetActiveWindow());
+
+  auto* overview_controller = Shell::Get()->overview_controller();
+  overview_controller->StartOverview();
+  EXPECT_TRUE(overview_controller->InOverviewSession());
+  auto* overview_session = overview_controller->overview_session();
+
+  HotseatWidget* hotseat_widget =
+      Shelf::ForWindow(win1.get())->hotseat_widget();
+  EXPECT_EQ(HotseatState::kExtended, hotseat_widget->state());
+
+  const struct {
+    aura::Window* window;
+    const char* trace_message;
+  } kTestTable[] = {{win0.get(), "Minimized window"},
+                    {win1.get(), "Normal window"}};
+
+  for (const auto& test_case : kTestTable) {
+    SCOPED_TRACE(test_case.trace_message);
+    auto* win = test_case.window;
+    auto* overview_item = overview_session->GetOverviewItemForWindow(win);
+    ASSERT_TRUE(overview_item);
+
+    auto* overview_grid = GetOverviewGridForRoot(Shell::GetPrimaryRootWindow());
+    const auto* desks_bar_view = overview_grid->desks_bar_view();
+    ASSERT_TRUE(desks_bar_view);
+    ASSERT_EQ(2u, desks_bar_view->mini_views().size());
+    auto* desk_2_mini_view = desks_bar_view->mini_views()[1].get();
+    auto* desk_2 = controller->desks()[1].get();
+    EXPECT_EQ(desk_2, desk_2_mini_view->desk());
+    auto* event_generator = GetEventGenerator();
+
+    {
+      // For this test to fail without the fix, we need to test while animations
+      // are not disabled.
+      ui::ScopedAnimationDurationScaleMode normal_anim(
+          ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
+      DragItemToPoint(overview_item,
+                      desk_2_mini_view->GetBoundsInScreen().CenterPoint(),
+                      event_generator,
+                      /*by_touch_gestures=*/false);
+    }
+
+    EXPECT_TRUE(overview_controller->InOverviewSession());
+    EXPECT_FALSE(DoesActiveDeskContainWindow(win));
+    EXPECT_EQ(HotseatState::kExtended, hotseat_widget->state());
+  }
+}
+
 namespace {
 
 // A client view that returns a given minimum size to be used on the widget's

ash/wm/overview/overview_item.cc

@@ -207,6 +207,13 @@ bool OverviewItem::Contains(const aura::Window* target) const {
   return transform_window_.Contains(target);
 }
 
+void OverviewItem::OnMovingWindowToAnotherDesk() {
+  is_moving_to_another_desk_ = true;
+  // Restore the dragged item window, so that its transform is reset to
+  // identity.
+  RestoreWindow(/*reset_transform=*/true);
+}
+
 void OverviewItem::RestoreWindow(bool reset_transform) {
   // TODO(oshima): SplitViewController has its own logic to adjust the
   // target state in |SplitViewController::OnOverviewModeEnding|.
@@ -223,9 +230,13 @@ void OverviewItem::RestoreWindow(bool reset_transform) {
   if (transform_window_.IsMinimized()) {
     const auto enter_exit_type = overview_session_->enter_exit_overview_type();
 
-    if (enter_exit_type ==
-        OverviewSession::EnterExitOverviewType::kImmediateExit) {
+    if (is_moving_to_another_desk_ ||
+        enter_exit_type ==
+            OverviewSession::EnterExitOverviewType::kImmediateExit) {
+      overview_session_->highlight_controller()->OnViewDestroyingOrDisabling(
+          overview_item_view_);
       ImmediatelyCloseWidgetOnExit(std::move(item_widget_));
+      overview_item_view_ = nullptr;
       return;
     }
 
@@ -828,8 +839,9 @@ OverviewAnimationType OverviewItem::GetExitOverviewAnimationType() {
 }
 
 OverviewAnimationType OverviewItem::GetExitTransformAnimationType() {
-  if (overview_session_->enter_exit_overview_type() ==
-      OverviewSession::EnterExitOverviewType::kImmediateExit) {
+  if (is_moving_to_another_desk_ ||
+      overview_session_->enter_exit_overview_type() ==
+          OverviewSession::EnterExitOverviewType::kImmediateExit) {
     return OVERVIEW_ANIMATION_NONE;
   }
 

ash/wm/overview/overview_item.h

@@ -50,6 +50,11 @@ class ASH_EXPORT OverviewItem : public views::ButtonListener,
   // Returns true if |target| is contained in this OverviewItem.
   bool Contains(const aura::Window* target) const;
 
+  // This called when the window is dragged and dropped on the mini view of
+  // another desk, which prepares this item for being removed from the grid, and
+  // the window to restore its transform.
+  void OnMovingWindowToAnotherDesk();
+
   // Restores and animates the managed window to its non overview mode state.
   // If |reset_transform| equals false, the window's transform will not be
   // reset to identity transform when exiting overview mode. It's needed when
@@ -224,6 +229,8 @@ class ASH_EXPORT OverviewItem : public views::ButtonListener,
 
   OverviewGrid* overview_grid() { return overview_grid_; }
 
+  bool is_moving_to_another_desk() const { return is_moving_to_another_desk_; }
+
   void set_should_use_spawn_animation(bool value) {
     should_use_spawn_animation_ = value;
   }
@@ -374,6 +381,11 @@ class ASH_EXPORT OverviewItem : public views::ButtonListener,
   // for the lifetime of |this|.
   OverviewGrid* overview_grid_;
 
+  // True when the item is dragged and dropped on another desk's mini view. This
+  // causes it to restore its transform immediately without any animations,
+  // since it is moving to an inactive desk, and therefore won't be visible.
+  bool is_moving_to_another_desk_ = false;
+
   // True if this item should be added to an active overview session using the
   // spawn animation on its first update. This implies an animation type of
   // OVERVIEW_ANIMATION_SPAWN_ITEM_IN_OVERVIEW. This value will be reset to

ash/wm/overview/scoped_overview_transform_window.cc

@@ -252,7 +252,13 @@ bool ScopedOverviewTransformWindow::Contains(const aura::Window* target) const {
 
   if (!IsMinimized())
     return false;
-  return overview_item_->item_widget()->GetNativeWindow()->Contains(target);
+
+  // A minimized window's item_widget_ may have already been destroyed.
+  const auto* item_widget = overview_item_->item_widget();
+  if (!item_widget)
+    return false;
+
+  return item_widget->GetNativeWindow()->Contains(target);
 }
 
 gfx::RectF ScopedOverviewTransformWindow::GetTransformedBounds() const {

ash/wm/wm_shadow_controller_delegate.cc

@@ -8,6 +8,7 @@
 #include "ash/shell.h"
 #include "ash/wm/desks/desks_util.h"
 #include "ash/wm/overview/overview_controller.h"
+#include "ash/wm/overview/overview_item.h"
 #include "ash/wm/overview/overview_session.h"
 #include "ash/wm/splitview/split_view_controller.h"
 #include "ash/wm/window_state.h"
@@ -39,12 +40,11 @@ bool WmShadowControllerDelegate::ShouldShowShadowForWindow(
     OverviewSession* overview_session = overview_controller->overview_session();
     // InOverviewSession() being true implies |overview_session| exists.
     DCHECK(overview_session);
-    // The window may be still in overview mode, but it belongs to a non-active
-    // desk, as it has just been dragged and dropped onto a non-active desk's
-    // mini_view. In this case, we shouldn't disable its shadow, so that it may
-    // restored properly.
+    // Windows in overview that are not moving out of the active desk should not
+    // have shadows.
+    auto* overview_item = overview_session->GetOverviewItemForWindow(window);
     if (desks_util::BelongsToActiveDesk(const_cast<aura::Window*>(window)) &&
-        overview_session->IsWindowInOverview(window)) {
+        overview_item && !overview_item->is_moving_to_another_desk()) {
       return false;
     }
   }