Choose closest covering position for large snap areas

This patch modifies the logic to find snap positions so that when a scroller is scrolled to a position which only partially covers a snap
area larger than itself, if it snaps to this large snap area, it snaps
to the nearest boundary of that snap area where the snap area covers the
snap port, rather than doing a jump to honor the specified scroll-snap-align.

Below are a few notes about changes made to tests:

snap-area-overflow-boundary.html is corrected so that both test cases
correctly verify the scroll offset when snapped to the lower element.
The scroll amount in the second test case is also adjusted to ensure
it is enough to snap to the lower div.

The expectations at the end of
scroll-on-large-element-not-covering-snapport.tentative.html are
updated to account for choosing a snap position different from the
scroll-snap-align-specified position for snap areas larger than their
snapports.

scroll-snap-type-on-root-element.html is updated to account for
scrollbar width since snapping to the right edge of the target is
valid.

This patch picks new programmatic scroll offsets for
snap-to-visible-areas-margin-both.html so that the top-left of
intersection of the right-top and left-bottom targets' scroll-margins
is closer than the bottom-right of the intersection.

scrollend-with-snap-on-fractional-offset.html and
scroll-start-with-scroll-snap-tentative.html are adjusted to use snap
area sizes that don't cover the snap port.

(cherry picked from commit 0295a2c)

