[go: up one dir, main page]

[Merge to M-96] desks: Do not allow dragging of a mini view being removed

We should skip handling drags of mini views that are
animating to be removed. Otherwise, they become our
drag targets and remain so, even when the animation
is done and the mini view is destroyed.

(cherry picked from commit bab2d02a8b84177d86325c76800c8c903755c1b4)

Fixed: 1274641
Test: Manually, added a regression test.
Change-Id: Iab2fa738b0cdc081dfd0b4e9c3201fe45d2a1189
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3307965
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Min Chen <minch@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#946280}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3312740
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Ahmed Fakhry <afakhry@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4664@{#1206}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}
diff --git a/ash/wm/desks/desk_mini_view.h b/ash/wm/desks/desk_mini_view.h
index 78f7e3e0..d651fec 100644
--- a/ash/wm/desks/desk_mini_view.h
+++ b/ash/wm/desks/desk_mini_view.h
@@ -63,6 +63,11 @@
   DesksBarView* owner_bar() { return owner_bar_; }
   const DeskPreviewView* desk_preview() const { return desk_preview_; }
 
+  bool is_animating_to_remove() const { return is_animating_to_remove_; }
+  void set_is_animating_to_remove(bool value) {
+    is_animating_to_remove_ = value;
+  }
+
   gfx::Rect GetPreviewBoundsInScreen() const;
 
   // Returns the associated desk's container window on the display this
@@ -152,6 +157,9 @@
   // The close button that shows on hover.
   CloseDeskButton* close_desk_button_;
 
+  // True when this mini view is being animated to be removed from the bar.
+  bool is_animating_to_remove_ = false;
+
   // We force showing the close button when the mini_view is long pressed or
   // tapped using touch gestures.
   bool force_show_close_button_ = false;
diff --git a/ash/wm/desks/desk_mini_view_animations.cc b/ash/wm/desks/desk_mini_view_animations.cc
index 50773ac..e985aac 100644
--- a/ash/wm/desks/desk_mini_view_animations.cc
+++ b/ash/wm/desks/desk_mini_view_animations.cc
@@ -118,6 +118,7 @@
                            DesksBarView* bar_view,
                            const bool to_zero_state)
       : removed_mini_view_(removed_mini_view) {
+    removed_mini_view_->set_is_animating_to_remove(true);
     ui::Layer* layer = removed_mini_view_->layer();
     ui::ScopedLayerAnimationSettings settings{layer->GetAnimator()};
     InitScopedAnimationSettings(&settings, kRemovedMiniViewsFadeOutDuration);
diff --git a/ash/wm/desks/desks_bar_view.cc b/ash/wm/desks/desks_bar_view.cc
index a02ce43..b3b1472 100644
--- a/ash/wm/desks/desks_bar_view.cc
+++ b/ash/wm/desks/desks_bar_view.cc
@@ -505,6 +505,9 @@
 
 void DesksBarView::HandlePressEvent(DeskMiniView* mini_view,
                                     const ui::LocatedEvent& event) {
+  if (mini_view->is_animating_to_remove())
+    return;
+
   DeskNameView::CommitChanges(GetWidget());
 
   gfx::PointF location = event.target()->GetScreenLocationF(event);
@@ -513,6 +516,9 @@
 
 void DesksBarView::HandleLongPressEvent(DeskMiniView* mini_view,
                                         const ui::LocatedEvent& event) {
+  if (mini_view->is_animating_to_remove())
+    return;
+
   DeskNameView::CommitChanges(GetWidget());
 
   // Initialize and start drag.
@@ -523,8 +529,9 @@
 
 void DesksBarView::HandleDragEvent(DeskMiniView* mini_view,
                                    const ui::LocatedEvent& event) {
-  // Do not perform drag if drag proxy is not initialized.
-  if (!drag_proxy_)
+  // Do not perform drag if drag proxy is not initialized, or the mini view is
+  // animating to be removed.
+  if (!drag_proxy_ || mini_view->is_animating_to_remove())
     return;
 
   gfx::PointF location = event.target()->GetScreenLocationF(event);
@@ -545,8 +552,9 @@
 
 bool DesksBarView::HandleReleaseEvent(DeskMiniView* mini_view,
                                       const ui::LocatedEvent& event) {
-  // Do not end drag if the proxy is not initialized.
-  if (!drag_proxy_)
+  // Do not end drag if the proxy is not initialized, or the mini view is
+  // animating to be removed.
+  if (!drag_proxy_ || mini_view->is_animating_to_remove())
     return false;
 
   // If the drag didn't start, finalize the drag. Otherwise, end the drag and
@@ -566,6 +574,8 @@
 
 void DesksBarView::InitDragDesk(DeskMiniView* mini_view,
                                 const gfx::PointF& location_in_screen) {
+  DCHECK(!mini_view->is_animating_to_remove());
+
   // If another view is being dragged, then end the drag.
   if (drag_view_)
     EndDragDesk(drag_view_, /*end_by_user=*/false);
@@ -587,6 +597,7 @@
   DCHECK(drag_view_);
   DCHECK(drag_proxy_);
   DCHECK_EQ(mini_view, drag_view_);
+  DCHECK(!mini_view->is_animating_to_remove());
 
   // Hide the dragged mini view.
   drag_view_->layer()->SetOpacity(0.0f);
@@ -603,6 +614,7 @@
   DCHECK(drag_view_);
   DCHECK(drag_proxy_);
   DCHECK_EQ(mini_view, drag_view_);
+  DCHECK(!mini_view->is_animating_to_remove());
 
   drag_proxy_->DragToX(location_in_screen.x());
 
@@ -631,6 +643,7 @@
   DCHECK(drag_view_);
   DCHECK(drag_proxy_);
   DCHECK_EQ(mini_view, drag_view_);
+  DCHECK(!mini_view->is_animating_to_remove());
 
   // Update default desk names after dropping.
   Shell::Get()->desks_controller()->UpdateDesksDefaultNames();
diff --git a/ash/wm/desks/desks_unittests.cc b/ash/wm/desks/desks_unittests.cc
index 73cdfe3..5ca36ee 100644
--- a/ash/wm/desks/desks_unittests.cc
+++ b/ash/wm/desks/desks_unittests.cc
@@ -5564,6 +5564,42 @@
   ExitOverview();
 }
 
+// Regression test for the asan failure at https://crbug.com/1274641.
+TEST_F(DesksTest, DragMiniViewWhileRemoving) {
+  NewDesk();
+  NewDesk();
+
+  EnterOverview();
+  EXPECT_TRUE(Shell::Get()->overview_controller()->InOverviewSession());
+
+  const auto* desks_bar_view =
+      GetOverviewGridForRoot(Shell::GetPrimaryRootWindow())->desks_bar_view();
+
+  auto* event_generator = GetEventGenerator();
+
+  // Cache the center point of the desk preview that is about to be removed.
+  auto* mini_view = desks_bar_view->mini_views().back();
+  const gfx::Point desk_preview_center =
+      mini_view->GetPreviewBoundsInScreen().CenterPoint();
+
+  {
+    // This test requires animation to repro the asan failure.
+    ui::ScopedAnimationDurationScaleMode animation_scale(
+        ui::ScopedAnimationDurationScaleMode::NORMAL_DURATION);
+
+    // This will trigger the mini view removal animation, and the miniview won't
+    // be removed immediately.
+    CloseDeskFromMiniView(mini_view, event_generator);
+
+    // Drag the mini view that is being animated to be removed, and expect drag
+    // not to start, nor trigger a crash or an asan failure.
+    event_generator->set_current_screen_location(desk_preview_center);
+    event_generator->PressLeftButton();
+    event_generator->MoveMouseBy(0, 50);
+    EXPECT_FALSE(desks_bar_view->IsDraggingDesk());
+  }
+}
+
 // Tests that the right desk containers are visible when switching between desks
 // really fast. Regression test for https://crbug.com/1194757.
 TEST_F(DesksTest, FastDeskSwitches) {