Bug 1766192 - Do `snap-scope` with the snapped point rather than the destination point. r=botond

Differential Revision: https://phabricator.services.mozilla.com/D144533

gfx/layers/apz/test/gtest/TestSnapping.cpp

@@ -29,11 +29,15 @@ TEST_F(APZCSnappingTesterMock, Bug1265510) {
 
   ScrollSnapInfo snap;
   snap.mScrollSnapStrictnessY = StyleScrollSnapStrictness::Mandatory;
+  snap.mSnapportSize = CSSSize::ToAppUnits(
+      layerVisibleRegion[0].GetBounds().Size() * LayerToCSSScale(1.0));
 
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+  snap.mSnapTargets.AppendElement(
-      Nothing(), Some(0 * AppUnitsPerCSSPixel()), nsRect()));
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(0 * AppUnitsPerCSSPixel()),
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+                                 CSSRect::ToAppUnits(CSSRect(0, 0, 10, 10))));
-      Nothing(), Some(100 * AppUnitsPerCSSPixel()), nsRect()));
+  snap.mSnapTargets.AppendElement(
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(100 * AppUnitsPerCSSPixel()),
+                                 CSSRect::ToAppUnits(CSSRect(0, 100, 10, 10))));
 
   ModifyFrameMetrics(root, [&](ScrollMetadata& aSm, FrameMetrics&) {
     aSm.SetSnapInfo(ScrollSnapInfo(snap));
@@ -102,11 +106,15 @@ TEST_F(APZCSnappingTesterMock, Snap_After_Pinch) {
   // Set up some basic scroll snapping
   ScrollSnapInfo snap;
   snap.mScrollSnapStrictnessY = StyleScrollSnapStrictness::Mandatory;
+  snap.mSnapportSize = CSSSize::ToAppUnits(
+      layerVisibleRegion[0].GetBounds().Size() * LayerToCSSScale(1.0));
 
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+  snap.mSnapTargets.AppendElement(
-      Nothing(), Some(0 * AppUnitsPerCSSPixel()), nsRect()));
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(0 * AppUnitsPerCSSPixel()),
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+                                 CSSRect::ToAppUnits(CSSRect(0, 0, 10, 10))));
-      Nothing(), Some(100 * AppUnitsPerCSSPixel()), nsRect()));
+  snap.mSnapTargets.AppendElement(
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(100 * AppUnitsPerCSSPixel()),
+                                 CSSRect::ToAppUnits(CSSRect(0, 100, 10, 10))));
 
   // Save the scroll snap info on the root APZC.
   // Also mark the root APZC as "root content", since APZC only allows
