Implement scroll-snap-stop: always

As specified in the spec, when the snap-area has scroll-snap-stop:
always, we should not pass its snap position when scrolling with an
intended direction.

This patch implements this feature by adding another round of search for
the IntendedEndAndDirectionStrategy. This second round searches for the
snap position with scroll-snap-stop: always that's closest to the
scroll's start position. It then compares with the result from the first
round of search, which is the snap position closest to the scroll's
target position. The comparison selects the area closest to the scroll's
start position ensuring a an area with snap stop is never bypassed.

Bug: 823998
Change-Id: Ic40f82263ced85f8a72c8f5a82d4fb76e403398f
Reviewed-on: https://chromium-review.googlesource.com/c/1460875Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Sandra Sun <sunyunjia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634421}

cc/input/scroll_snap_data.cc

@@ -16,12 +16,6 @@ bool IsMutualVisible(const SnapSearchResult& a, const SnapSearchResult& b) {
          b.visible_range().Contains(gfx::RangeF(a.snap_offset()));
 }
 
-bool SnappableOnAxis(const SnapAreaData& area, SearchAxis search_axis) {
-  return search_axis == SearchAxis::kX
-             ? area.scroll_snap_align.alignment_inline != SnapAlignment::kNone
-             : area.scroll_snap_align.alignment_block != SnapAlignment::kNone;
-}
-
 void SetOrUpdateResult(const SnapSearchResult& candidate,
                        base::Optional<SnapSearchResult>* result) {
   if (result->has_value())
@@ -30,6 +24,30 @@ void SetOrUpdateResult(const SnapSearchResult& candidate,
     *result = candidate;
 }
 
+const base::Optional<SnapSearchResult>& ClosestSearchResult(
+    const gfx::ScrollOffset reference_point,
+    SearchAxis axis,
+    const base::Optional<SnapSearchResult>& a,
+    const base::Optional<SnapSearchResult>& b) {
+  if (!a.has_value())
+    return b;
+  if (!b.has_value())
+    return a;
+
+  float reference_position =
+      axis == SearchAxis::kX ? reference_point.x() : reference_point.y();
+  float position_a = a.value().snap_offset();
+  float position_b = b.value().snap_offset();
+  DCHECK(
+      (reference_position <= position_a && reference_position <= position_b) ||
+      (reference_position >= position_a && reference_position >= position_b));
+
+  float distance_a = std::abs(position_a - reference_position);
+  float distance_b = std::abs(position_b - reference_position);
+
+  return distance_a < distance_b ? a : b;
+}
+
 }  // namespace
 
 SnapSearchResult::SnapSearchResult(float offset, const gfx::RangeF& range)
