Bug 1551806 - Don't try to snap if there is no valid snap positions for the scroll-snap v1 implementation. r=botond
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Fri, 17 May 2019 20:50:24 +0000
changeset 533278 ef3c6d8bb498bfdae4c53cc9eec12f70bb9284ed
parent 533277 a6e9e91432d4c224cef1ea0b8c232ccd4c57dd13
child 533279 14743da36853e6b1d745418e128030b89bbb6fe2
push id11276
push userrgurzau@mozilla.com
push dateMon, 20 May 2019 13:11:24 +0000
treeherdermozilla-beta@847755a7c325 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1551806
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 1551806 - Don't try to snap if there is no valid snap positions for the scroll-snap v1 implementation. r=botond From the spec [1]; If a valid snap position exists then the scroll container must snap at the termination of a scroll (if none exist then no snapping occurs). Both of test cases in this commit fail without this change. [1] https://drafts.csswg.org/css-scroll-snap-1/#valdef-scroll-snap-type-mandatory Differential Revision: https://phabricator.services.mozilla.com/D31409
gfx/layers/FrameMetrics.h
gfx/layers/apz/test/mochitest/helper_scroll_snap_no_valid_snap_position.html
gfx/layers/apz/test/mochitest/mochitest.ini
gfx/layers/apz/test/mochitest/test_group_scroll_snap.html
layout/generic/ScrollSnap.cpp
testing/web-platform/meta/css/css-scroll-snap/scroll-snap-type-on-root-element.html.ini
testing/web-platform/tests/css/css-scroll-snap/no-snap-position.html
--- a/gfx/layers/FrameMetrics.h
+++ b/gfx/layers/FrameMetrics.h
@@ -732,16 +732,23 @@ struct ScrollSnapInfo {
            mSnapportSize == aOther.mSnapportSize;
   }
 
   bool HasScrollSnapping() const {
     return mScrollSnapTypeY != mozilla::StyleScrollSnapStrictness::None ||
            mScrollSnapTypeX != mozilla::StyleScrollSnapStrictness::None;
   }
 
+  bool HasSnapPositions() const {
+    return (!mSnapPositionX.IsEmpty() &&
+            mScrollSnapTypeX != mozilla::StyleScrollSnapStrictness::None) ||
+           (!mSnapPositionY.IsEmpty() &&
+            mScrollSnapTypeY != mozilla::StyleScrollSnapStrictness::None);
+  }
+
   void InitializeScrollSnapType(WritingMode aWritingMode,
                                 const nsStyleDisplay* aDisplay);
 
   // The scroll frame's scroll-snap-type.
   mozilla::StyleScrollSnapStrictness mScrollSnapTypeX =
       mozilla::StyleScrollSnapStrictness::None;
   mozilla::StyleScrollSnapStrictness mScrollSnapTypeY =
       mozilla::StyleScrollSnapStrictness::None;
new file mode 100644
--- /dev/null
+++ b/gfx/layers/apz/test/mochitest/helper_scroll_snap_no_valid_snap_position.html
@@ -0,0 +1,44 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <meta name="viewport" content="width=device-width, initial-scale=1">
+  <title>No snapping occurs if there is no valid snap position</title>
+  <script src="apz_test_utils.js"></script>
+  <script src="apz_test_native_event_utils.js"></script>
+  <script src="/tests/SimpleTest/paint_listener.js"></script>
+  <style>
+    div {
+      position: absolute;
+    }
+    #scroller {
+      width: 100%;
+      height: 500px;
+      overflow-y: scroll;
+      scroll-snap-type: y mandatory;
+    }
+    .child {
+      width: 100%;
+      height: 100px;
+      background-color: blue;
+    }
+  </style>
+</head>
+<body>
+  <div id="scroller">
+    <div class="child" style="top: 0px;"></div>
+    <div style="width: 100%; height: 2000px;"></div>
+    <div class="child" style="top: 1000px;"></div>
+  </div>
+  <script type="application/javascript">
+    function* test(testDriver) {
+      yield moveMouseAndScrollWheelOver(scroller, 100, 100, testDriver);
+
+      ok(scroller.scrollTop > 0, "Scroll should happen some amount");
+    }
+
+    waitUntilApzStable().then(runContinuation(test)).then(subtestDone);
+  </script>
+</body>
+</html>
+
--- a/gfx/layers/apz/test/mochitest/mochitest.ini
+++ b/gfx/layers/apz/test/mochitest/mochitest.ini
@@ -55,8 +55,10 @@
   skip-if = (os == 'android') # wheel events not supported on mobile
 [test_wheel_transactions.html]
   skip-if = (toolkit == 'android') # wheel events not supported on mobile
 [test_group_overrides.html]
   skip-if = (toolkit == 'android') # wheel events not supported on mobile
 [test_group_hittest.html]
   skip-if = (toolkit == 'android') # mouse events not supported on mobile
 [test_group_zoomToFocusedInput.html]
