Bug 1373833 - Factor scroll-margin values into scroll snap position calculation. r=botond

scroll-margin is for each elements in the scroll container and snap positions
are shifted by the value.

https://drafts.csswg.org/css-scroll-snap-1/#scroll-margin
https://drafts.csswg.org/css-scroll-snap-1/#scroll-snap-area

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

--HG--
extra : moz-landing-system : lando

layout/generic/nsGfxScrollFrame.cpp

@@ -6519,15 +6519,31 @@ uint32_t nsIScrollableFrame::GetPerceivedScrollingDirections() const {
   return directions;
 }
 
+static nsRect InflateByScrollMargin(const nsRect& aTargetRect,
+                                    const StyleRect<StyleLength>& aScrollMargin,
+                                    const nsRect& aScrolledRect) {
+  nsMargin scrollMargin(aScrollMargin.Get(eSideTop).ToAppUnits(),
+                        aScrollMargin.Get(eSideRight).ToAppUnits(),
+                        aScrollMargin.Get(eSideBottom).ToAppUnits(),
+                        aScrollMargin.Get(eSideLeft).ToAppUnits());
+
+  // Inflate the rect by scroll-margin.
+  nsRect result = aTargetRect;
+  result.Inflate(scrollMargin);
+
+  // But don't be beyond the limit boundary.
+  return result.Intersect(aScrolledRect);
+}
+
 /**
  * Append scroll positions for valid snap positions into |aSnapInfo| if
  * applicable.
  */
 static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
                                          const nsIFrame* aScrolledFrame,
+                                         const nsRect& aScrolledRect,
                                          const Maybe<nsRect>& aSnapport,
                                          ScrollSnapInfo& aSnapInfo) {
-  // FIXME: Bug 1373833: This target rect should be inflated by scroll-margin.
   nsRect targetRect = nsLayoutUtils::TransformFrameRectToAncestor(
       aFrame, aFrame->GetRectRelativeToSelf(), aScrolledFrame);
   // Ignore elements outside of the snapport when we scroll to the given
@@ -6537,6 +6553,20 @@ static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
     return;
   }
 
+  // These snap range shouldn't be involved with scroll-margin since we just
+  // need the visible range of the target element.
+  if (targetRect.width > aSnapInfo.mSnapportSize.width) {
+    aSnapInfo.mXRangeWiderThanSnapport.AppendElement(
+        ScrollSnapInfo::ScrollSnapRange(targetRect.X(), targetRect.XMost()));
+  }
+  if (targetRect.height > aSnapInfo.mSnapportSize.height) {
+    aSnapInfo.mYRangeWiderThanSnapport.AppendElement(
+        ScrollSnapInfo::ScrollSnapRange(targetRect.Y(), targetRect.YMost()));
+  }
+
+  targetRect = InflateByScrollMargin(
+      targetRect, aFrame->StyleMargin()->mScrollMargin, aScrolledRect);
+
   WritingMode writingMode = aScrolledFrame->GetWritingMode();
   LogicalRect logicalTargetRect(writingMode, targetRect,
                                 aSnapInfo.mSnapportSize);
@@ -6627,15 +6657,6 @@ static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
                               : aSnapInfo.mSnapPositionY)
         .AppendElement(blockDirectionPosition.value());
   }
-
-  if (targetRect.width > aSnapInfo.mSnapportSize.width) {
-    aSnapInfo.mXRangeWiderThanSnapport.AppendElement(
-        ScrollSnapInfo::ScrollSnapRange(targetRect.X(), targetRect.XMost()));
-  }
-  if (targetRect.height > aSnapInfo.mSnapportSize.height) {
-    aSnapInfo.mYRangeWiderThanSnapport.AppendElement(
-        ScrollSnapInfo::ScrollSnapRange(targetRect.Y(), targetRect.YMost()));
-  }
 }
 
 /**
@@ -6646,6 +6667,7 @@ static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
  */
 static void CollectScrollPositionsForSnap(nsIFrame* aFrame,
                                           nsIFrame* aScrolledFrame,
+                                          const nsRect& aScrolledRect,
                                           const Maybe<nsRect>& aSnapport,
                                           ScrollSnapInfo& aSnapInfo) {
   MOZ_ASSERT(StaticPrefs::layout_css_scroll_snap_v1_enabled());
@@ -6661,9 +6683,11 @@ static void CollectScrollPositionsForSnap(nsIFrame* aFrame,
               StyleScrollSnapAlignKeyword::None ||
           styleDisplay->mScrollSnapAlign.block !=
               StyleScrollSnapAlignKeyword::None) {
-        AppendScrollPositionsForSnap(f, aScrolledFrame, aSnapport, aSnapInfo);
+        AppendScrollPositionsForSnap(f, aScrolledFrame, aScrolledRect,
+                                     aSnapport, aSnapInfo);
       }
-      CollectScrollPositionsForSnap(f, aScrolledFrame, aSnapport, aSnapInfo);
+      CollectScrollPositionsForSnap(f, aScrolledFrame, aScrolledRect, aSnapport,
+                                    aSnapInfo);
     }
   }
 }