Bug: 1420762
Change-Id: I72d18b8ca44ea5aef015e59b67e4de34a66d8a1e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4806971
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: David Awogbemila <awogbemila@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1193634}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4853949
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: David Awogbemila <awogbemila@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/branch-heads/5993@{#188}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}

cc/base/features.cc

@@ -53,6 +53,10 @@ BASE_FEATURE(kScrollSnapCoveringUseNativeFling,
              "ScrollSnapCoveringUseNativeFling",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
+BASE_FEATURE(kScrollSnapPreferCloserCovering,
+             "ScrollSnapPreferCloserCovering",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 BASE_FEATURE(kHudDisplayForPerformanceMetrics,
              "HudDisplayForPerformanceMetrics",
              base::FEATURE_DISABLED_BY_DEFAULT);

cc/base/features.h

@@ -47,6 +47,12 @@ CC_BASE_EXPORT BASE_DECLARE_FEATURE(kScrollSnapCoveringAvoidNestedSnapAreas);
 // allowing much more natural scrolling within these areas.
 CC_BASE_EXPORT BASE_DECLARE_FEATURE(kScrollSnapCoveringUseNativeFling);
 
+// When enabled, if a snap container is snapping to a large snap area, it will
+// snap to the closest covering position if it is closer than the aligned
+// position. This avoid unnecessary jumps that attempt to honor the
+// scroll-snap-align value.
+CC_BASE_EXPORT BASE_DECLARE_FEATURE(kScrollSnapPreferCloserCovering);
+
 // When enabled, cc will show blink's Web-Vital metrics inside its heads up
 // display.
 CC_BASE_EXPORT BASE_DECLARE_FEATURE(kHudDisplayForPerformanceMetrics);

cc/input/scroll_snap_data.cc

@@ -133,6 +133,15 @@ absl::optional<SnapSearchResult> SearchResultForDodgingRange(
   return result;
 }
 
+bool CanCoverSnapportOnAxis(SearchAxis axis,
+                            const gfx::RectF& container_rect,
+                            const gfx::RectF& area_rect) {
+  return (axis == SearchAxis::kY &&
+          area_rect.height() >= container_rect.height()) ||
+         (axis == SearchAxis::kX &&
+          area_rect.width() >= container_rect.width());
+}
+
 }  // namespace
 
 SnapSearchResult::SnapSearchResult(float offset, const gfx::RangeF& range)
@@ -504,11 +513,10 @@ SnapContainerData::FindClosestValidAreaInternal(
 
     SnapSearchResult candidate = GetSnapSearchResult(axis, area);
     evaluate(candidate);
-
     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.
+        (base::FeatureList::IsEnabled(features::kScrollSnapPreferCloserCovering)
+             ? CanCoverSnapportOnAxis(axis, rect_, area.rect)
+             : IsSnapportCoveredOnAxis(axis, intended_position, area.rect))) {
       if (absl::optional<SnapSearchResult> covering =
               FindCoveringCandidate(area, axis, candidate, intended_position)) {
         if (covering->snap_offset() == intended_position) {

cc/input/scroll_snap_data_unittest.cc

@@ -798,11 +798,11 @@ TEST_F(ScrollSnapDataTest, CoveringWithOverlap1) {
   // Snap up, out of small_1.
   TestSnapPositionY(container, 2000, -150, Type::kCovered, 1800, 50, 1800);
   // Snap to small_2.
-  TestSnapPositionY(container, 1600, 600, Type::kAligned, 2300);
+  TestSnapPositionY(container, 1600, 601, Type::kAligned, 2300);
   // Scroll inside small_2.
   TestSnapPositionY(container, 2300, 50, Type::kCovered, 2350, 2300, 2400);
   // Snap out of small_2.
-  TestSnapPositionY(container, 2300, 150, Type::kCovered, 2600, 2600, 4750);
+  TestSnapPositionY(container, 2300, 200, Type::kCovered, 2600, 2600, 4750);
   // Snap up into small_2.
   TestSnapPositionY(container, 2700, -300, Type::kCovered, 2400, 2300, 2400);
 }

third_party/blink/web_tests/external/wpt/css/css-scroll-snap-2/scroll-start/scroll-start-with-scroll-snap.tentative.html

@@ -17,21 +17,27 @@
     }
 
     .spacer {
-      height: 100px;
+      height: 200px;
       width: 100px;
     }
 
     .scroller {
-      height: 100px;
+      height: 220px;
       width: 100px;
       overflow: scroll;
-      scroll-start: 200px;
       scroll-snap-type: both mandatory;
     }
 
+    #single_snap_scroller {
+      scroll-start: 100%;
+    }
+    #multi_snap_scroller {
+      scroll-start: 350px;
+    }
+
     .snap_point {
       width: 100px;
-      height: 100px;
+      height: 200px;
       scroll-snap-align: start;
     }
   </style>
@@ -53,11 +59,11 @@
     }, "snap overrides scroll-start position");
 
     test((t) => {
-      // scroll-start sets the initial scroll offset to the top of the third
-      // snap_point, so the scroller snaps to the third snap_point.
+      // scroll-start sets the initial scroll offset to 350px which is closer to
+      // the third snap point than the second, so the scroller should snap to
+      // the third snap_point.
       assert_equals(multi_snap_scroller.scrollTop,
-        snap_point_1.getBoundingClientRect().height +
-        snap_point_2.getBoundingClientRect().height,
+        multi_snap_scroller.scrollHeight - multi_snap_scroller.clientHeight,
         "scroller snaps to snap point closer to start position.");
     }, "scroller snaps based on scroll-start position");
   </script>

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/input/snap-area-overflow-boundary.html

@@ -3,6 +3,9 @@
 <title></title>
 <meta name="assert" content="Test passes if snap is to the nearest edge">
 <style>
+  body {
+    margin: 0px;
+  }
   #scroller {
     scroll-snap-type: block mandatory;
     overflow-y: scroll;
@@ -105,7 +108,7 @@
   await keyPress(scroller, "ArrowDown");
   await waitForAnimationEnd(scrollTop);
   assert_equals(scroller.scrollTop,
-                next.clientTop,
+                next.offsetTop,
                 'Advance to next snap-area');
 
 }, "Keyboard scrolling with vertical snap-area overflow");
@@ -126,7 +129,7 @@
 
   // Target position for wheel scroll overshoots the boundary of the snap-area.
   // Ensure that we stop at the boundary.
-  const scrollAmount =
+  let scrollAmount =
       target.clientHeight - scroller.clientHeight - scroller.scrollTop + 1;
 
   await new test_driver.Actions()
@@ -137,13 +140,15 @@
                       'End boundary of snap-area is valid snap target');
 
   // Must not get stuck at a snap position. Since already at the end of the
