splitview: Avoid using animation disabled key to skip auto snapping

This CL allows AutoSnapController to check if
DragWindowFromShelfController is restoring windows after dragging.
AutoSnapController uses kAnimationsDisabledKey to skip auto snapping
after transient hide & show operations by DragWindowFromShelfController.
But kAnimationsDisabledKey may be used in other locations so we should
not rely on it.
This CL also cleans up auto snapping trigger conditions.

BUG=chromium:1107306
TEST=DragWindowFromShelfControllerTest.HideWindowDuringWindowDragging*
TEST=SplitViewControllerTest.AutoSnapFromMinimizedState

Change-Id: Ibc4f72558db3cf0efa070a77e44d3247b79d9ee7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303337
Commit-Queue: Toshiki Kikuchi <toshikikikuchi@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#793002}

ash/home_screen/drag_window_from_shelf_controller.cc

@@ -28,6 +28,7 @@
 #include "ash/wm/splitview/split_view_controller.h"
 #include "ash/wm/splitview/split_view_drag_indicators.h"
 #include "ash/wm/splitview/split_view_utils.h"
+#include "ash/wm/window_properties.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_transient_descendant_iterator.h"
 #include "ash/wm/window_util.h"
@@ -98,13 +99,16 @@ class DragWindowFromShelfController::WindowsHider
 
       hidden_windows_.push_back(window);
       window->AddObserver(this);
+      window->SetProperty(kHideDuringWindowDragging, true);
     }
     window_util::MinimizeAndHideWithoutAnimation(hidden_windows_);
   }
 
   ~WindowsHider() override {
-    for (auto* window : hidden_windows_)
+    for (auto* window : hidden_windows_) {
       window->RemoveObserver(this);
+      window->ClearProperty(kHideDuringWindowDragging);
+    }
     hidden_windows_.clear();
   }
 
@@ -113,6 +117,7 @@ class DragWindowFromShelfController::WindowsHider
       window->RemoveObserver(this);
       ScopedAnimationDisabler disabler(window);
       window->Show();
+      window->ClearProperty(kHideDuringWindowDragging);
     }
     hidden_windows_.clear();
   }

ash/home_screen/drag_window_from_shelf_controller_unittest.cc

@@ -28,6 +28,7 @@
 #include "ash/wm/splitview/split_view_constants.h"
 #include "ash/wm/splitview/split_view_controller.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller_test_api.h"
+#include "ash/wm/window_properties.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -120,8 +121,10 @@ class DragWindowFromShelfControllerTest : public AshTestBase {
   DISALLOW_COPY_AND_ASSIGN(DragWindowFromShelfControllerTest);
 };
 