@@ -156,10 +164,14 @@ TEST_F(APZCSnappingTesterMock, SnapOnPanEndWithZeroVelocity) {
   // Set up two snap points, 30 and 100.
   ScrollSnapInfo snap;
   snap.mScrollSnapStrictnessY = StyleScrollSnapStrictness::Mandatory;
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+  snap.mSnapportSize = CSSSize::ToAppUnits(
-      Nothing(), Some(30 * AppUnitsPerCSSPixel()), nsRect()));
+      layerVisibleRegion[0].GetBounds().Size() * LayerToCSSScale(1.0));
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+  snap.mSnapTargets.AppendElement(
-      Nothing(), Some(100 * AppUnitsPerCSSPixel()), nsRect()));
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(30 * AppUnitsPerCSSPixel()),
+                                 CSSRect::ToAppUnits(CSSRect(0, 30, 10, 10))));
+  snap.mSnapTargets.AppendElement(
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(100 * AppUnitsPerCSSPixel()),
+                                 CSSRect::ToAppUnits(CSSRect(0, 100, 10, 10))));
 
   // Save the scroll snap info on the root APZC.
   ModifyFrameMetrics(root, [&](ScrollMetadata& aSm, FrameMetrics& aMetrics) {
@@ -227,10 +239,14 @@ TEST_F(APZCSnappingTesterMock, SnapOnPanEndWithPositiveVelocity) {
   // Set up two snap points, 30 and 100.
   ScrollSnapInfo snap;
   snap.mScrollSnapStrictnessY = StyleScrollSnapStrictness::Mandatory;
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+  snap.mSnapportSize = CSSSize::ToAppUnits(
-      Nothing(), Some(30 * AppUnitsPerCSSPixel()), nsRect()));
+      layerVisibleRegion[0].GetBounds().Size() * LayerToCSSScale(1.0));
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+  snap.mSnapTargets.AppendElement(
-      Nothing(), Some(100 * AppUnitsPerCSSPixel()), nsRect()));
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(30 * AppUnitsPerCSSPixel()),
+                                 CSSRect::ToAppUnits(CSSRect(0, 30, 10, 10))));
+  snap.mSnapTargets.AppendElement(
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(100 * AppUnitsPerCSSPixel()),
+                                 CSSRect::ToAppUnits(CSSRect(0, 100, 10, 10))));
 
   // Save the scroll snap info on the root APZC.
   ModifyFrameMetrics(root, [&](ScrollMetadata& aSm, FrameMetrics& aMetrics) {

gfx/layers/apz/test/gtest/TestSnappingOnMomentum.cpp

@@ -27,11 +27,14 @@ TEST_F(APZCSnappingOnMomentumTesterMock, Snap_On_Momentum) {
   // Set up some basic scroll snapping
   ScrollSnapInfo snap;
   snap.mScrollSnapStrictnessY = StyleScrollSnapStrictness::Mandatory;
-
+  snap.mSnapportSize = CSSSize::ToAppUnits(
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+      layerVisibleRegion[0].GetBounds().Size() * LayerToCSSScale(1.0));
-      Nothing(), Some(0 * AppUnitsPerCSSPixel()), nsRect()));
+  snap.mSnapTargets.AppendElement(
-  snap.mSnapTargets.AppendElement(ScrollSnapInfo::SnapTarget(
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(0 * AppUnitsPerCSSPixel()),
-      Nothing(), Some(100 * AppUnitsPerCSSPixel()), nsRect()));
+                                 CSSRect::ToAppUnits(CSSRect(0, 0, 10, 10))));
+  snap.mSnapTargets.AppendElement(
+      ScrollSnapInfo::SnapTarget(Nothing(), Some(100 * AppUnitsPerCSSPixel()),
+                                 CSSRect::ToAppUnits(CSSRect(0, 100, 10, 10))));
 
   ModifyFrameMetrics(root, [&](ScrollMetadata& aSm, FrameMetrics&) {
     aSm.SetSnapInfo(ScrollSnapInfo(snap));

layout/generic/ScrollSnap.cpp

@@ -39,6 +39,7 @@ class CalcSnapPoints final {
     return std::abs(
         NSCoordSaturatingSubtract(mSecondBestEdge.y, mBestEdge.y, nscoord_MAX));
   }
+  const nsPoint& Destination() const { return mDestination; }
 
  protected:
   ScrollUnit mUnit;
@@ -207,6 +208,18 @@ void CalcSnapPoints::AddEdge(nscoord aEdge, nscoord aDestination,
 static void ProcessSnapPositions(CalcSnapPoints& aCalcSnapPoints,
                                  const ScrollSnapInfo& aSnapInfo) {
   for (const auto& target : aSnapInfo.mSnapTargets) {
+    nsPoint snapPoint(target.mSnapPositionX ? *target.mSnapPositionX
+                                            : aCalcSnapPoints.Destination().x,
+                      target.mSnapPositionY ? *target.mSnapPositionY
+                                            : aCalcSnapPoints.Destination().y);
+    nsRect snappedPort = nsRect(snapPoint, aSnapInfo.mSnapportSize);
+    // Ignore snap points if snapping to the point would leave the snap area
+    // outside of the snapport.
+    // https://drafts.csswg.org/css-scroll-snap-1/#snap-scope
+    if (!snappedPort.Intersects(target.mSnapArea)) {
+      continue;
+    }
+
     if (target.mSnapPositionX) {
       aCalcSnapPoints.AddVerticalEdge(*target.mSnapPositionX);
     }

layout/generic/nsGfxScrollFrame.cpp

@@ -7627,7 +7627,6 @@ static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
                                          const nsIFrame* aScrolledFrame,
                                          const nsRect& aScrolledRect,
                                          const nsMargin& aScrollPadding,
-                                         const Maybe<nsRect>& aSnapport,
                                          WritingMode aWritingModeOnScroller,
                                          ScrollSnapInfo& aSnapInfo) {
   nsRect targetRect = nsLayoutUtils::TransformFrameRectToAncestor(
@@ -7639,13 +7638,6 @@ static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
   nsRect snapArea =
       InflateByScrollMargin(targetRect, scrollMargin, aScrolledRect);
 
-  // Ignore elements outside of the snapport when we scroll to the given
-  // destination.
-  // https://drafts.csswg.org/css-scroll-snap-1/#snap-scope
-  if (aSnapport && !aSnapport->Intersects(snapArea)) {
-    return;
-  }
-
   // These snap range shouldn't be involved with scroll-margin since we just
   // need the visible range of the target element.
   if (snapArea.width > aSnapInfo.mSnapportSize.width) {
@@ -7764,13 +7756,13 @@ static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
 /**
  * Collect the scroll positions corresponding to snap positions of frames in the
  * subtree rooted at |aFrame|, relative to |aScrolledFrame|, into |aSnapInfo|.
- * If |aSnapport| is given, elements outside of the range of |aSnapport| will be
- * ignored.
  */
-static void CollectScrollPositionsForSnap(
+static void CollectScrollPositionsForSnap(nsIFrame* aFrame,
-    nsIFrame* aFrame, nsIFrame* aScrolledFrame, const nsRect& aScrolledRect,
+                                          nsIFrame* aScrolledFrame,
-    const nsMargin& aScrollPadding, const Maybe<nsRect>& aSnapport,
+                                          const nsRect& aScrolledRect,
-    WritingMode aWritingModeOnScroller, ScrollSnapInfo& aSnapInfo) {
+                                          const nsMargin& aScrollPadding,
+                                          WritingMode aWritingModeOnScroller,
+                                          ScrollSnapInfo& aSnapInfo) {
   // Snap positions only affect the nearest ancestor scroll container on the
   // element's containing block chain.
   nsIScrollableFrame* sf = do_QueryFrame(aFrame);
@@ -7786,12 +7778,12 @@ static void CollectScrollPositionsForSnap(
           styleDisplay->mScrollSnapAlign.block !=
               StyleScrollSnapAlignKeyword::None) {
         AppendScrollPositionsForSnap(f, aScrolledFrame, aScrolledRect,
-                                     aScrollPadding, aSnapport,
+                                     aScrollPadding, aWritingModeOnScroller,
-                                     aWritingModeOnScroller, aSnapInfo);
+                                     aSnapInfo);
       }
       CollectScrollPositionsForSnap(f, aScrolledFrame, aScrolledRect,
-                                    aScrollPadding, aSnapport,
+                                    aScrollPadding, aWritingModeOnScroller,
-                                    aWritingModeOnScroller, aSnapInfo);
+                                    aSnapInfo);
     }
   }
 }
@@ -7867,20 +7859,12 @@ layers::ScrollSnapInfo ScrollFrameHelper::ComputeScrollSnapInfo(
 
   nsRect snapport = GetScrollPortRect();
   nsMargin scrollPadding = GetScrollPadding();
-
+  snapport.Deflate(scrollPadding);
-  Maybe<nsRect> snapportOnDestination;
-  if (aDestination) {
-    snapport.MoveTo(aDestination.value());
-    snapport.Deflate(scrollPadding);
-    snapportOnDestination.emplace(snapport);
-  } else {
-    snapport.Deflate(scrollPadding);
-  }
 
   result.mSnapportSize = snapport.Size();
   CollectScrollPositionsForSnap(mScrolledFrame, mScrolledFrame,
-                                GetScrolledRect(), scrollPadding,
+                                GetScrolledRect(), scrollPadding, writingMode,
-                                snapportOnDestination, writingMode, result);
+                                result);
   return result;
 }
 

testing/web-platform/meta/css/css-scroll-snap/input/keyboard.html.ini

@@ -1,27 +1,11 @@
 [keyboard.html]
   expected: TIMEOUT
   [If there is no valid snap offset on the arrow key's direction other than the current offset, and the scroll-snap-type is proximity, go to the original intended offset]
-    expected: NOTRUN
+    expected: [TIMEOUT, NOTRUN, FAIL]
-
-  [If the original intended offset is valid as making a snap area cover the snapport, but there's a defined snap offset in between, use the defined snap offset.]
-    expected: NOTRUN
-
-  [Snaps to top-left after pressing ArrowUp]
-    expected: TIMEOUT
-
-  [Snaps to top-left after pressing ArrowLeft]
-    expected: NOTRUN
 
   [Snaps to top-right after pressing ArrowRight]
     expected:
       if os == "mac": [NOTRUN, PASS, FAIL]
-      NOTRUN
-
-  [If the original intended offset is valid as making a snap area cover thesnapport, and there's no other snap offset in between, use the originalintended offset]
-    expected: NOTRUN
-
-  [If there is no valid snap offset on the arrow key's direction other than the current offset, and the scroll-snap-type is mandatory, stay at the current offset.]
-    expected: NOTRUN
 
   [Snaps to bottom-left after pressing ArrowDown]
     expected:
@@ -30,4 +14,9 @@
       if (os == "android") and swgl: PASS
       if (os == "android") and not swgl: FAIL
       if (os == "win") and swgl: PASS
-      [PASS, FAIL]
+
+  [If the original intended offset is valid as making a snap area cover the snapport, but there's a defined snap offset in between, use the defined snap offset.]
+    expected: [TIMEOUT, PASS]
+
+  [If there is no valid snap offset on the arrow key's direction other than the current offset, and the scroll-snap-type is mandatory, stay at the current offset.]
+    expected: [NOTRUN, PASS]

testing/web-platform/meta/css/css-scroll-snap/scroll-snap-stop.html.ini

@@ -7,7 +7,3 @@
 
   [A scroll with intended direction and end position should not pass a snap area with scroll-snap-stop: always.]
     expected: FAIL
-
-  [A scroll with intended end position should always choose the closest snap position regardless of the scroll-snap-stop value.]
-    expected: FAIL
-

testing/web-platform/meta/css/css-scroll-snap/scrollTo-scrollBy-snaps.html.ini

@@ -1,25 +0,0 @@
-[scrollTo-scrollBy-snaps.html]
-  [assign scrollLeft and scrollTop for {left: 10000, top: -100} on div lands on (1000, 0)]
-    expected: FAIL
-
-  [assign scrollLeft and scrollTop for {left: 10000, top: -100} on viewport-defining element lands on (1000, 0)]
-    expected: FAIL
-
-  [scrollBy({left: 10000, top: -100}) on div lands on (1000, 0)]
-    expected: FAIL
-
-  [scrollTo({left: 10000, top: -100}) on div lands on (1000, 0)]
-    expected: FAIL
-
-  [scrollTo({left: 10000, top: -100}) on viewport-defining element lands on (1000, 0)]
-    expected: FAIL
-
-  [scrollTo({left: 10000, top: -100}) on window lands on (1000, 0)]
-    expected: FAIL
-
-  [scrollBy({left: 10000, top: -100}) on window lands on (1000, 0)]
-    expected: FAIL
-
-  [scrollBy({left: 10000, top: -100}) on viewport-defining element lands on (1000, 0)]
-    expected: FAIL
-

testing/web-platform/meta/css/css-scroll-snap/snap-to-visible-areas-x-axis.html.ini

@@ -0,0 +1,4 @@
+[snap-to-visible-areas-x-axis.html]
+  [Only snap to visible area on X axis, even when the non-visible ones are closer]
+    expected: FAIL
+    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1768393

testing/web-platform/meta/css/css-scroll-snap/snap-to-visible-areas-y-axis.html.ini

@@ -0,0 +1,4 @@
+[snap-to-visible-areas-y-axis.html]
+  [Only snap to visible area on Y axis, even when the non-visible ones are closer]
+    expected: FAIL
+    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1768393