-  // snap area, we should advance to the next.
+  // snap area, we should advance to the next. scrollAmount must be enough to
+  // advance to next snap position.
+  scrollAmount = next.clientHeight / 2 + 10 /* margin-bottom */;
   await new test_driver.Actions()
-       .scroll(50, 50, 0, 50, {origin: scroller})
+       .scroll(50, 50, 0, scrollAmount, {origin: scroller})
        .send();
   await waitForAnimationEnd(scrollTop);
   assert_equals(scroller.scrollTop,
-                next.clientTop,
+                next.offsetTop,
                 'Advance to next snap-area');
 
 }, "Mouse-wheel scrolling with vertical snap-area overflow");

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/scroll-on-large-element-not-covering-snapport.tentative.html

@@ -80,12 +80,9 @@
 
   // Scroll back a bit.
   scroller.scrollBy(0, -10);
-  // Now, snap back to the third snap point because at the moment when scrolling
-  // up by 10px, the large third snap target element isn't covering over the
-  // snapport, i.e. only bottom 10px of the large element is in the snapport.
-  // See https://github.com/w3c/csswg-drafts/issues/7262
+  // This should snap to the bottom of the 3rd snap area and not all the way
+  // back up to its top.
   assert_equals(scroller.scrollLeft, 0);
-  assert_equals(scroller.scrollTop, 80);
-}, 'There\'s no valid snap positions on large element if it doesn\'t cover ' +
-   'the snapport');
+  assert_equals(scroller.scrollTop, 880);
+}, "snaps to bottom edge of large snap area that doesn't cover the snap port.");
 </script>

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/scroll-snap-type-on-root-element.html

@@ -4,12 +4,13 @@
 <meta name="viewport" content="width=device-width,initial-scale=1,minimum-scale=1">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
+<body>
 <style>
-html {
+body {
   height: 8000px;
   width: 3000px;
+  margin: 0px;
 }
-
 #target {
   position: absolute;
   background-color: blue;
@@ -68,7 +69,13 @@
 
   window.scrollTo(200, 1800);
 
-  assert_equals(document.scrollingElement.scrollLeft, 100, "inline should snap");
+  // Though the target's width is 100vw, there may be room to scroll the window
+  // within the target because of the scrollbar width and the browser may snap
+  // to the right edge (instead of the left edge of the target, since it is
+  // closer to the offset of 200 and still a valid snap point) within this room.
+  const scrollbar_width = window.innerWidth - document.documentElement.clientWidth;
+  assert_approx_equals(document.scrollingElement.scrollLeft, 100, scrollbar_width, "inline should snap");
   assert_equals(document.scrollingElement.scrollTop, 1800, "block should not snap");
 }, "The writing-mode (horizontal-tb) on the body is used ");
 </script>
+</body>

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000-ref.html

@@ -35,28 +35,47 @@
 .ltr { direction: ltr; }
 .rtl { direction: rtl; }
 
-.TB.large.invert .target { top: auto; }
-.LR.large.invert .target { left: auto; }
-.RL.large.invert .target { right: auto; }
-
-.TB.ltr.large.invert .target { left:   auto; }
-.TB.rtl.large.invert .target { right:  auto; }
-.LR.ltr.large.invert .target { top:    auto; }
-.LR.rtl.large.invert .target { bottom: auto; }
-.RL.ltr.large.invert .target { top:    auto; }
-.RL.rtl.large.invert .target { bottom: auto; }
-
 /* not absolutizing the border colors, so that the test passes even if css-logical is not supported; */
 .large.invert {
   border: solid silver;
   border-block-end-color: blue;
   border-inline-end-color: blue;
 }
 
