Bug 1530253 - Trigger resnap on scroll-snap-type or scroll-snap-align changes. r=emilio

Depends on D148863

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

layout/generic/ScrollSnap.cpp

@@ -571,4 +571,12 @@ void ScrollSnapUtils::PostPendingResnapIfNeededFor(nsIFrame* aFrame) {
   }
 }
 
+void ScrollSnapUtils::PostPendingResnapFor(nsIFrame* aFrame) {
+  if (nsIScrollableFrame* sf = nsLayoutUtils::GetNearestScrollableFrame(
+          aFrame, nsLayoutUtils::SCROLLABLE_SAME_DOC |
+                      nsLayoutUtils::SCROLLABLE_INCLUDE_HIDDEN)) {
+    sf->PostPendingResnap();
+  }
+}
+
 }  // namespace mozilla

layout/generic/ScrollSnap.h

@@ -64,6 +64,13 @@ struct ScrollSnapUtils {
   // Post a pending re-snap request if the given |aFrame| is one of the snap
   // points on the last scroll operation.
   static void PostPendingResnapIfNeededFor(nsIFrame* aFrame);
+
+  // Similar to above PostPendingResnapIfNeededFor but post a pending re-snap
+  // request even if the given |aFrame| is not one of the last snap point.
+  // This is basically used for cases there was no valid snap point on the last
+  // scroll operation but the given |aFrame| might be a valid snap point now,
+  // e.g changing the scroll-snap-align property from `none` to something.
+  static void PostPendingResnapFor(nsIFrame* aFrame);
 };
 
 }  // namespace mozilla

layout/generic/nsGfxScrollFrame.cpp

@@ -5806,6 +5806,14 @@ void ScrollFrameHelper::Destroy(PostDestroyData& aPostDestroyData) {
   RemoveObservers();
 }
 
