Bug 1546057 - Use the clamped destination position for overflow checks. r=botond
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Wed, 24 Apr 2019 21:56:26 +0000
changeset 530036 eda47278259fdd88b27f573b1d848435f7acc4b4
parent 530035 4e06efa9d48ede0a40fdbb321c870986e72a0cae
child 530037 23fa85727bd08cecb31c0bd43f27d9507a6189a6
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1546057
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1546057 - Use the clamped destination position for overflow checks. r=botond Differential Revision: https://phabricator.services.mozilla.com/D28578
layout/generic/ScrollSnap.cpp
testing/web-platform/tests/css/css-scroll-snap/overflowing-snap-areas.html
--- a/layout/generic/ScrollSnap.cpp
+++ b/layout/generic/ScrollSnap.cpp
@@ -285,29 +285,32 @@ Maybe<nsPoint> ScrollSnapUtils::GetSnapP
   if (StaticPrefs::layout_css_scroll_snap_v1_enabled()) {
     ProcessSnapPositions(calcSnapPoints, aSnapInfo);
 
     // If the distance between the first and the second candidate snap points
     // is larger than the snapport size and the snapport is covered by larger
     // elements, any points inside the covering area should be valid snap
     // points.
     // https://drafts.csswg.org/css-scroll-snap-1/#snap-overflow
+    // NOTE: |aDestination| sometimes points outside of the scroll range, e.g.
+    // by the APZC fling, so for the overflow checks we need to clamp it.
+    nsPoint clampedDestination = aScrollRange.ClampPoint(aDestination);
     for (auto range : aSnapInfo.mXRangeWiderThanSnapport) {
-      if (range.IsValid(aDestination.x, aSnapInfo.mSnapportSize.width) &&
+      if (range.IsValid(clampedDestination.x, aSnapInfo.mSnapportSize.width) &&
           calcSnapPoints.XDistanceBetweenBestAndSecondEdge() >
               aSnapInfo.mSnapportSize.width) {
-        calcSnapPoints.AddVerticalEdge(aDestination.x);
+        calcSnapPoints.AddVerticalEdge(clampedDestination.x);
         break;
       }
     }
     for (auto range : aSnapInfo.mYRangeWiderThanSnapport) {
-      if (range.IsValid(aDestination.y, aSnapInfo.mSnapportSize.height) &&
+      if (range.IsValid(clampedDestination.y, aSnapInfo.mSnapportSize.height) &&
           calcSnapPoints.YDistanceBetweenBestAndSecondEdge() >
               aSnapInfo.mSnapportSize.height) {
-        calcSnapPoints.AddHorizontalEdge(aDestination.y);
+        calcSnapPoints.AddHorizontalEdge(clampedDestination.y);
         break;
       }
     }
   } else {
     nsPoint destPos = aSnapInfo.mScrollSnapDestination;
 
     if (aSnapInfo.mScrollSnapIntervalX.isSome()) {
       nscoord interval = aSnapInfo.mScrollSnapIntervalX.value();
--- a/testing/web-platform/tests/css/css-scroll-snap/overflowing-snap-areas.html
+++ b/testing/web-platform/tests/css/css-scroll-snap/overflowing-snap-areas.html
@@ -58,16 +58,17 @@ div {
 
 <div class="scroller-y" id="y">
   <div class="space"></div>
   <div style="top: 200px;">
     <div class="target large-y"></div>
     <div class="target small" style="top: 200px"></div>
     <div class="target small" style="top: 600px"></div>
     <div class="target small" style="top: 1200px"></div>
+    <div class="target large-y" style="top: 2000px"></div>
   </div>
 </div>
 
 <div class="scroller-x" id="two-axes" style="scroll-snap-type: both mandatory">
   <div class="space"></div>
   <div class="target large-x" style="top: 200px"></div>
 </div>
 
@@ -139,15 +140,29 @@ test(() => {
 test(() => {
   scroller_y.scrollTo(0, 1500);
   assert_equals(scroller_y.scrollLeft, 0);
   assert_equals(scroller_y.scrollTop, 1500);
 }, "Snap to current scroll position which is a valid snap position, as the " +
    "snap area covers snapport on y and there is no subsequent snap positions.");
 
 test(() => {
+  const maxScrollTop = scroller_y.scrollHeight - scroller_y.clientHeight;
+
+  // Scroll to the bottom edge which is a valid snap position that a large
+  // target element covers the snapport.
+  scroller_y.scrollTo(0, maxScrollTop);
+  assert_equals(scroller_y.scrollTop, maxScrollTop);
+
+  // Scroll to `the bottom edge + 1`.
+  scroller_y.scrollTo(0, maxScrollTop + 1);
+  assert_equals(scroller_y.scrollTop, maxScrollTop);
+}, "Don't snap back even if scrollTo tries to scroll to positions which are " +
+   "outside of the scroll range and if a snap target element covers the snaport");
+
+test(() => {
   two_axes_scroller.scrollTo(10, 100);
   assert_equals(two_axes_scroller.scrollLeft, 10);
   assert_equals(two_axes_scroller.scrollTop, 200);
 }, "Snap to current scroll position on x as the area is covering x axis." +
    "However, we snap to the specified snap position on y as the area is not " +
    "covering y axis.");
 </script>