Bug 1546038 - Include scroll-margin areas into snap area. r=botond
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Tue, 23 Apr 2019 03:58:57 +0000
changeset 470436 6f0320a8555580db1b9c8cc7e2c7452b30bc157b
parent 470435 3e3a2ecafdaafb3ef75009b7662be7e84451d70b
child 470437 43003251156190b170a66b2c213d94d7d02cbf10
push id35906
push useraciure@mozilla.com
push dateTue, 23 Apr 2019 22:14:56 +0000
treeherdermozilla-central@0ce3633f8b80 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1546038
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 1546038 - Include scroll-margin areas into snap area. r=botond This patch also renames `targetRect` to `snapArea` to represent it more accurately. Differential Revision: https://phabricator.services.mozilla.com/D28307
gfx/layers/FrameMetrics.h
layout/generic/nsGfxScrollFrame.cpp
testing/web-platform/tests/css/css-scroll-snap/snap-to-visible-areas.html
--- a/gfx/layers/FrameMetrics.h
+++ b/gfx/layers/FrameMetrics.h
@@ -782,17 +782,17 @@ struct ScrollSnapInfo {
   };
   // An array of the range that the target element is larger than the snapport
   // on the axis.
   // Snap positions in this range will be valid snap positions in the case where
   // the distance between the closest snap position and the second closest snap
   // position is still larger than the snapport size.
   // See https://drafts.csswg.org/css-scroll-snap-1/#snap-overflow
   //
-  // Note: This range doesn't contain scroll-margin values.
+  // Note: This range contains scroll-margin values.
   nsTArray<ScrollSnapRange> mXRangeWiderThanSnapport;
   nsTArray<ScrollSnapRange> mYRangeWiderThanSnapport;
 
   // Note: This snapport size has been already deflated by scroll-padding.
   nsSize mSnapportSize;
 };
 
 // clang-format off
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -6558,45 +6558,49 @@ static void AppendScrollPositionsForSnap
                                          const nsIFrame* aScrolledFrame,
                                          const nsRect& aScrolledRect,
                                          const nsMargin& aScrollPadding,
                                          const Maybe<nsRect>& aSnapport,
                                          WritingMode aWritingModeOnScroller,
                                          ScrollSnapInfo& aSnapInfo) {
   nsRect targetRect = nsLayoutUtils::TransformFrameRectToAncestor(
       aFrame, aFrame->GetRectRelativeToSelf(), aScrolledFrame);
+
+  // The snap area contains scroll-margin values.
+  // https://drafts.csswg.org/css-scroll-snap-1/#scroll-snap-area
+  nsMargin scrollMargin = aFrame->StyleMargin()->GetScrollMargin();
+  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(targetRect)) {
+  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 (targetRect.width > aSnapInfo.mSnapportSize.width) {
+  if (snapArea.width > aSnapInfo.mSnapportSize.width) {
     aSnapInfo.mXRangeWiderThanSnapport.AppendElement(
-        ScrollSnapInfo::ScrollSnapRange(targetRect.X(), targetRect.XMost()));
-  }
-  if (targetRect.height > aSnapInfo.mSnapportSize.height) {
+        ScrollSnapInfo::ScrollSnapRange(snapArea.X(), snapArea.XMost()));
+  }
+  if (snapArea.height > aSnapInfo.mSnapportSize.height) {
     aSnapInfo.mYRangeWiderThanSnapport.AppendElement(
-        ScrollSnapInfo::ScrollSnapRange(targetRect.Y(), targetRect.YMost()));
-  }
-
-  nsMargin scrollMargin = aFrame->StyleMargin()->GetScrollMargin();
-  targetRect = InflateByScrollMargin(targetRect, scrollMargin, aScrolledRect);
+        ScrollSnapInfo::ScrollSnapRange(snapArea.Y(), snapArea.YMost()));
+  }
 
   // Shift target rect position by the scroll padding to get the padded
   // position thus we don't need to take account scroll-padding values in
   // ScrollSnapUtils::GetSnapPointForDestination() when it gets called from
   // the compositor thread.
-  targetRect.y -= aScrollPadding.top;
-  targetRect.x -= aScrollPadding.left;
-
-  LogicalRect logicalTargetRect(aWritingModeOnScroller, targetRect,
+  snapArea.y -= aScrollPadding.top;
+  snapArea.x -= aScrollPadding.left;
+
+  LogicalRect logicalTargetRect(aWritingModeOnScroller, snapArea,
                                 aSnapInfo.mSnapportSize);
   LogicalSize logicalSnapportRect(aWritingModeOnScroller,
                                   aSnapInfo.mSnapportSize);
 
   Maybe<nscoord> blockDirectionPosition;
   Maybe<nscoord> inlineDirectionPosition;
 
   const nsStyleDisplay* styleDisplay = aFrame->StyleDisplay();
--- a/testing/web-platform/tests/css/css-scroll-snap/snap-to-visible-areas.html
+++ b/testing/web-platform/tests/css/css-scroll-snap/snap-to-visible-areas.html
@@ -35,22 +35,30 @@ div {
   top: 0px;
 }
 
 #left-bottom {
   left: 0px;
   top: 800px;
 }
 
+#right-bottom {
+  left: 1800px;
+  top: 1800px;
+  scroll-margin-top: 1000px;
+  scroll-margin-left: 1000px;
+}
+
 </style>
 <div id="scroller">
   <div id="space"></div>
   <div id="left-top" class="snap"></div>
   <div id="right-top" class="snap"></div>
   <div id="left-bottom" class="snap"></div>
+  <div id="right-bottom" class="snap"></div>
 </div>
 <script>
 var scroller = document.getElementById("scroller");
 test(() => {
   scroller.scrollTo(0, 0);
   assert_equals(scroller.scrollLeft, 0);
   assert_equals(scroller.scrollTop, 0);
   scroller.scrollTo(300, 0);
@@ -61,9 +69,18 @@ test(() => {
 test(() => {
   scroller.scrollTo(0, 0);
   assert_equals(scroller.scrollLeft, 0);
   assert_equals(scroller.scrollTop, 0);
   scroller.scrollTo(0, 300);
   assert_equals(scroller.scrollLeft, 0);
   assert_equals(scroller.scrollTop, 800);
 }, 'Only snap to visible area on Y axis, even when the non-visible ones are closer');
+
+test(() => {
+  scroller.scrollTo(0, 0);
+  assert_equals(scroller.scrollLeft, 0);
+  assert_equals(scroller.scrollTop, 0);
+  scroller.scrollTo(300, 300);
+  assert_equals(scroller.scrollLeft, 800);
+  assert_equals(scroller.scrollTop, 800);
+}, 'snap to snap area inflated by scroll-margin, even when the non-visible ones are closer');
 </script>