@@ -6754,7 +6778,8 @@ layers::ScrollSnapInfo ScrollFrameHelper::ComputeScrollSnapInfo(
 
     result.mSnapportSize = snapportSize;
     CollectScrollPositionsForSnap(mScrolledFrame, mScrolledFrame,
-                                  snapportOnDestination, result);
+                                  GetScrolledRect(), snapportOnDestination,
+                                  result);
     return result;
   }
 

layout/style/nsStyleStruct.cpp

@@ -202,13 +202,25 @@ nsStyleMargin::nsStyleMargin(const nsStyleMargin& aSrc)
 
 nsChangeHint nsStyleMargin::CalcDifference(
     const nsStyleMargin& aNewData) const {
-  if (mMargin == aNewData.mMargin) {
+  if (mMargin == aNewData.mMargin && mScrollMargin == aNewData.mScrollMargin) {
     return nsChangeHint(0);
   }
-  // Margin differences can't affect descendant intrinsic sizes and
-  // don't need to force children to reflow.
-  return nsChangeHint_NeedReflow | nsChangeHint_ReflowChangesSizeOrPosition |
-         nsChangeHint_ClearAncestorIntrinsics;
+
+  nsChangeHint hint = nsChangeHint(0);
+
+  if (mMargin != aNewData.mMargin) {
+    // Margin differences can't affect descendant intrinsic sizes and
+    // don't need to force children to reflow.
+    hint |= nsChangeHint_NeedReflow | nsChangeHint_ReflowChangesSizeOrPosition |
+            nsChangeHint_ClearAncestorIntrinsics;
+  }
+
+  if (mScrollMargin != aNewData.mScrollMargin) {
+    // FIXME: Bug 1530253 Support re-snapping when scroll-margin changes.
+    hint |= nsChangeHint_NeutralChange;
+  }
+
+  return hint;
 }
 
 nsStylePadding::nsStylePadding(const Document& aDocument)

testing/web-platform/tests/css/css-scroll-snap/scroll-margin.html

@@ -0,0 +1,73 @@
+<!DOCTYPE html>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap-1/#scroll-margin" />
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+div {
+  position: absolute;
+  margin: 0px;
+}
+#scroller {
+  height: 500px;
+  width: 500px;
+  overflow: hidden;
+  scroll-snap-type: both mandatory;
+}
+#target {
+  width: 300px;
+  height: 300px;
+  background-color: blue;
+}
+#another-target {
+  width: 300px;
+  height: 300px;
+  top: 400px;
+  left: 400px;
+  background-color: blue;
+  scroll-snap-align: start;
+}
+</style>
+
+<div id="scroller">
+  <div style="width: 2000px; height: 2000px;"></div>
+  <div id="target"></div>
+  <div id="another-target"></div>
+</div>
+
+<script>
+test(() => {
+  target.style.scrollSnapAlign = "start";
+  target.style.scrollMargin = "100px";
+  target.style.left = "300px";
+  target.style.top = "300px";
+
+  scroller.scrollTo(0, 0);
+  // `target position (300px, 300px)` - `margin (100px, 100px)`.
+  assert_equals(scroller.scrollLeft, 200);
+  assert_equals(scroller.scrollTop, 200);
+
+  target.style.scrollSnapAlign = "end";
+
+  // `target position (300px, 300px)` + `target size (300px, 300px) +
+  // `margin (100px, 100px) - `scroller size (500px, 500px)`.
+  scroller.scrollTo(0, 0);
+  assert_equals(scroller.scrollLeft, 200);
+  assert_equals(scroller.scrollTop, 200);
+}, "Snaps to the positions adjusted by scroll-margin");
+
+test(() => {
+  target.style.left = "0px";
+  target.style.top = "0px";
+
+  target.style.scrollSnapAlign = "start";
+  target.style.scrollMargin = "100px";
+
+  // Scroll to the position between #target and #another-target elements but
+  // if the scroll-margin 100px contributed to the snap start-aligned snap
+  // position it will be farther than #another-target.
+  scroller.scrollTo(200, 200);
+  assert_equals(scroller.scrollLeft, 0);
+  assert_equals(scroller.scrollTop, 0);
+}, "scroll-margin doesn't contribute to the snap position of the element " +
+   "if it's outside of the scroll port");
+</script>