+[test_group_scroll_snap.html]
+  skip-if = (os == 'android') # wheel events not supported on mobile
new file mode 100644
--- /dev/null
+++ b/gfx/layers/apz/test/mochitest/test_group_scroll_snap.html
@@ -0,0 +1,37 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Various tests for scroll snap</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="application/javascript" src="apz_test_utils.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+  <script type="application/javascript">
+
+const prefs = [
+  ["general.smoothScroll", false],
+  // ensure that any mouse movement will trigger a new wheel transaction,
+  // because in this test we move the mouse a bunch and want to recalculate
+  // the target APZC after each such movement.
+  ["mousewheel.transaction.ignoremovedelay", 0],
+  ["mousewheel.transaction.timeout", 0],
+  ["layout.css.scroll-snap-v1.enabled", true],
+];
+
+const subtests = [
+  {"file": "helper_scroll_snap_no_valid_snap_position.html", "prefs": prefs},
+];
+
+if (isApzEnabled()) {
+  SimpleTest.waitForExplicitFinish();
+  window.onload = function() {
+    runSubtestsSeriallyInFreshWindows(subtests)
+    .then(SimpleTest.finish, SimpleTest.finish);
+  };
+}
+
+  </script>
+</head>
+<body>
+</body>
+</html>
--- a/layout/generic/ScrollSnap.cpp
+++ b/layout/generic/ScrollSnap.cpp
@@ -275,16 +275,21 @@ Maybe<nsPoint> ScrollSnapUtils::GetSnapP
     const ScrollSnapInfo& aSnapInfo, nsIScrollableFrame::ScrollUnit aUnit,
     const nsRect& aScrollRange, const nsPoint& aStartPos,
     const nsPoint& aDestination) {
   if (aSnapInfo.mScrollSnapTypeY == StyleScrollSnapStrictness::None &&
       aSnapInfo.mScrollSnapTypeX == StyleScrollSnapStrictness::None) {
     return Nothing();
   }
 
+  if (StaticPrefs::layout_css_scroll_snap_v1_enabled() &&
+      !aSnapInfo.HasSnapPositions()) {
+    return Nothing();
+  }
+
   CalcSnapPoints calcSnapPoints(aUnit, aDestination, aStartPos);
 
   if (StaticPrefs::layout_css_scroll_snap_v1_enabled()) {
     ProcessSnapPositions(calcSnapPoints, aSnapInfo);
 
     // If the distance between the first and the second candidate snap points
     // is larger than the snapport size and the snapport is covered by larger
     // elements, any points inside the covering area should be valid snap
--- a/testing/web-platform/meta/css/css-scroll-snap/scroll-snap-type-on-root-element.html.ini
+++ b/testing/web-platform/meta/css/css-scroll-snap/scroll-snap-type-on-root-element.html.ini
@@ -1,9 +1,9 @@
 [scroll-snap-type-on-root-element.html]
   [The writing-mode on the body is used]
-    expected:
-      if (os == "android") and e10s: FAIL
+    expected: FAIL
+    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1552089
 
   [The scroll-snap-type on the root element is applied]
     expected:
       if (os == "android") and e10s: FAIL
 
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-scroll-snap/no-snap-position.html
@@ -0,0 +1,60 @@
+<!DOCTYPE html>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap-1/#valdef-scroll-snap-type-mandatory" />
+<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;
+}
+.child {
+  width: 300px;
+  height: 300px;
+  background-color: blue;
+}
+</style>
+
+<div id="scroller">
+  <div class="child" style="top: 0px; left: 0px;"></div>
+  <div class="child" style="top: 1000px; left: 1000px;"></div>
+  <div style="width: 2000px; height: 2000px;"></div>
+</div>
+
+<script>
+test(() => {
+  scroller.scrollSnapType = "both mandatory";
+
+  // Scroll to where the first child is in view.
+  scroller.scrollTo(100, 100);
+  assert_equals(scroller.scrollLeft, 100);
+  assert_equals(scroller.scrollTop, 100);
+
+  // Scroll to where the second child is in view.
+  scroller.scrollTo(900, 900);
+  assert_equals(scroller.scrollLeft, 900);
+  assert_equals(scroller.scrollTop, 900);
+}, "No snapping occurs if there is no valid snap position");
+
+test(() => {
+  scroller.scrollSnapType = "x mandatory";
+
+  for (const target of document.querySelectorAll(".child")) {
+    target.scrollSnapAlign = "start none";
+  }
+
+  // Scroll to where the first child is in view.
+  scroller.scrollTo(100, 100);
+  assert_equals(scroller.scrollLeft, 100);
+  assert_equals(scroller.scrollTop, 100);
+
+  // Scroll to where the second child is in view.
+  scroller.scrollTo(900, 900);
+  assert_equals(scroller.scrollLeft, 900);
+  assert_equals(scroller.scrollTop, 900);
+}, "No snapping occurs if there is no valid snap position matches scroll-snap-type");
+</script>