splitview: Improve dragging in overview mode.

Add a minimum distance for windows that get tapped and already in a snap region
(windows tapped close too the left or right side of the screen). This is
so users do not accidently snap too easily when trying to activate the
window. Also fix a bug where you can drag very straight across or down
without the overview window moving.

Test: ash_unittests SplitViewWindowSelectorTest.Dragging
Bug: 778933, 781324
Change-Id: I1b4e993951260f079f85851890854dc217ea95f3
Reviewed-on: https://chromium-review.googlesource.com/757985
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517458}

ash/wm/overview/overview_window_drag_controller.cc

@@ -23,17 +23,12 @@ namespace ash {
 
 namespace {
 
-// The minimum offset that will be considered as a drag event.
-constexpr int kMinimiumDragOffset = 5;
-
 // Returns true if |screen_orientation| is a primary orientation.
 bool IsPrimaryScreenOrientation(
     blink::WebScreenOrientationLockType screen_orientation) {
-  if (screen_orientation == blink::kWebScreenOrientationLockLandscapePrimary ||
-      screen_orientation == blink::kWebScreenOrientationLockPortraitPrimary) {
-    return true;
-  }
-  return false;
+  return screen_orientation ==
+             blink::kWebScreenOrientationLockLandscapePrimary ||
+         screen_orientation == blink::kWebScreenOrientationLockPortraitPrimary;
 }
 
 }  // namespace
@@ -48,9 +43,16 @@ OverviewWindowDragController::~OverviewWindowDragController() {}
 void OverviewWindowDragController::InitiateDrag(
     WindowSelectorItem* item,
     const gfx::Point& location_in_screen) {
-  previous_event_location_ = location_in_screen;
   item_ = item;
 
+  previous_event_location_ = location_in_screen;
+  // No need to track the initial event location if the event does not start in
+  // a snap region.
+  initial_event_location_ =
+      GetSnapPosition(location_in_screen) == SplitViewController::NONE
+          ? base::nullopt
+          : base::make_optional(location_in_screen);
+
   window_selector_->SetSplitViewOverviewOverlayIndicatorType(
       split_view_controller_->CanSnap(item->GetWindow())
           ? IndicatorType::DRAG_AREA
@@ -59,12 +61,14 @@ void OverviewWindowDragController::InitiateDrag(
 }
 
 void OverviewWindowDragController::Drag(const gfx::Point& location_in_screen) {
-  if (!did_move_ &&
-      (std::abs(location_in_screen.x() - previous_event_location_.x()) <
-           kMinimiumDragOffset ||
-       std::abs(location_in_screen.y() - previous_event_location_.y()) <
-           kMinimiumDragOffset)) {
-    return;
+  if (!did_move_) {
+    gfx::Vector2d distance = location_in_screen - previous_event_location_;
+    // Do not start dragging if the distance from |location_in_screen| to
+    // |previous_event_location_| is not greater than |kMinimumDragOffset|.
+    if (std::abs(distance.x()) < kMinimumDragOffset &&
+        std::abs(distance.y()) < kMinimumDragOffset) {
+      return;
+    }
   }
   did_move_ = true;
 
@@ -75,25 +79,28 @@ void OverviewWindowDragController::Drag(const gfx::Point& location_in_screen) {
   item_->SetBounds(bounds, OverviewAnimationType::OVERVIEW_ANIMATION_NONE);
   previous_event_location_ = location_in_screen;
 
-  UpdatePhantomWindowAndWindowGrid(location_in_screen);
-
-  // Show the CANNOT_SNAP ui on the split view overview overlay if the window
-  // cannot be snapped, otherwise show the drag ui only while the phantom window
-  // is hidden.
-  IndicatorType indicator_type = IndicatorType::CANNOT_SNAP;
-  if (split_view_controller_->CanSnap(item_->GetWindow())) {
-    indicator_type = IsPhantomWindowShowing() ? IndicatorType::NONE
-                                              : IndicatorType::DRAG_AREA;
+  if (ShouldUpdatePhantomWindowOrSnap(location_in_screen)) {
+    UpdatePhantomWindowAndWindowGrid(location_in_screen);
+
+    // Show the CANNOT_SNAP ui on the split view overview overlay if the window
+    // cannot be snapped, otherwise show the drag ui only while the phantom
+    // window is hidden.
+    IndicatorType indicator_type = IndicatorType::CANNOT_SNAP;
+    if (split_view_controller_->CanSnap(item_->GetWindow())) {
+      indicator_type = IsPhantomWindowShowing() ? IndicatorType::NONE
+                                                : IndicatorType::DRAG_AREA;
+    }
+    window_selector_->SetSplitViewOverviewOverlayIndicatorType(indicator_type,
+                                                               gfx::Point());
   }
-  window_selector_->SetSplitViewOverviewOverlayIndicatorType(indicator_type,
-                                                             gfx::Point());
 }
 
 void OverviewWindowDragController::CompleteDrag(
     const gfx::Point& location_in_screen) {
   // Update window grid bounds and |snap_position_| in case the screen
   // orientation was changed.
-  UpdatePhantomWindowAndWindowGrid(location_in_screen);
+  if (ShouldUpdatePhantomWindowOrSnap(location_in_screen))
+    UpdatePhantomWindowAndWindowGrid(location_in_screen);
   phantom_window_controller_.reset();
   window_selector_->SetSplitViewOverviewOverlayIndicatorType(
       IndicatorType::NONE, gfx::Point());
@@ -104,10 +111,12 @@ void OverviewWindowDragController::CompleteDrag(
     did_move_ = false;
     // If the window was dragged around but should not be snapped, move it back
     // to overview window grid.
-    if (snap_position_ == SplitViewController::NONE)
+    if (!ShouldUpdatePhantomWindowOrSnap(location_in_screen) ||
+        snap_position_ == SplitViewController::NONE) {
       window_selector_->PositionWindows(true /* animate */);
-    else
+    } else {
       SnapWindow(snap_position_);
+    }
   }
 }
 
@@ -184,8 +193,51 @@ void OverviewWindowDragController::UpdatePhantomWindowAndWindowGrid(
   phantom_window_controller_->Show(phantom_bounds_in_screen);
 }
 
+bool OverviewWindowDragController::ShouldUpdatePhantomWindowOrSnap(
+    const gfx::Point& event_location) {
+  if (initial_event_location_ == base::nullopt)
+    return true;
+
+  auto snap_position = GetSnapPosition(event_location);
+  if (snap_position == SplitViewController::NONE) {
+    // If the event started in a snap region, but has since moved out set
+    // |initial_event_location_| to |event_location| which is guarenteed to not
+    // be in a snap region so that the phantom window or snap mechanism works
+    // normally for the rest of the drag.
+    initial_event_location_ = base::nullopt;
+    return true;
+  }
+
+  // The phantom window can update or the item can snap even if the drag events
+  // are in the snap region, if the event has traveled past the threshold in the
+  // direction of the attempted snap region.
+  const gfx::Vector2d distance = event_location - *initial_event_location_;
+  // Check the x-axis distance for landscape, y-axis distance for portrait.
+  int distance_scalar =
+      split_view_controller_->IsCurrentScreenOrientationLandscape()
+          ? distance.x()
+          : distance.y();
+
+  // If the snap region is physically on the left/top side of the device, check
+  // that |distance_scalar| is less than -|threshold|. If the snap region is
+  // physically on the right/bottom side of the device, check that
+  // |distance_scalar| is greater than |threshold|. Note: in some orientations
+  // SplitViewController::LEFT is not physically on the left/top.
+  const int threshold =
+      OverviewWindowDragController::kMinimumDragOffsetAlreadyInSnapRegionDp;
+  const bool inverted = !SplitViewController::IsLeftWindowOnTopOrLeftOfScreen(
+      Shell::Get()->screen_orientation_controller()->GetCurrentOrientation());
+  const bool on_the_left_or_top =
+      (!inverted && snap_position == SplitViewController::LEFT) ||
+      (inverted && snap_position == SplitViewController::RIGHT);
+
+  return on_the_left_or_top ? distance_scalar <= -threshold
+                            : distance_scalar >= threshold;
+}
+
 SplitViewController::SnapPosition OverviewWindowDragController::GetSnapPosition(
     const gfx::Point& location_in_screen) const {
+  DCHECK(item_);
   gfx::Rect area(
       ScreenUtil::GetDisplayWorkAreaBoundsInParent(item_->GetWindow()));
   ::wm::ConvertRectToScreen(item_->GetWindow()->GetRootWindow(), &area);

ash/wm/overview/overview_window_drag_controller.h

@@ -10,6 +10,7 @@
 #include "ash/ash_export.h"
 #include "ash/wm/splitview/split_view_controller.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "ui/gfx/geometry/point.h"
 
 namespace ash {
@@ -26,6 +27,11 @@ class ASH_EXPORT OverviewWindowDragController {
   // Snapping distance between the dragged window with the screen edge. It's
   // useful especially for touch events.
   static constexpr int kScreenEdgeInsetForDrag = 200;
+  // The minimum offset that will be considered as a drag event.
+  static constexpr int kMinimumDragOffset = 5;
+  // The minimum offset that an item must be moved before it is considered a
+  // drag event, if the drag starts in one of the snap regions.
+  static constexpr int kMinimumDragOffsetAlreadyInSnapRegionDp = 48;
 
   explicit OverviewWindowDragController(WindowSelector* window_selector);
   ~OverviewWindowDragController();
@@ -53,6 +59,11 @@ class ASH_EXPORT OverviewWindowDragController {
   // Updates visuals for the user while dragging items around.
   void UpdatePhantomWindowAndWindowGrid(const gfx::Point& location_in_screen);
 
+  // Dragged items should not attempt to show the phantom window or snap if
+  // the drag started in a snap region and has not been dragged pass the
+  // threshold.
+  bool ShouldUpdatePhantomWindowOrSnap(const gfx::Point& event_location);
+
   SplitViewController::SnapPosition GetSnapPosition(
       const gfx::Point& location_in_screen) const;
 
@@ -74,6 +85,11 @@ class ASH_EXPORT OverviewWindowDragController {
   // The location of the previous mouse/touch/gesture event in screen.
   gfx::Point previous_event_location_;
 
+  // The location of the initial mouse/touch/gesture event in screen. It is
+  // nullopt if the initial drag location was not a snap region, or if the it
+  // was a snap region but the drag has since moved out.
+  base::Optional<gfx::Point> initial_event_location_;
+
   // Set to true once the bounds of |item_| change.
   bool did_move_ = false;
 

ash/wm/overview/window_selector_unittest.cc

@@ -1929,6 +1929,14 @@ class SplitViewWindowSelectorTest : public WindowSelectorTest {
   SplitViewWindowSelectorTest() = default;
   ~SplitViewWindowSelectorTest() override = default;
 
+  enum class SelectorItemLocation {
+    CENTER,
+    ORIGIN,
+    TOP_RIGHT,
+    BOTTOM_RIGHT,
+    BOTTOM_LEFT,
+  };
+
   void SetUp() override {
     WindowSelectorTest::SetUp();
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ -1966,14 +1974,47 @@ class SplitViewWindowSelectorTest : public WindowSelectorTest {
         ->split_view_divider_->GetDividerBoundsInScreen(is_dragging);
   }
 
-  void DragWindowTo(WindowSelectorItem* item, const gfx::Point& end_location) {
+  // Drags a window selector item |item| from its center or one of its corners
+  // to |end_location|. This should be used over
+  // DragWindowTo(WindowSelectorItem*, gfx::Point) when testing snapping a
+  // window, but the windows centerpoint may be inside a snap region, thus the
+  // window will not snapped.
+  void DragWindowTo(WindowSelectorItem* item,
+                    const gfx::Point& end_location,
+                    SelectorItemLocation location) {
     // Start drag in the middle of the seletor item.
-    const gfx::Point start_location(item->target_bounds().CenterPoint());
+    gfx::Point start_location;
+    switch (location) {
+      case SelectorItemLocation::CENTER:
+        start_location = item->target_bounds().CenterPoint();
+        break;
+      case SelectorItemLocation::ORIGIN:
+        start_location = item->target_bounds().origin();
+        break;
+      case SelectorItemLocation::TOP_RIGHT:
+        start_location = item->target_bounds().top_right();
+        break;
+      case SelectorItemLocation::BOTTOM_RIGHT:
+        start_location = item->target_bounds().bottom_right();
+        break;
+      case SelectorItemLocation::BOTTOM_LEFT:
+        start_location = item->target_bounds().bottom_left();
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
     window_selector()->InitiateDrag(item, start_location);
     window_selector()->Drag(item, end_location);
     window_selector()->CompleteDrag(item, end_location);
   }
 
+  // Drags a window selector item |item| from its center point to
+  // |end_location|.
+  void DragWindowTo(WindowSelectorItem* item, const gfx::Point& end_location) {
+    DragWindowTo(item, end_location, SelectorItemLocation::CENTER);
+  }
+
   // Creates a window which cannot be snapped by splitview.
   std::unique_ptr<aura::Window> CreateUnsnappableWindow(
       const gfx::Rect& bounds = gfx::Rect()) {
@@ -2056,6 +2097,88 @@ TEST_F(SplitViewWindowSelectorTest, DragOverviewWindowToSnap) {
   EXPECT_FALSE(window_selector_controller()->IsSelecting());
 }
 
+TEST_F(SplitViewWindowSelectorTest, Dragging) {
+  ui::test::EventGenerator& generator = GetEventGenerator();
+
+  std::unique_ptr<aura::Window> right_window = CreateTestWindow();
+  std::unique_ptr<aura::Window> left_window = CreateTestWindow();
+
+  ToggleOverview();
+  ASSERT_TRUE(window_selector_controller()->IsSelecting());
+
+  WindowSelectorItem* left_selector_item =
+      GetWindowItemForWindow(0, left_window.get());
+  WindowSelectorItem* right_selector_item =
+      GetWindowItemForWindow(0, right_window.get());
+
+  // The inset on each side of the screen which is a snap region. Items dragged
+  // to and released under this region will get snapped.
+  const int drag_offset = OverviewWindowDragController::kMinimumDragOffset;
+  const int drag_offset_snap_region =
+      OverviewWindowDragController::kMinimumDragOffsetAlreadyInSnapRegionDp;
+  const int edge_inset = OverviewWindowDragController::kScreenEdgeInsetForDrag;
+  const int screen_width =
+      ScreenUtil::GetDisplayWorkAreaBoundsInParent(left_window.get()).width();
+  // The selector item has a margin which does not accept events. Inset any
+  // event aimed at the selector items edge so events will reach it.
+  const int selector_item_inset = 20;
+
+  // Check the two windows set up have a region which is under no snap region, a
+  // region that is under the left snap region and a region that is under the
+  // right snap region.
+  ASSERT_GT(left_selector_item->target_bounds().CenterPoint().x(), edge_inset);
+  ASSERT_LT(
+      left_selector_item->target_bounds().origin().x() + selector_item_inset,
+      edge_inset);
+  ASSERT_GT(right_selector_item->target_bounds().top_right().x() -
+                selector_item_inset,
+            screen_width - edge_inset);
+
+  // Verify if the drag is not started in either snap region, the drag still
+  // must move by |drag_offset| before split view acknowledges the drag (ie.
+  // starts moving the selector item).
+  generator.set_current_location(
+      left_selector_item->target_bounds().CenterPoint());
+  generator.PressLeftButton();
+  const gfx::Rect left_original_bounds = left_selector_item->target_bounds();
+  generator.MoveMouseBy(drag_offset - 1, 0);
+  EXPECT_EQ(left_original_bounds, left_selector_item->target_bounds());
+  generator.MoveMouseBy(1, 0);
+  EXPECT_NE(left_original_bounds, left_selector_item->target_bounds());
+  generator.ReleaseLeftButton();
+
+  // Verify if the drag is started in the left snap region, the drag needs to
+  // move by |drag_offset_snap_region| towards the left side of the screen
+  // before split view acknowledges the drag (shows the phantom window).
+  ASSERT_TRUE(window_selector_controller()->IsSelecting());
+  generator.set_current_location(gfx::Point(
+      left_selector_item->target_bounds().origin().x() + selector_item_inset,
+      left_selector_item->target_bounds().CenterPoint().y()));
+  generator.PressLeftButton();
+  generator.MoveMouseBy(-drag_offset_snap_region + 1, 0);
+  EXPECT_FALSE(window_drag_controller()->IsPhantomWindowShowing());
+  generator.MoveMouseBy(-1, 0);
+  EXPECT_TRUE(window_drag_controller()->IsPhantomWindowShowing());
+  // Drag back to the middle before releasing so that we stay in overview mode
+  // on release.
+  generator.MoveMouseTo(left_original_bounds.CenterPoint());
+  generator.ReleaseLeftButton();
+
+  // Verify if the drag is started in the right snap region, the drag needs to
+  // move by |drag_offset_snap_region| towards the right side of the screen
+  // before split view acknowledges the drag.
+  ASSERT_TRUE(window_selector_controller()->IsSelecting());
+  generator.set_current_location(
+      gfx::Point(right_selector_item->target_bounds().top_right().x() -
+                     selector_item_inset,
+                 right_selector_item->target_bounds().CenterPoint().y()));
+  generator.PressLeftButton();
+  generator.MoveMouseBy(drag_offset_snap_region - 1, 0);
+  EXPECT_FALSE(window_drag_controller()->IsPhantomWindowShowing());
+  generator.MoveMouseBy(1, 0);
+  EXPECT_TRUE(window_drag_controller()->IsPhantomWindowShowing());
+}
+
 // Verify the window grid size changes as expected when dragging items around in
 // overview mode when split view is enabled.
 TEST_F(SplitViewWindowSelectorTest, WindowGridSizeWhileDraggingWithSplitView) {
@@ -2415,7 +2538,7 @@ TEST_F(SplitViewWindowSelectorTest, SplitViewRotationTest) {
   work_area_rect =
       split_view_controller()->GetDisplayWorkAreaBoundsInScreen(window2.get());
   end_location2 = gfx::Point(work_area_rect.width(), work_area_rect.height());
-  DragWindowTo(selector_item2, end_location2);
+  DragWindowTo(selector_item2, end_location2, SelectorItemLocation::ORIGIN);
   EXPECT_EQ(split_view_controller()->state(),
             SplitViewController::BOTH_SNAPPED);
   EXPECT_EQ(split_view_controller()->left_window(), window2.get());
@@ -2440,7 +2563,7 @@ TEST_F(SplitViewWindowSelectorTest, SplitViewRotationTest) {
   work_area_rect =
       split_view_controller()->GetDisplayWorkAreaBoundsInScreen(window2.get());
   end_location2 = gfx::Point(work_area_rect.width(), work_area_rect.height());
-  DragWindowTo(selector_item2, end_location2);
+  DragWindowTo(selector_item2, end_location2, SelectorItemLocation::ORIGIN);
   EXPECT_EQ(split_view_controller()->state(),
             SplitViewController::BOTH_SNAPPED);
   EXPECT_EQ(split_view_controller()->left_window(), window2.get());

ash/wm/splitview/split_view_controller.cc

@@ -58,16 +58,6 @@ gfx::Point GetBoundedPosition(const gfx::Point& location_in_screen,
                bounds_in_screen.y()));
 }
 
-// Returns ture if |left_window_| should be placed on the left or top side of
-// the screen.
-bool IsLeftWindowOnTopOrLeftOfScreen(
-    blink::WebScreenOrientationLockType screen_orientation) {
-  return screen_orientation ==
-             blink::kWebScreenOrientationLockLandscapePrimary ||
-         screen_orientation ==
-             blink::kWebScreenOrientationLockPortraitSecondary;
-}
-
 // Transpose the given |rect|.
 void TransposeRect(gfx::Rect* rect) {
   rect->SetRect(rect->y(), rect->x(), rect->height(), rect->width());
@@ -100,6 +90,15 @@ bool SplitViewController::ShouldAllowSplitView() {
   return true;
 }
 
+// static
+bool SplitViewController::IsLeftWindowOnTopOrLeftOfScreen(
+    blink::WebScreenOrientationLockType screen_orientation) {
+  return screen_orientation ==
+             blink::kWebScreenOrientationLockLandscapePrimary ||
+         screen_orientation ==
+             blink::kWebScreenOrientationLockPortraitSecondary;
+}
+
 bool SplitViewController::CanSnap(aura::Window* window) {
   if (!wm::CanActivateWindow(window))
     return false;

ash/wm/splitview/split_view_controller.h

@@ -63,9 +63,14 @@ class ASH_EXPORT SplitViewController : public aura::WindowObserver,
   ~SplitViewController() override;
 
   // Returns true if split view mode is supported. Currently the split view
-  // mode is only supported in tablet mode (tablet mode).
+  // mode is only supported in tablet mode.
   static bool ShouldAllowSplitView();
 
+  // Returns true if |left_window_| should be placed on the left or top side of
+  // the screen.
+  static bool IsLeftWindowOnTopOrLeftOfScreen(
+      blink::WebScreenOrientationLockType screen_orientation);
+
   // Returns true if |window| can be activated and snapped.
   bool CanSnap(aura::Window* window);