+void ScrollFrameHelper::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
+  if (aOldComputedStyle && !mIsRoot &&
+      mOuter->StyleDisplay()->mScrollSnapType !=
+          aOldComputedStyle->StyleDisplay()->mScrollSnapType) {
+    PostPendingResnap();
+  }
+}
+
 void ScrollFrameHelper::RemoveObservers() {
   if (mAsyncScroll) {
     mAsyncScroll->RemoveObserver();

layout/generic/nsGfxScrollFrame.h

@@ -107,6 +107,7 @@ class ScrollFrameHelper : public nsIReflowCallback {
   void PostOverflowEvent();
   using PostDestroyData = nsIFrame::PostDestroyData;
   void Destroy(PostDestroyData& aPostDestroyData);
+  void DidSetComputedStyle(ComputedStyle* aOldComputedStyle);
 
   void BuildDisplayList(nsDisplayListBuilder* aBuilder,
                         const nsDisplayListSet& aLists);
@@ -986,6 +987,10 @@ class nsHTMLScrollFrame : public nsContainerFrame,
   void RemoveFrame(ChildListID aListID, nsIFrame* aOldFrame) final;
 
   void DestroyFrom(nsIFrame* aDestructRoot, PostDestroyData&) override;
+  void DidSetComputedStyle(ComputedStyle* aOldComputedStyle) final {
+    nsContainerFrame::DidSetComputedStyle(aOldComputedStyle);
+    mHelper.DidSetComputedStyle(aOldComputedStyle);
+  }
 
   nsIScrollableFrame* GetScrollTargetFrame() const final {
     return const_cast<nsHTMLScrollFrame*>(this);
@@ -1430,6 +1435,10 @@ class nsXULScrollFrame final : public nsBoxFrame,
 
   void DestroyFrom(nsIFrame* aDestructRoot,
                    PostDestroyData& aPostDestroyData) final;
+  void DidSetComputedStyle(ComputedStyle* aOldComputedStyle) final {
+    nsBoxFrame::DidSetComputedStyle(aOldComputedStyle);
+    mHelper.DidSetComputedStyle(aOldComputedStyle);
+  };
 
   nsIScrollableFrame* GetScrollTargetFrame() const final {
     return const_cast<nsXULScrollFrame*>(this);

layout/generic/nsIFrame.cpp

@@ -104,6 +104,7 @@
 
 #include "gfxContext.h"
 #include "nsAbsoluteContainingBlock.h"
+#include "ScrollSnap.h"
 #include "StickyScrollContainer.h"
 #include "nsFontInflationData.h"
 #include "nsRegion.h"
@@ -1294,6 +1295,16 @@ void nsIFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
       handleStickyChange = disp->mPosition == StylePositionProperty::Sticky ||
                            oldDisp->mPosition == StylePositionProperty::Sticky;
     }
+    if (disp->mScrollSnapAlign != oldDisp->mScrollSnapAlign) {
+      ScrollSnapUtils::PostPendingResnapFor(this);
+    }
+    if (aOldComputedStyle->IsRootElementStyle() &&
+        disp->mScrollSnapType != oldDisp->mScrollSnapType) {
+      if (nsIScrollableFrame* scrollableFrame =
+              PresShell()->GetRootScrollFrameAsScrollable()) {
+        scrollableFrame->PostPendingResnap();
+      }
+    }
   } else {  // !aOldComputedStyle
     handleStickyChange = disp->mPosition == StylePositionProperty::Sticky;
   }

layout/style/nsStyleStruct.cpp

@@ -2432,13 +2432,9 @@ nsChangeHint nsStyleDisplay::CalcDifference(
     hint |= nsChangeHint_NeedReflow | nsChangeHint_ReflowChangesSizeOrPosition;
   }
 
-  if (mScrollSnapAlign != aNewData.mScrollSnapAlign) {
-    // FIXME: Bug 1530253 Support re-snapping when scroll-snap-align changes.
-    hint |= nsChangeHint_NeutralChange;
-  }
-  if (mScrollSnapType != aNewData.mScrollSnapType ||
+  if (mScrollSnapAlign != aNewData.mScrollSnapAlign ||
+      mScrollSnapType != aNewData.mScrollSnapType ||
       mScrollSnapStop != aNewData.mScrollSnapStop) {
-    // FIXME: Bug 1530253 Support re-snapping when scroll-snap-type changes.
     hint |= nsChangeHint_RepaintFrame;
   }
   if (mScrollBehavior != aNewData.mScrollBehavior) {

testing/web-platform/meta/css/css-scroll-snap/snap-after-relayout/changing-scroll-snap-align.html.ini

@@ -1,13 +0,0 @@
-[changing-scroll-snap-align.html]
-  [Removing the current target's snap alignment should make the scroller resnap to a new snap area.]
-    expected: FAIL
-
-  [Changing an element snap alignment from none to start by adding a class should make the scroller resnap.]
-    expected: FAIL
-
-  [Changing the current target's snap alignment should make the scroller resnap to it even if another snap position is closer to the current offset]
-    expected: FAIL
-
-  [Changing an element snap alignment from none to start should make thescroller resnap.]
-    expected: FAIL
-

testing/web-platform/meta/css/css-scroll-snap/snap-after-relayout/changing-scroll-snap-type.html.ini

@@ -1,10 +0,0 @@
-[changing-scroll-snap-type.html]
-  [Changing the scroller's snap type to y should make it resnap on the y-axis.]
-    expected: FAIL
-
-  [Changing the scroller's snap type axis should make it resnap.]
-    expected: FAIL
-
-  [Changing the scroller's snap type to x should make it resnap on the x-axis.]
-    expected: FAIL
-

testing/web-platform/tests/css/css-scroll-snap/snap-after-relayout/changing-scroll-snap-type-on-root-element.html

@@ -0,0 +1,95 @@
+<!DOCTYPE html>
+<html>
+<title>
+  Updating the scroll-snap-type of the root element should make it resnap accordingly.
+  This is another vairant of changing-scroll-snap-type.html for the root element.
+</title>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap/#re-snap" />
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+div {
+  position: absolute;
+  margin: 0;
+}
+
+html {
+  overflow: hidden;
+  scroll-snap-type: none;
+}
+
+#y-target {
+  width: 300px;
+  height: 300px;
+  top: 100px;
+  left: 0;
+  background-color: green;
+  scroll-snap-align: start none;
+}
+
+#x-target {
+  width: 300px;
+  height: 300px;
+  top: 0;
+  left: 100px;
+  background-color: red;
+  scroll-snap-align: none start;
+}
+
+.area {
+  width: 1000vw;
+  height: 1000vh;
+}
+</style>
+
+<div class="area"></div>
+<div id="x-target"></div>
+<div id="y-target"></div>
+
+<script>
+const x_target = document.getElementById("x_target");
+const y_target = document.getElementById("y_target");
+const scroller = document.documentElement;
+
+function cleanup() {
+  scroller.style.setProperty("scroll-snap-type", "none");
+}
+
+test(t => {
+  t.add_cleanup(cleanup);
+  scroller.scrollTo(0,0);
+  assert_equals(scroller.scrollTop, 0);
+  assert_equals(scroller.scrollLeft, 0);
+
+  scroller.style.setProperty("scroll-snap-type", "y mandatory");
+  assert_equals(scroller.scrollTop, 100);
+  assert_equals(scroller.scrollLeft, 0);
+}, "Changing the scroller's snap type to y should make it resnap on the y-axis.");
+
+test(t => {
+  t.add_cleanup(cleanup);
+  scroller.scrollTo(0,0);
+  assert_equals(scroller.scrollTop, 0);
+  assert_equals(scroller.scrollLeft, 0);
+
+  scroller.style.setProperty("scroll-snap-type", "x mandatory");
+  assert_equals(scroller.scrollLeft, 100);
+  assert_equals(scroller.scrollTop, 0);
+}, "Changing the scroller's snap type to x should make it resnap on the x-axis.");
+
+
+test(t => {
+  t.add_cleanup(cleanup);
+  scroller.scrollTo(0,0);
+  assert_equals(scroller.scrollTop, 0);
+  assert_equals(scroller.scrollLeft, 0);
+
+  scroller.style.setProperty("scroll-snap-type", "x mandatory");
+  assert_equals(scroller.scrollLeft, 100);
+  assert_equals(scroller.scrollTop, 0);
+
+  scroller.style.setProperty("scroll-snap-type", "y mandatory");
+  assert_equals(scroller.scrollTop, 100);
+}, "Changing the scroller's snap type axis should make it resnap.");
+</script>
+</html>