@@ -155,6 +173,26 @@ base::Optional<SnapSearchResult> SnapContainerData::FindClosestValidArea(
     const SnapSearchResult& cros_axis_snap_result) const {
   base::Optional<SnapSearchResult> result =
       FindClosestValidAreaInternal(axis, strategy, cros_axis_snap_result);
+
+  // For EndAndDirectionStrategy, if there is a snap area with snap-stop:always,
+  // and is between the starting position and the above result, we should choose
+  // the first snap area with snap-stop:always.
+  // This additional search is executed only if we found a result, while the
+  // additional search for the relaxed_strategy is executed only if we didn't
+  // find a result. So we put this search first so we can return early if we
+  // could find a result.
+  if (result.has_value() && strategy.ShouldRespectSnapStop()) {
+    std::unique_ptr<SnapSelectionStrategy> must_only_strategy =
+        SnapSelectionStrategy::CreateForDirection(
+            strategy.current_position(),
+            strategy.intended_position() - strategy.current_position(),
+            SnapStopAlwaysFilter::kRequire);
+    base::Optional<SnapSearchResult> must_only_result =
+        FindClosestValidAreaInternal(axis, *must_only_strategy,
+                                     cros_axis_snap_result, false);
+    result = ClosestSearchResult(strategy.current_position(), axis, result,
+                                 must_only_result);
+  }
   // Our current direction based strategies are too strict ignoring the other
   // directions even when we have no candidate in the given direction. This is
   // particularly problematic with mandatory snap points and for fling
@@ -178,7 +216,8 @@ base::Optional<SnapSearchResult>
 SnapContainerData::FindClosestValidAreaInternal(
     SearchAxis axis,
     const SnapSelectionStrategy& strategy,
-    const SnapSearchResult& cros_axis_snap_result) const {
+    const SnapSearchResult& cros_axis_snap_result,
+    bool should_consider_covering) const {
   // The search result from the snap area that's closest to the search origin.
   base::Optional<SnapSearchResult> closest;
   // The search result with the intended position if it makes a snap area cover
@@ -202,11 +241,12 @@ SnapContainerData::FindClosestValidAreaInternal(
   float smallest_distance =
       axis == SearchAxis::kX ? proximity_range_.x() : proximity_range_.y();
   for (const SnapAreaData& area : snap_area_list_) {
-    if (!SnappableOnAxis(area, axis))
+    if (!strategy.IsValidSnapArea(axis, area))
       continue;
 
     SnapSearchResult candidate = GetSnapSearchResult(axis, area);
-    if (IsSnapportCoveredOnAxis(axis, intended_position, area.rect)) {
+    if (should_consider_covering &&
+        IsSnapportCoveredOnAxis(axis, intended_position, area.rect)) {
       // Since snap area will cover the snapport, we consider the intended
       // position as a valid snap position.
       SnapSearchResult covering_candidate = candidate;
@@ -229,6 +269,9 @@ SnapContainerData::FindClosestValidAreaInternal(
         closest = candidate;
       }
     }
+    if (!should_consider_covering)
+      continue;
+
     if (candidate.snap_offset() < intended_position &&
         candidate.snap_offset() > prev) {
       prev = candidate.snap_offset();
@@ -238,6 +281,7 @@ SnapContainerData::FindClosestValidAreaInternal(
       next = candidate.snap_offset();
     }
   }
+
   // According to the spec [1], if the snap area is covering the snapport, the
   // scroll position is a valid snap position only if the distance between the
   // geometrically previous and subsequent snap positions in that axis is larger

cc/input/scroll_snap_data.h

@@ -218,10 +218,13 @@ class CC_EXPORT SnapContainerData {
   // or the original scroll offset if this is the first iteration of search.
   // Returns the candidate as SnapSearchResult that includes the area's
   // |snap_offset| and its visible range on the cross axis.
+  // When |should_consider_covering| is true, the current offset can be valid if
+  // it makes a snap area cover the snapport.
   base::Optional<SnapSearchResult> FindClosestValidAreaInternal(
       SearchAxis axis,
       const SnapSelectionStrategy& strategy,
-      const SnapSearchResult& cross_axis_snap_result) const;
+      const SnapSearchResult& cross_axis_snap_result,
+      bool should_consider_covering = true) const;
 
   // A wrapper of FindClosestValidAreaInternal(). If
   // FindClosestValidAreaInternal() doesn't return a valid result when the snap

cc/input/scroll_snap_data_unittest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "cc/input/scroll_snap_data.h"
+#include <memory>
 #include "cc/input/snap_selection_strategy.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
@@ -307,4 +308,99 @@ TEST_F(ScrollSnapDataTest, MandatorySnapsBackwardIfNoValidAreaForward) {
       container.FindSnapPosition(*end_direction_strategy, &snap_position));
 }
 
+TEST_F(ScrollSnapDataTest, ShouldNotPassScrollSnapStopAlwaysElement) {
+  SnapContainerData container(
+      ScrollSnapType(false, SnapAxis::kBoth, SnapStrictness::kMandatory),
+      gfx::RectF(0, 0, 200, 200), gfx::ScrollOffset(2000, 2000));
+  SnapAreaData must_snap_1(ScrollSnapAlign(SnapAlignment::kStart),
+                           gfx::RectF(200, 0, 100, 100), true);
+  SnapAreaData must_snap_2(ScrollSnapAlign(SnapAlignment::kStart),
+                           gfx::RectF(400, 0, 100, 100), true);
+  SnapAreaData closer_to_target(ScrollSnapAlign(SnapAlignment::kStart),
+                                gfx::RectF(600, 0, 100, 100), false);
+  container.AddSnapAreaData(must_snap_1);
+  container.AddSnapAreaData(must_snap_2);
+  container.AddSnapAreaData(closer_to_target);
+  gfx::ScrollOffset snap_position;
+
+  std::unique_ptr<SnapSelectionStrategy> end_direction_strategy =
+      SnapSelectionStrategy::CreateForEndAndDirection(
+          gfx::ScrollOffset(0, 0), gfx::ScrollOffset(600, 0));
+  EXPECT_TRUE(
+      container.FindSnapPosition(*end_direction_strategy, &snap_position));
+
+  // Even though closer_to_target and must_snap_2 are closer to the target
+  // position of the scroll, the must_snap_1 which is closer to the start
+  // shouldn't be passed.
+  EXPECT_EQ(200, snap_position.x());
+  EXPECT_EQ(0, snap_position.y());
+}
+
+TEST_F(ScrollSnapDataTest, SnapStopAlwaysOverridesCoveringSnapArea) {
+  SnapContainerData container(
+      ScrollSnapType(false, SnapAxis::kBoth, SnapStrictness::kMandatory),
+      gfx::RectF(0, 0, 200, 200), gfx::ScrollOffset(600, 800));
+  SnapAreaData stop_area(ScrollSnapAlign(SnapAlignment::kStart),
+                         gfx::RectF(100, 0, 100, 100), true);
+  SnapAreaData covering_area(ScrollSnapAlign(SnapAlignment::kStart),
+                             gfx::RectF(250, 0, 600, 600), false);
+  container.AddSnapAreaData(stop_area);
+  container.AddSnapAreaData(covering_area);
+
+  gfx::ScrollOffset snap_position;
+  std::unique_ptr<SnapSelectionStrategy> strategy =
+      SnapSelectionStrategy::CreateForEndAndDirection(
+          gfx::ScrollOffset(0, 0), gfx::ScrollOffset(300, 0));
+  // The fling is from (0, 0) to (300, 0), and the destination would make
+  // the |covering_area| perfectly cover the snapport. However, another area
+  // with snap-stop:always precedes this |covering_area| so we snap at
+  // (100, 100).
+  EXPECT_TRUE(container.FindSnapPosition(*strategy, &snap_position));
+  EXPECT_EQ(100, snap_position.x());
+  EXPECT_EQ(0, snap_position.y());
+}
+
+TEST_F(ScrollSnapDataTest, SnapStopAlwaysInReverseDirection) {
+  SnapContainerData container(
+      ScrollSnapType(false, SnapAxis::kBoth, SnapStrictness::kMandatory),
+      gfx::RectF(0, 0, 200, 300), gfx::ScrollOffset(600, 800));
+  SnapAreaData stop_area(ScrollSnapAlign(SnapAlignment::kStart),
+                         gfx::RectF(100, 0, 100, 100), true);
+  container.AddSnapAreaData(stop_area);
+
+  gfx::ScrollOffset snap_position;
+  std::unique_ptr<SnapSelectionStrategy> strategy =
+      SnapSelectionStrategy::CreateForEndAndDirection(
+          gfx::ScrollOffset(150, 0), gfx::ScrollOffset(200, 0));
+  // The fling is from (150, 0) to (350, 0), but the snap position is in the
+  // reverse direction at (100, 0). Since the container has mandatory for
+  // snapstrictness, we should go back to snap at (100, 0).
+  EXPECT_TRUE(container.FindSnapPosition(*strategy, &snap_position));
+  EXPECT_EQ(100, snap_position.x());
+  EXPECT_EQ(0, snap_position.y());
+}
+
+TEST_F(ScrollSnapDataTest, SnapStopAlwaysNotInterferingWithDirectionStrategy) {
+  SnapContainerData container(
+      ScrollSnapType(false, SnapAxis::kBoth, SnapStrictness::kMandatory),
+      gfx::RectF(0, 0, 200, 300), gfx::ScrollOffset(600, 800));
+  SnapAreaData closer_area(ScrollSnapAlign(SnapAlignment::kStart),
+                           gfx::RectF(100, 0, 1, 1), false);
+  SnapAreaData stop_area(ScrollSnapAlign(SnapAlignment::kStart),
+                         gfx::RectF(120, 0, 1, 1), true);
+  container.AddSnapAreaData(closer_area);
+  container.AddSnapAreaData(stop_area);
+
+  // The DirectionStrategy should always choose the first snap position
+  // regardless its scroll-snap-stop value.
+  gfx::ScrollOffset snap_position;
+  std::unique_ptr<SnapSelectionStrategy> direction_strategy =
+      SnapSelectionStrategy::CreateForDirection(gfx::ScrollOffset(90, 0),
+                                                gfx::ScrollOffset(50, 0));
+  snap_position = gfx::ScrollOffset();
+  EXPECT_TRUE(container.FindSnapPosition(*direction_strategy, &snap_position));
+  EXPECT_EQ(100, snap_position.x());
+  EXPECT_EQ(0, snap_position.y());
+}
+
 }  // namespace cc

cc/input/snap_selection_strategy.cc

@@ -17,8 +17,9 @@ SnapSelectionStrategy::CreateForEndPosition(
 
 std::unique_ptr<SnapSelectionStrategy>
 SnapSelectionStrategy::CreateForDirection(gfx::ScrollOffset current_position,
-                                          gfx::ScrollOffset step) {
-  return std::make_unique<DirectionStrategy>(current_position, step);
+                                          gfx::ScrollOffset step,
+                                          SnapStopAlwaysFilter filter) {
+  return std::make_unique<DirectionStrategy>(current_position, step, filter);
 }
 
 std::unique_ptr<SnapSelectionStrategy>
@@ -33,6 +34,17 @@ bool SnapSelectionStrategy::HasIntendedDirection() const {
   return true;
 }
 
+bool SnapSelectionStrategy::ShouldRespectSnapStop() const {
+  return false;
+}
+
+bool SnapSelectionStrategy::IsValidSnapArea(SearchAxis axis,
+                                            const SnapAreaData& area) const {
+  return axis == SearchAxis::kX
+             ? area.scroll_snap_align.alignment_inline != SnapAlignment::kNone
+             : area.scroll_snap_align.alignment_block != SnapAlignment::kNone;
+}
+
 bool EndPositionStrategy::ShouldSnapOnX() const {
   return scrolled_x_;
 }
@@ -95,6 +107,13 @@ bool DirectionStrategy::IsValidSnapPosition(SearchAxis axis,
   }
 }
 
+bool DirectionStrategy::IsValidSnapArea(SearchAxis axis,
+                                        const SnapAreaData& area) const {
+  return SnapSelectionStrategy::IsValidSnapArea(axis, area) &&
+         (snap_stop_always_filter_ == SnapStopAlwaysFilter::kIgnore ||
+          area.must_snap);
+}
+
 const base::Optional<SnapSearchResult>& DirectionStrategy::PickBestResult(
     const base::Optional<SnapSearchResult>& closest,
     const base::Optional<SnapSearchResult>& covering) const {
@@ -150,6 +169,10 @@ bool EndAndDirectionStrategy::IsValidSnapPosition(SearchAxis axis,
   }
 }
 
+bool EndAndDirectionStrategy::ShouldRespectSnapStop() const {
+  return true;
+}
+
 const base::Optional<SnapSearchResult>& EndAndDirectionStrategy::PickBestResult(
     const base::Optional<SnapSearchResult>& closest,
     const base::Optional<SnapSearchResult>& covering) const {

cc/input/snap_selection_strategy.h

@@ -11,6 +11,8 @@
 
 namespace cc {
 
+enum class SnapStopAlwaysFilter { kIgnore, kRequire };
+
 // This class represents an abstract strategy that decide which snap selection
 // should be considered valid. There are concrete implementations for three core
 // scrolling types: scroll with end position only, scroll with direction only,
@@ -25,7 +27,8 @@ class CC_EXPORT SnapSelectionStrategy {
       bool scrolled_y);
   static std::unique_ptr<SnapSelectionStrategy> CreateForDirection(
       gfx::ScrollOffset current_position,
-      gfx::ScrollOffset step);
+      gfx::ScrollOffset step,
+      SnapStopAlwaysFilter filter = SnapStopAlwaysFilter::kIgnore);
   static std::unique_ptr<SnapSelectionStrategy> CreateForEndAndDirection(
       gfx::ScrollOffset current_position,
       gfx::ScrollOffset displacement);
@@ -47,9 +50,14 @@ class CC_EXPORT SnapSelectionStrategy {
   // Returns true if the selection strategy considers the given snap offset
   // valid for the current axis.
   virtual bool IsValidSnapPosition(SearchAxis axis, float position) const = 0;
+  virtual bool IsValidSnapArea(SearchAxis axis, const SnapAreaData& data) const;
 
   virtual bool HasIntendedDirection() const;
 
+  // Returns true if a snap area with scroll-snap-stop:always should not be
+  // bypassed.
+  virtual bool ShouldRespectSnapStop() const;
+
   // Returns the best result according to snap selection strategy. This method
   // is called at the end of selection process to make the final decision.
   //
@@ -116,8 +124,11 @@ class EndPositionStrategy : public SnapSelectionStrategy {
 class DirectionStrategy : public SnapSelectionStrategy {
  public:
   DirectionStrategy(const gfx::ScrollOffset& current_position,
-                    const gfx::ScrollOffset& step)
-      : SnapSelectionStrategy(current_position), step_(step) {}
+                    const gfx::ScrollOffset& step,
+                    SnapStopAlwaysFilter filter)
+      : SnapSelectionStrategy(current_position),
+        step_(step),
+        snap_stop_always_filter_(filter) {}
   ~DirectionStrategy() override = default;
 
   bool ShouldSnapOnX() const override;
@@ -127,6 +138,8 @@ class DirectionStrategy : public SnapSelectionStrategy {
   gfx::ScrollOffset base_position() const override;
 
   bool IsValidSnapPosition(SearchAxis axis, float position) const override;
+  bool IsValidSnapArea(SearchAxis axis,
+                       const SnapAreaData& area) const override;
 
   const base::Optional<SnapSearchResult>& PickBestResult(
       const base::Optional<SnapSearchResult>& closest,
@@ -135,6 +148,7 @@ class DirectionStrategy : public SnapSelectionStrategy {
  private:
   // The default step for this DirectionStrategy.
   const gfx::ScrollOffset step_;
+  SnapStopAlwaysFilter snap_stop_always_filter_;
 };
 
 // Examples for intended direction and end position scrolls include
@@ -160,6 +174,8 @@ class EndAndDirectionStrategy : public SnapSelectionStrategy {
 
   bool IsValidSnapPosition(SearchAxis axis, float position) const override;
 
+  bool ShouldRespectSnapStop() const override;
+
   const base::Optional<SnapSearchResult>& PickBestResult(
       const base::Optional<SnapSearchResult>& closest,
       const base::Optional<SnapSearchResult>& covering) const override;

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/scroll-snap-stop-always.html

+<!DOCTYPE html>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap/#scroll-snap-stop" />
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+div {
+  position: absolute;
+}
+#scroller {
+  width: 400px;
+  height: 400px;
+  overflow: scroll;
+  scroll-snap-type: both mandatory;
+}
+#space {
+  left: 0px;
+  top: 0px;
+  width: 2100px;
+  height: 2100px;
+}
+.target {
+  width: 50px;
+  height: 50px;
+  scroll-snap-align: start;
+}
+.origin {
+  left: 0px;
+  top: 0px;
+}
+.always-stop {
+  left: 100px;
+  top: 0px;
+  scroll-snap-stop: always;
+}
+.closer {
+  left: 200px;
+  top: 0px;
+}
+</style>
+
+<div id="scroller">
+  <div id="space"></div>
+  <div class="target origin"></div>
+  <div class="target always-stop"></div>
+  <div class="target closer"></div>
+</div>
+
+<script>
+var scroller = document.getElementById("scroller");
+test(() => {
+  scroller.scrollTo(0, 0);
+  assert_equals(scroller.scrollLeft, 0);
+  assert_equals(scroller.scrollTop, 0);
+
+  scroller.scrollBy(300, 0);
+  assert_equals(scroller.scrollLeft, 100);
+  assert_equals(scroller.scrollTop, 0);
+}, "A scroll with intended direction and end position should not pass a snap " +
+   "area with scroll-snap-stop: always.")
+
+test(() => {
+  scroller.scrollTo(0, 0);
+  assert_equals(scroller.scrollLeft, 0);
+  assert_equals(scroller.scrollTop, 0);
+
+  scroller.scrollTo(300, 0);
+  assert_equals(scroller.scrollLeft, 200);
+  assert_equals(scroller.scrollTop, 0);
+}, "A scroll with intended end position should always choose the closest snap " +
+   "position regardless of the scroll-snap-stop value.")
+</script>