Bug 1373832 - Deflate the snapport by scroll-padding and adjust the snap positions by the padding value. r=botond
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Thu, 11 Apr 2019 06:21:48 +0000
changeset 468967 b500d17a3bf5523d70fe27d246a59ec7074cf35b
parent 468966 acbcbc1dc6e0d261a41620e0ee5ad0e73edc15ac
child 468968 2b8493e37c626b53fe8a3c0852573943412af809
push id82883
push userhikezoe@mozilla.com
push dateThu, 11 Apr 2019 07:40:43 +0000
treeherderautoland@b500d17a3bf5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1373832
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 1373832 - Deflate the snapport by scroll-padding and adjust the snap positions by the padding value. r=botond https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding Differential Revision: https://phabricator.services.mozilla.com/D21635
gfx/layers/FrameMetrics.h
layout/generic/nsGfxScrollFrame.cpp
layout/style/nsStyleStruct.cpp
testing/web-platform/tests/css/css-scroll-snap/scroll-padding.html
--- a/gfx/layers/FrameMetrics.h
+++ b/gfx/layers/FrameMetrics.h
@@ -777,16 +777,18 @@ struct ScrollSnapInfo {
   // 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.
   nsTArray<ScrollSnapRange> mXRangeWiderThanSnapport;
   nsTArray<ScrollSnapRange> mYRangeWiderThanSnapport;
+
+  // Note: This snapport size has been already deflated by scroll-padding.
   nsSize mSnapportSize;
 };
 
 // clang-format off
 MOZ_DEFINE_ENUM_CLASS_WITH_BASE(
   OverscrollBehavior, uint8_t, (
     Auto,
     Contain,
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -6537,16 +6537,17 @@ static nsRect InflateByScrollMargin(cons
 
 /**
  * Append scroll positions for valid snap positions into |aSnapInfo| if
  * applicable.
  */
 static void AppendScrollPositionsForSnap(const nsIFrame* aFrame,
                                          const nsIFrame* aScrolledFrame,
                                          const nsRect& aScrolledRect,
+                                         const nsMargin& aScrollPadding,
                                          const Maybe<nsRect>& aSnapport,
                                          ScrollSnapInfo& aSnapInfo) {
   nsRect targetRect = nsLayoutUtils::TransformFrameRectToAncestor(
       aFrame, aFrame->GetRectRelativeToSelf(), aScrolledFrame);
   // 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)) {
@@ -6562,16 +6563,23 @@ static void AppendScrollPositionsForSnap
   if (targetRect.height > aSnapInfo.mSnapportSize.height) {
     aSnapInfo.mYRangeWiderThanSnapport.AppendElement(
         ScrollSnapInfo::ScrollSnapRange(targetRect.Y(), targetRect.YMost()));
   }
 
   targetRect = InflateByScrollMargin(
       targetRect, aFrame->StyleMargin()->mScrollMargin, aScrolledRect);
 
+  // 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;
+
   WritingMode writingMode = aScrolledFrame->GetWritingMode();
   LogicalRect logicalTargetRect(writingMode, targetRect,
                                 aSnapInfo.mSnapportSize);
   LogicalSize logicalSnapportRect(writingMode, aSnapInfo.mSnapportSize);
 
   Maybe<nscoord> blockDirectionPosition;
   Maybe<nscoord> inlineDirectionPosition;
 
@@ -6663,36 +6671,37 @@ static void AppendScrollPositionsForSnap
  * 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(nsIFrame* aFrame,
                                           nsIFrame* aScrolledFrame,
                                           const nsRect& aScrolledRect,
+                                          const nsMargin& aScrollPadding,
                                           const Maybe<nsRect>& aSnapport,
                                           ScrollSnapInfo& aSnapInfo) {
   MOZ_ASSERT(StaticPrefs::layout_css_scroll_snap_v1_enabled());
 
   nsIFrame::ChildListIterator childLists(aFrame);
   for (; !childLists.IsDone(); childLists.Next()) {
     nsFrameList::Enumerator childFrames(childLists.CurrentList());
     for (; !childFrames.AtEnd(); childFrames.Next()) {
       nsIFrame* f = childFrames.get();
 
       const nsStyleDisplay* styleDisplay = f->StyleDisplay();
       if (styleDisplay->mScrollSnapAlign.inline_ !=
               StyleScrollSnapAlignKeyword::None ||
           styleDisplay->mScrollSnapAlign.block !=
               StyleScrollSnapAlignKeyword::None) {
         AppendScrollPositionsForSnap(f, aScrolledFrame, aScrolledRect,
-                                     aSnapport, aSnapInfo);
+                                     aScrollPadding, aSnapport, aSnapInfo);
       }
-      CollectScrollPositionsForSnap(f, aScrolledFrame, aScrolledRect, aSnapport,
-                                    aSnapInfo);
+      CollectScrollPositionsForSnap(f, aScrolledFrame, aScrolledRect,
+                                    aScrollPadding, aSnapport, aSnapInfo);
     }
   }
 }
 
 /**
  * Collect the scroll-snap-coordinates of frames in the subtree rooted at
  * |aFrame|, relative to |aScrolledFrame|, into |aOutCoords|.
  */
@@ -6726,16 +6735,55 @@ static void CollectScrollSnapCoordinates
         }
       }
 
       CollectScrollSnapCoordinates(f, aScrolledFrame, aOutCoords);
     }
   }
 }
 