+.TB.large.invert .target::before { top: auto; }
+.LR.large.invert .target::before { left: auto; }
+.RL.large.invert .target::before { right: auto; }
+
+.TB.ltr.large.invert .target::before { left:   auto; }
+.TB.rtl.large.invert .target::before { right:  auto; }
+.LR.ltr.large.invert .target::before { top:    auto; }
+.LR.rtl.large.invert .target::before { bottom: auto; }
+.RL.ltr.large.invert .target::before { top:    auto; }
+.RL.rtl.large.invert .target::before { bottom: auto; }
+
+.large.invert .target::before {
+  width: 9px;
+  height: 9px;
+  background: orange;
+  top: 0; left: 0; right: 0; bottom: 0;
+  position: absolute;
+  content: '';
+}
+
+.large.invert .target {
+  display: block;
+  background: none;
+  width: 30px;
+  height: 30px;
+  border-block-start: 20px solid red;
+  border-inline-start: 20px solid red;
+}
+
 </style>
 
 <p>Test passes if there is an orange square tucked into each blue corner without gaps,
-and there is no red.
+and there is no red, except for the large inverted cases which should have red
+in the silver corner and smaller orange boxes in the blue corner.
 
 <div class="wrapper">
 <!-- Simple Small Cases -->
@@ -87,6 +106,7 @@
 
 <!-- Target-inverted Small Cases
      This row should be identical to the previous. -->
+
 <div class="scroller TB ltr small invert">
   <div class="target"></div>
 </div>
@@ -165,3 +185,4 @@
 </div>
 
 </div> <!-- wrapper -->
+

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html

@@ -78,7 +78,8 @@
 </style>
 
 <p>Test passes if there is an orange square tucked into each blue corner without gaps,
-and there is no red.
+  and there is no red, except for the large inverted cases which should have red
+  in the silver corner and smaller orange boxes in the blue corner.
 
 <div class="wrapper">
 <!-- Simple Small Cases -->

third_party/blink/web_tests/external/wpt/css/css-scroll-snap/snap-into-covering-area.html

@@ -0,0 +1,77 @@
+<!DOCTYPE html>
+<html>
+
+<head>
+  <link rel="help" href="https://drafts.csswg.org/css-scroll-snap-1/#scroll-snap-type" />
+  <script src="/resources/testharness.js"></script>
+  <script src="/resources/testharnessreport.js"></script>
+  <script src="/resources/testdriver.js"></script>
+  <script src="/resources/testdriver-actions.js"></script>
+  <script src="/resources/testdriver-vendor.js"></script>
+  <script src="/dom/events/scrolling/scroll_support.js"></script>
+</head>
+
+<body>
+  <style>
+    #scroller {
+      overflow: scroll;
+      height: 500px;
+      width: 500px;
+      background-color: blue;
+      scroll-snap-type: y mandatory;
+      position: absolute;
+    }
+
+    .snap_point {
+      scroll-snap-align: start;
+      width: 40%;
+      position: relative;
+      left: 30%;
+    }
+
+    .big {
+      height: 1000%;
+      background-color: pink;
+      border: solid 1px red;
+    }
+
+    .small {
+      height: 50%;
+      background-color: purple;
+      border: solid 1px black;
+    }
+  </style>
+  <div id="scroller">
+    <div class="big snap_point" id="big_snap_point"></div>
+    <div class="small snap_point">
+    <button id="scrollerButton">scrollerButton</button>
+    </div>
+  </div>
+  <script>
+    promise_test(async(t) => {
+      const x = scroller.clientWidth / 2;
+      const y = scroller.clientHeight / 2;
+
+      // Scroll all the way down to the smaller snap area which doesn't cover
+      // the snapport.
+      let scrollend_promise = new Promise((resolve) => {
+        scroller.addEventListener("scrollend", resolve);
+      });
+      scroller.scrollTop = scroller.scrollHeight;
+      await scrollend_promise;
+
+      // Scroll up with one press of the arrow-up button.
+      scrollend_promise = new Promise((resolve) => {
+        scroller.addEventListener("scrollend", resolve);
+      });
+      const arrowUp = '\uE013';
+      await test_driver.send_keys(scrollerButton, arrowUp);
+
+      await scrollend_promise;
+      assert_equals(scroller.scrollTop, big_snap_point.offsetHeight - scroller.clientHeight,
+          "scroller is snapped to the bottom of the larger snap area, not the top");
+    });
+  </script>
+</body>
+
+</html>