Bug 1552089 - Don't tweak snapport position even in the case of RTL scroll containers. r=botond
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Fri, 17 May 2019 20:36:57 +0000
changeset 474445 14743da36853e6b1d745418e128030b89bbb6fe2
parent 474444 ef3c6d8bb498bfdae4c53cc9eec12f70bb9284ed
child 474446 d7a7edbebd6a08f22d78b5c86b2f2d4573eb77dd
push id113154
push usernerli@mozilla.com
push dateSun, 19 May 2019 09:30:32 +0000
treeherdermozilla-inbound@d7a7edbebd6a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbotond
bugs1552089
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 1552089 - Don't tweak snapport position even in the case of RTL scroll containers. r=botond In RTL scroll containers, the right most x-axis scroll position is 0 and leftward scroll positions are negative values. And also nsLayoutUtils::TransformFrameRectToAncestor, which is used to tell whether the snap target element is inside the destination snapport or not [1], returns negative x-axis positions for elements in RTL scroll containers if the element is positioned at places where the elements are outside of the initial scroll position (0, 0). So we don't need to tweak snapport postion even in the case of RTL scroll containers. Instead, what we needed in the first place is that we choose a proper x-axis scroll position that the targe element appears inside the snapport. [1] https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/layout/generic/nsGfxScrollFrame.cpp#6604-6605,6616-6617 Depends on D31409 Differential Revision: https://phabricator.services.mozilla.com/D31410
layout/generic/nsGfxScrollFrame.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/snap-inline-block.html
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -6917,22 +6917,17 @@ layers::ScrollSnapInfo ScrollFrameHelper
   WritingMode writingMode = GetFrameForDir()->GetWritingMode();
   result.InitializeScrollSnapType(writingMode, disp);
 
   nsRect snapport = GetScrollPortRect();
   nsMargin scrollPadding = GetScrollPadding();
 
   Maybe<nsRect> snapportOnDestination;
   if (aDestination) {
-    if (IsPhysicalLTR()) {
-      snapport.MoveTo(aDestination.value());
-    } else {
-      snapport.MoveTo(
-          nsPoint(aDestination->x - snapport.Size().width, aDestination->y));
-    }
+    snapport.MoveTo(aDestination.value());
     snapport.Deflate(scrollPadding);
     snapportOnDestination.emplace(snapport);
   } else {
     snapport.Deflate(scrollPadding);
   }
 
   result.mSnapportSize = snapport.Size();
   CollectScrollPositionsForSnap(mScrolledFrame, mScrolledFrame,
--- 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: FAIL
-    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1552089
+    expected:
+      if (os == "android") and e10s: FAIL
 
   [The scroll-snap-type on the root element is applied]
     expected:
       if (os == "android") and e10s: FAIL
 
--- a/testing/web-platform/tests/css/css-scroll-snap/snap-inline-block.html
+++ b/testing/web-platform/tests/css/css-scroll-snap/snap-inline-block.html
@@ -39,18 +39,20 @@ const scroller_height = scroller.clientH
   ["vertical-rl",   scroller_width - 700, 300]
 ].forEach(([writing_mode, left, top]) => {
   test(() => {
     const target_left = getComputedStyle(target).left;
     scroller.style.writingMode = writing_mode;
     target.style.scrollSnapAlign = "end start";
     if (writing_mode == "vertical-rl") {
       target.style.left = (scroller_width - 700) + "px";
+      scroller.scrollTo(-500, 0);
+    } else {
+      scroller.scrollTo(0, 0);
     }
-    scroller.scrollTo(0, 0);
     assert_equals(scroller.scrollLeft, left, "aligns correctly on x");
     assert_equals(scroller.scrollTop, top, "aligns correctly on y");
     target.style.left = target_left;
     scroller.style.writingMode = "";
   }, "Snaps correctly for " + writing_mode +
      " writing mode with 'scroll-snap-align: end start' alignment");
 });
 
@@ -60,50 +62,52 @@ const scroller_height = scroller.clientH
   ["vertical-rl",   target.clientWidth - 700, 500 - scroller_height]
 ].forEach(([writing_mode, left, top]) => {
   test(() => {
     const target_left = getComputedStyle(target).left;
     scroller.style.writingMode = writing_mode;
     target.style.scrollSnapAlign = "start end";
     if (writing_mode == "vertical-rl") {
       target.style.left = (scroller_width - 700) + "px";
+      scroller.scrollTo(-500, 0);
+    } else {
+      scroller.scrollTo(0, 0);
     }
-    scroller.scrollTo(0, 0);
     assert_equals(scroller.scrollLeft, left, "aligns correctly on x");
     assert_equals(scroller.scrollTop, top, "aligns correctly on y");
     target.style.left = target_left;
     scroller.style.writingMode = "";
   }, "Snaps correctly for " + writing_mode +
      " writing mode with 'scroll-snap-align: start end' alignment");
 });
 
 test(() => {
   const target_left = getComputedStyle(target).left;
   scroller.style.direction = "rtl";
   target.style.scrollSnapAlign = "end start";
   target.style.left = (scroller_width - 700) + "px";
 
-  scroller.scrollTo(0, 0);
+  scroller.scrollTo(-500, 0);
   assert_equals(scroller.scrollLeft, target.clientWidth - 700,
                 "aligns correctly on x");
   assert_equals(scroller.scrollTop, 500 - scroller_height,
                 "aligns correctly on y");
 
   target.style.left = target_left;
   scroller.style.direction = "";
 }, "Snaps correctly for 'direction: rtl' with 'scroll-snap-align: end start' " +
    "alignment");
 
 test(() => {
   const target_left = getComputedStyle(target).left;
   scroller.style.direction = "rtl";
   target.style.scrollSnapAlign = "start end";
   target.style.left = (scroller_width - 700) + "px";
 
-  scroller.scrollTo(0, 0);
+  scroller.scrollTo(-500, 0);
   assert_equals(scroller.scrollLeft, scroller_width - 700,
                 "aligns correctly on x");
   assert_equals(scroller.scrollTop, 300, "aligns correctly on y");
 
   target.style.left = target_left;
   scroller.style.direction = "";
 }, "Snaps correctly for 'direction: rtl' with 'scroll-snap-align: start end' " +
    "alignment");