+static nscoord ResolveScrollPaddingStyleValue(
+    const StyleRect<mozilla::NonNegativeLengthPercentageOrAuto>&
+        aScrollPaddingStyle,
+    Side aSide, const nsSize& aScrollPortSize) {
+  if (aScrollPaddingStyle.Get(aSide).IsAuto()) {
+    // https://drafts.csswg.org/css-scroll-snap-1/#valdef-scroll-padding-auto
+    return 0;
+  }
+
+  nscoord percentageBasis;
+  switch (aSide) {
+    case eSideTop:
+    case eSideBottom:
+      percentageBasis = aScrollPortSize.height;
+      break;
+    case eSideLeft:
+    case eSideRight:
+      percentageBasis = aScrollPortSize.width;
+      break;
+  }
+
+  return aScrollPaddingStyle.Get(aSide).AsLengthPercentage().Resolve(
+      percentageBasis);
+}
+
+static nsMargin ResolveScrollPaddingStyle(
+    const StyleRect<mozilla::NonNegativeLengthPercentageOrAuto>&
+        aScrollPaddingStyle,
+    const nsSize& aScrollPortSize) {
+  return nsMargin(ResolveScrollPaddingStyleValue(aScrollPaddingStyle, eSideTop,
+                                                 aScrollPortSize),
+                  ResolveScrollPaddingStyleValue(aScrollPaddingStyle,
+                                                 eSideRight, aScrollPortSize),
+                  ResolveScrollPaddingStyleValue(aScrollPaddingStyle,
+                                                 eSideBottom, aScrollPortSize),
+                  ResolveScrollPaddingStyleValue(aScrollPaddingStyle, eSideLeft,
+                                                 aScrollPortSize));
+}
+
 layers::ScrollSnapInfo ScrollFrameHelper::ComputeScrollSnapInfo(
     const Maybe<nsPoint>& aDestination) const {
   ScrollSnapInfo result;
 
   ScrollStyles styles = GetScrollStylesFromFrame();
 
   if (styles.mScrollSnapTypeY == StyleScrollSnapStrictness::None &&
       styles.mScrollSnapTypeX == StyleScrollSnapStrictness::None) {
@@ -6759,32 +6807,40 @@ layers::ScrollSnapInfo ScrollFrameHelper
   }
   if (styles.mScrollSnapPointsY.GetUnit() != eStyleUnit_None) {
     result.mScrollSnapIntervalY =
         Some(styles.mScrollSnapPointsY.ComputeCoordPercentCalc(
             scrollPortSize.height));
   }
 
   if (StaticPrefs::layout_css_scroll_snap_v1_enabled()) {
-    // FIXME: Bug 1373832: The snapport should be deflated by scroll-padding.
-    nsSize snapportSize = GetScrollPortRect().Size();
+    // The spec says percentage values are relative to the scroll port size.
+    // https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding
+    nsRect snapport = GetScrollPortRect();
+    nsMargin scrollPadding = ResolveScrollPaddingStyle(
+        mOuter->StylePadding()->mScrollPadding, snapport.Size());
 
     Maybe<nsRect> snapportOnDestination;
     if (aDestination) {
-      snapportOnDestination.emplace(
-          IsPhysicalLTR() ? nsRect(aDestination.value(), snapportSize)
-                          : nsRect(nsPoint(aDestination->x - snapportSize.width,
-                                           aDestination->y),
-                                   snapportSize));
-    }
-
-    result.mSnapportSize = snapportSize;
+      if (IsPhysicalLTR()) {
+        snapport.MoveTo(aDestination.value());
+      } else {
+        snapport.MoveTo(
+            nsPoint(aDestination->x - snapport.Size().width, aDestination->y));
+      }
+      snapport.Deflate(scrollPadding);
+      snapportOnDestination.emplace(snapport);
+    } else {
+      snapport.Deflate(scrollPadding);
+    }
+
+    result.mSnapportSize = snapport.Size();
     CollectScrollPositionsForSnap(mScrolledFrame, mScrolledFrame,
-                                  GetScrolledRect(), snapportOnDestination,
-                                  result);
+                                  GetScrolledRect(), scrollPadding,
+                                  snapportOnDestination, result);
     return result;
   }
 
   CollectScrollSnapCoordinates(mScrolledFrame, mScrolledFrame,
                                result.mScrollSnapCoordinates);
 
   return result;
 }
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -231,28 +231,41 @@ nsStylePadding::nsStylePadding(const Doc
 
 nsStylePadding::nsStylePadding(const nsStylePadding& aSrc)
     : mPadding(aSrc.mPadding), mScrollPadding(aSrc.mScrollPadding) {
   MOZ_COUNT_CTOR(nsStylePadding);
 }
 
 nsChangeHint nsStylePadding::CalcDifference(
     const nsStylePadding& aNewData) const {
-  if (mPadding == aNewData.mPadding) {
+  if (mPadding == aNewData.mPadding &&
+      mScrollPadding == aNewData.mScrollPadding) {
     return nsChangeHint(0);
   }
-  // Padding differences can't affect descendant intrinsic sizes, but do need
-  // to force children to reflow so that we can reposition them, since their
-  // offsets are from our frame bounds but our content rect's position within
-  // those bounds is moving.
-  // FIXME: It would be good to return a weaker hint here that doesn't
-  // force reflow of all descendants, but the hint would need to force
-  // reflow of the frame's children (see how
-  // ReflowInput::InitResizeFlags initializes the inline-resize flag).
-  return NS_STYLE_HINT_REFLOW & ~nsChangeHint_ClearDescendantIntrinsics;
+
+  nsChangeHint hint = nsChangeHint(0);
+
+  if (mPadding != aNewData.mPadding) {
+    // Padding differences can't affect descendant intrinsic sizes, but do need
+    // to force children to reflow so that we can reposition them, since their
+    // offsets are from our frame bounds but our content rect's position within
+    // those bounds is moving.
+    // FIXME: It would be good to return a weaker hint here that doesn't
+    // force reflow of all descendants, but the hint would need to force
+    // reflow of the frame's children (see how
+    // ReflowInput::InitResizeFlags initializes the inline-resize flag).
+    hint |= NS_STYLE_HINT_REFLOW & ~nsChangeHint_ClearDescendantIntrinsics;
+  }
+
+  if (mScrollPadding != aNewData.mScrollPadding) {
+    // FIXME: Bug 1530253 Support re-snapping when scroll-padding changes.
+    hint |= nsChangeHint_NeutralChange;
+  }
+
+  return hint;
 }
 
 static nscoord TwipsPerPixel(const Document& aDocument) {
   auto* pc = aDocument.GetPresContext();
   return pc ? pc->AppUnitsPerDevPixel() : mozilla::AppUnitsPerCSSPixel();
 }
 
 static inline BorderRadius ZeroBorderRadius() {
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-scroll-snap/scroll-padding.html
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding" />
+<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;
+}
+</style>
+
+<div id="scroller">
+  <div style="width: 2000px; height: 2000px;"></div>
+  <div id="target"></div>
+</div>
+
+<script>
+test(() => {
+  scroller.style.scrollPadding = "100px";
+
+  target.style.scrollSnapAlign = "start";
+  target.style.left = "300px";
+  target.style.top = "300px";
+
+  scroller.scrollTo(0, 0);
+  // `target position (300px, 300px)` - `padding (100px, 100px)`.
+  assert_equals(scroller.scrollLeft, 200);
+  assert_equals(scroller.scrollTop, 200);
+
+  target.style.scrollSnapAlign = "end";
+
+  // `target position (300px, 300px)` + `target size (300px, 300px) +
+  // `padding (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-padding");
+</script>