-// Tests that we may hide different sets of windows in different scenarios.
-TEST_F(DragWindowFromShelfControllerTest, HideWindowDuringWindowDragging) {
+// Tests that we may hide different sets of windows with a special flag
+// kHideDuringWindowDragging.
+TEST_F(DragWindowFromShelfControllerTest,
+       HideWindowDuringWindowDraggingWithFlag) {
   UpdateDisplay("400x400");
   const gfx::Rect shelf_bounds =
       Shelf::ForWindow(Shell::GetPrimaryRootWindow())->GetIdealBounds();
@@ -132,21 +135,48 @@ TEST_F(DragWindowFromShelfControllerTest, HideWindowDuringWindowDragging) {
   EXPECT_TRUE(window1->IsVisible());
   EXPECT_TRUE(window2->IsVisible());
   EXPECT_TRUE(window3->IsVisible());
+  EXPECT_FALSE(window1->GetProperty(kHideDuringWindowDragging));
+  EXPECT_FALSE(window2->GetProperty(kHideDuringWindowDragging));
+  EXPECT_FALSE(window3->GetProperty(kHideDuringWindowDragging));
 
   StartDrag(window1.get(), shelf_bounds.CenterPoint(), HotseatState::kExtended);
   Drag(gfx::Point(200, 200), 1.f, 1.f);
   EXPECT_TRUE(window1->IsVisible());
   EXPECT_FALSE(window2->IsVisible());
   EXPECT_FALSE(window3->IsVisible());
+  EXPECT_FALSE(window1->GetProperty(kHideDuringWindowDragging));
+  EXPECT_TRUE(window2->GetProperty(kHideDuringWindowDragging));
+  EXPECT_TRUE(window3->GetProperty(kHideDuringWindowDragging));
   EndDrag(shelf_bounds.CenterPoint(), /*velocity_y=*/base::nullopt);
   EXPECT_TRUE(window1->IsVisible());
   EXPECT_TRUE(window2->IsVisible());
   EXPECT_TRUE(window3->IsVisible());
+  EXPECT_FALSE(window1->GetProperty(kHideDuringWindowDragging));
+  EXPECT_FALSE(window2->GetProperty(kHideDuringWindowDragging));
+  EXPECT_FALSE(window3->GetProperty(kHideDuringWindowDragging));
+}
+
+// Tests that we may hide different sets of windows in splitview and restores
+// windows correctly after dragging.
+TEST_F(DragWindowFromShelfControllerTest,
+       HideWindowDuringWindowDraggingInSplitView) {
+  UpdateDisplay("400x400");
+  const gfx::Rect shelf_bounds =
+      Shelf::ForWindow(Shell::GetPrimaryRootWindow())->GetIdealBounds();
+
+  auto window3 = CreateTestWindow();
+  auto window2 = CreateTestWindow();
+  auto window1 = CreateTestWindow();
+  EXPECT_TRUE(window1->IsVisible());
+  EXPECT_TRUE(window2->IsVisible());
+  EXPECT_TRUE(window3->IsVisible());
 
   // In splitview mode, the snapped windows will stay visible during dragging.
   split_view_controller()->SnapWindow(window1.get(), SplitViewController::LEFT);
   split_view_controller()->SnapWindow(window2.get(),
                                       SplitViewController::RIGHT);
+
+  // Try to drag a left snapped window
   StartDrag(window1.get(), shelf_bounds.left_center(), HotseatState::kExtended);
   Drag(gfx::Point(0, 200), 1.f, 1.f);
   EXPECT_TRUE(window1->IsVisible());
@@ -156,7 +186,17 @@ TEST_F(DragWindowFromShelfControllerTest, HideWindowDuringWindowDragging) {
   EXPECT_TRUE(window1->IsVisible());
   EXPECT_TRUE(window2->IsVisible());
   EXPECT_TRUE(window3->IsVisible());
+  // Ensure that all windows are restored correctly without triggering auto
+  // snapping.
+  EXPECT_TRUE(split_view_controller()->IsWindowInSplitView(window1.get()));
+  EXPECT_EQ(split_view_controller()->GetPositionOfSnappedWindow(window1.get()),
+            SplitViewController::LEFT);
+  EXPECT_TRUE(split_view_controller()->IsWindowInSplitView(window2.get()));
+  EXPECT_EQ(split_view_controller()->GetPositionOfSnappedWindow(window2.get()),
+            SplitViewController::RIGHT);
+  EXPECT_FALSE(split_view_controller()->IsWindowInSplitView(window3.get()));
 
+  // Try to drag a right snapped window
   StartDrag(window2.get(), shelf_bounds.right_center(),
             HotseatState::kExtended);
   Drag(gfx::Point(400, 200), 1.f, 1.f);
@@ -167,6 +207,15 @@ TEST_F(DragWindowFromShelfControllerTest, HideWindowDuringWindowDragging) {
   EXPECT_TRUE(window1->IsVisible());
   EXPECT_TRUE(window2->IsVisible());
   EXPECT_TRUE(window3->IsVisible());
+  // Ensure that all windows are restored correctly without triggering auto
+  // snapping.
+  EXPECT_TRUE(split_view_controller()->IsWindowInSplitView(window1.get()));
+  EXPECT_EQ(split_view_controller()->GetPositionOfSnappedWindow(window1.get()),
+            SplitViewController::LEFT);
+  EXPECT_TRUE(split_view_controller()->IsWindowInSplitView(window2.get()));
+  EXPECT_EQ(split_view_controller()->GetPositionOfSnappedWindow(window2.get()),
+            SplitViewController::RIGHT);
+  EXPECT_FALSE(split_view_controller()->IsWindowInSplitView(window3.get()));
 }
 
 // Test home launcher is hidden during dragging.

ash/wm/splitview/split_view_controller.cc

@@ -31,6 +31,7 @@
 #include "ash/wm/splitview/split_view_utils.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/wm/tablet_mode/tablet_mode_window_state.h"
+#include "ash/wm/window_properties.h"
 #include "ash/wm/window_resizer.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_transient_descendant_iterator.h"
@@ -405,22 +406,29 @@ class SplitViewController::AutoSnapController
 
   // aura::WindowObserver:
   void OnWindowVisibilityChanging(aura::Window* window, bool visible) override {
-    // TODO(toshikikikuchi): Consider avoiding to use kAnimationsDisabledKey
-    // here just for |DragWindowFromShelfController|.
-    if (visible && WindowState::Get(window) &&
-        WindowState::Get(window)->IsMinimized() &&
-        !window->GetProperty(aura::client::kAnimationsDisabledKey)) {
-      // A visible but minimized window state triggers an implicit un-minimizing
-      // by someone (e.g.
-      // |WorkspaceLayoutManager::OnChildWindowVisibilityChanged| or
-      // |WorkspaceLayoutManager::OnWindowActivating|). This emits a window
-      // state change event but it is unnecessary for to-be-snapped windows
-      // because some clients (e.g. ARC app) handle a window state change
-      // asynchronously. So in the case, we here try to snap a window before
-      // other's handling. Animation-disabled visibility changes are used for
-      // transient hide & show operations so not applicable for auto snapping.
-      AutoSnapWindowIfNeeded(window);
-    }
+    // When a minimized window's visibility changes from invisible to visible or
+    // is about to activate, it triggers an implicit un-minimizing (e.g.
+    // |WorkspaceLayoutManager::OnChildWindowVisibilityChanged| or
+    // |WorkspaceLayoutManager::OnWindowActivating|). This emits a window
+    // state change event but it is unnecessary for to-be-snapped windows
+    // because some clients (e.g. ARC app) handle a window state change
+    // asynchronously. So in the case, we here try to snap a window before
+    // other's handling to avoid the implicit un-minimizing.
+
+    // Auto snapping is applicable for window changed to be visible.
+    if (!visible)
+      return;
+
+    // Already un-minimized windows are not applicable for auto snapping.
+    if (!WindowState::Get(window) || !WindowState::Get(window)->IsMinimized())
+      return;
+
+    // Visibility changes while restoring windows after dragged is transient
+    // hide & show operations so not applicable for auto snapping.
+    if (window->GetProperty(kHideDuringWindowDragging))
+      return;
+
+    AutoSnapWindowIfNeeded(window);
   }
 
   void OnWindowAddedToRootWindow(aura::Window* window) override {

ash/wm/splitview/split_view_controller_unittest.cc

@@ -47,6 +47,7 @@
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/wm/tablet_mode/tablet_mode_window_drag_delegate.h"
 #include "ash/wm/tablet_mode/tablet_mode_window_resizer.h"
+#include "ash/wm/window_properties.h"
 #include "ash/wm/window_resizer.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_state_delegate.h"
@@ -2735,6 +2736,21 @@ TEST_P(SplitViewControllerTest, AutoSnapFromMinimizedState) {
   EXPECT_FALSE(split_view_controller()->InSplitViewMode());
   EXPECT_FALSE(split_view_controller()->IsWindowInSplitView(window2.get()));
 
+  // Nothing should happen for transient visibility changing due to dragging.
+  Shell::Get()->tablet_mode_controller()->SetEnabledForTest(true);
+  split_view_controller()->SnapWindow(window3.get(), SplitViewController::LEFT);
+  EXPECT_TRUE(split_view_controller()->InTabletSplitViewMode());
+  EXPECT_TRUE(split_view_controller()->IsWindowInSplitView(window3.get()));
+  EXPECT_EQ(split_view_controller()->GetPositionOfSnappedWindow(window3.get()),
+            SplitViewController::LEFT);
+
+  WindowState::Get(window1.get())->Minimize();
+  window1->SetProperty(kHideDuringWindowDragging, true);
+  window1->Show();
+  EXPECT_FALSE(split_view_controller()->IsWindowInSplitView(window1.get()));
+
+  window1->ClearProperty(kHideDuringWindowDragging);
+
   // Should performs auto snapping when showing a snappable window in table
   // split view mode.
   Shell::Get()->tablet_mode_controller()->SetEnabledForTest(true);

ash/wm/window_properties.cc

@@ -11,6 +11,8 @@ DEFINE_EXPORTED_UI_CLASS_PROPERTY_TYPE(ASH_EXPORT, ash::WindowState*)
 
 namespace ash {
 
+DEFINE_UI_CLASS_PROPERTY_KEY(bool, kHideDuringWindowDragging, false)
+
 DEFINE_UI_CLASS_PROPERTY_KEY(bool, kLockedToRootKey, false)
 
 DEFINE_UI_CLASS_PROPERTY_KEY(bool, kWindowIsJanky, false)

ash/wm/window_properties.h

@@ -22,6 +22,11 @@ class WindowState;
 
 // Alphabetical sort.
 
+// A property key to indicate whether this window is temporarily hidden because
+// of the window dragging.
+ASH_EXPORT extern const aura::WindowProperty<bool>* const
+    kHideDuringWindowDragging;
+
 // If this is set to true, the window stays in the same root window even if the
 // bounds outside of its root window is set.
 ASH_EXPORT extern const aura::WindowProperty<bool>* const kLockedToRootKey;