Bug 1774315 - Don't clamp target scroll position to layer scale. r=mstange

This prevents scrollLeft = <integer> to return scrollLeft = <integer>,
which is right now papered by bug 1674687.

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

Author: emilio

dom/events/test/window_bug1369072.html

@@ -21,7 +21,7 @@
 <div id="content" style="display: none">
 </div>
 <pre id="test">
-<script type="application/javascript">
+<script>
 
 SimpleTest.waitForExplicitFinish();
 SimpleTest.waitForFocus(runTests, window);
@@ -31,6 +31,11 @@
   window.opener.ok.apply(window.opener, arguments);
 }
 
+function info()
+{
+  window.opener.info.apply(window.opener, arguments);
+}
+
 function is()
 {
   window.opener.is.apply(window.opener, arguments);
@@ -89,23 +94,38 @@
   {
     await resetScroll();
 
-    return new Promise(resolve => {
-      // Wait scroll event
-      function onScroll() {
-        SimpleTest.executeSoon(resolve);
-      }
-      window.addEventListener("scroll", onScroll, { once: true });
-      iframe.contentWindow.addEventListener("scroll", onScroll, { once: true });
-
-      if (aVertical) {
-        synthesizeKey("KEY_ArrowDown");
-      } else {
-        synthesizeKey("KEY_ArrowRight");
-      }
-    });
+    function promiseScrollOnWindow(win) {
+      is(win.document.documentElement.scrollTop, 0, "Reset didn't work?");
+      is(win.document.documentElement.scrollLeft, 0, "Reset didn't work?");
+
+      return new Promise(resolve => {
+        win.addEventListener("scroll", function listener() {
+          // FIXME(bug 1674687): The first scroll might be fractional (and
+          // scrollTop / Left will round down to zero until bug 1674687 is
+          // fixed). So make sure we've scrolled at least a pixel before
+          // resolving. This can be removed once bug 1674687 is fixed.
+          if (win.document.documentElement.scrollLeft || win.document.documentElement.scrollTop) {
+            win.removeEventListener("scroll", listener);
+            SimpleTest.executeSoon(resolve);
+          }
+        });
+      });
+    }
+
+    let promise = Promise.race([
+      promiseScrollOnWindow(window),
+      promiseScrollOnWindow(iframe.contentWindow),
+    ]);
+
+    if (aVertical) {
+      synthesizeKey("KEY_ArrowDown");
+    } else {
+      synthesizeKey("KEY_ArrowRight");
+    }
+    return promise;
   }
 
-  // When iframe element has focus and the iframe document isn't scrollable, the parent document should be scrolled instead.
+  info("When iframe element has focus and the iframe document isn't scrollable, the parent document should be scrolled instead.");
   document.body.focus();
   iframe.focus();
   await tryToScrollWithKey(true);
@@ -114,7 +134,7 @@
   ok(document.documentElement.scrollLeft > 0, "ArrowRight keydown event at the iframe whose content is not scrollable should cause scrolling the parent document");
   await resetScroll();
 
-  // When iframe element has focus and the iframe document scrollable, the parent document shouldn't be scrolled.
+  info("When iframe element has focus and the iframe document scrollable, the parent document shouldn't be scrolled.");
   document.body.focus();
   div.style.height = "1000px";
   div.style.width = "1000px";
@@ -127,7 +147,7 @@
   ok(iframe.contentDocument.documentElement.scrollLeft > 0, "ArrowRight keydown event at the iframe whose content is scrollable should cause scrolling the iframe document");
   await resetScroll();
 
-  // If iframe document cannot scroll to specific direction, parent document should be scrolled instead.
+  info("If iframe document cannot scroll to specific direction, parent document should be scrolled instead.");
   div.style.height = "1px";
   div.style.width = "1000px";
   iframe.focus();
@@ -148,7 +168,7 @@
   ok(document.documentElement.scrollLeft > 0, "ArrowRight keydown event at the iframe whose content is scrollable only vertically should cause scrolling the parent document");
   await resetScroll();
 
-  // Hidden iframe shouldn't consume keyboard events if it was not scrollable.
+  info("Hidden iframe shouldn't consume keyboard events if it was not scrollable.");
   document.body.focus();
   anchor.focus();
   iframe.style.display = "none";

gfx/layers/apz/test/mochitest/helper_scroll_linked_effect_detector.html

@@ -54,7 +54,9 @@
   // Setup a scroll-linked effect callback.
   await promiseScrollAndEvent(() => {
     isnot(window.scrollY, 0, "we've already scrolled some amount");
-    target.style.top = window.scrollY + "px";
+    // ceil() makes sure that the non-effective setter below is actually
+    // non-effective.
+    target.style.top = Math.ceil(window.scrollY) + "px";
     eventTimeStamp = document.timeline.currentTime;
   });
   is(eventTimeStamp, document.timeline.currentTime,

gfx/layers/apz/test/mochitest/test_bug1151667.html

@@ -48,8 +48,16 @@
   var subframe = document.getElementById("subframe");
   await promiseNativeWheelAndWaitForScrollEvent(subframe, 100, 150, 0, -10);
 
-  is(subframe.scrollTop > 0, true, "We should have scrolled the subframe down");
-  is(document.documentElement.scrollTop, 0, "We should not have scrolled the page");
+  while (!subframe.scrollTop) {
+    is(document.documentElement.scrollTop, 0, "We should not have scrolled the page");
+    info("Waiting for one more scroll");
+    // FIXME(bug 1674687): The first scroll might be fractional, and thus still
+    // round down to zero. Wait for another scroll event.
+    // Remove this when bug 1674687 is fixed.
+    await new Promise(r => subframe.addEventListener("scroll", r, { once: true }));
+  }
+  is(document.documentElement.scrollTop, 0, "We should still not have scrolled the page");
+  ok(subframe.scrollTop > 0, "We should have scrolled the subframe down");
 }
 
 SimpleTest.waitForExplicitFinish();

layout/generic/nsGfxScrollFrame.cpp

@@ -2787,89 +2787,28 @@ void nsHTMLScrollFrame::ScrollVisual() {
 
 /**
  * Clamp desired scroll position aDesired and range [aDestLower, aDestUpper]
- * to [aBoundLower, aBoundUpper] and then select the appunit value from among
- * aBoundLower, aBoundUpper and those such that (aDesired - aCurrent) *
- * aRes/aAppUnitsPerPixel is an integer (or as close as we can get
- * modulo rounding to appunits) that is in [aDestLower, aDestUpper] and
- * closest to aDesired.  If no such value exists, return the nearest in
- * [aDestLower, aDestUpper].
+ * to [aBoundLower, aBoundUpper]
  */
-static nscoord ClampAndAlignWithPixels(nscoord aDesired, nscoord aBoundLower,
+static nscoord ClampScrollPositionAxis(nscoord aDesired, nscoord aBoundLower,
                                        nscoord aBoundUpper, nscoord aDestLower,
-                                       nscoord aDestUpper,
-                                       nscoord aAppUnitsPerPixel, double aRes,
-                                       nscoord aCurrent) {
+                                       nscoord aDestUpper) {
   // Intersect scroll range with allowed range, by clamping the ends
   // of aRange to be within bounds
   nscoord destLower = clamped(aDestLower, aBoundLower, aBoundUpper);
   nscoord destUpper = clamped(aDestUpper, aBoundLower, aBoundUpper);
-
-  nscoord desired = clamped(aDesired, destLower, destUpper);
-
-  double currentLayerVal = (aRes * aCurrent) / aAppUnitsPerPixel;
-  double desiredLayerVal = (aRes * desired) / aAppUnitsPerPixel;
-  double delta = desiredLayerVal - currentLayerVal;
-  double nearestLayerVal = NS_round(delta) + currentLayerVal;
-
-  // Convert back from PaintedLayer space to appunits relative to the top-left
-  // of the scrolled frame.
-  nscoord aligned =
-      aRes == 0.0
-          ? 0.0
-          : NSToCoordRoundWithClamp(nearestLayerVal * aAppUnitsPerPixel / aRes);
-
-  // Use a bound if it is within the allowed range and closer to desired than
-  // the nearest pixel-aligned value.
-  if (aBoundUpper == destUpper &&
-      static_cast<decltype(Abs(desired))>(aBoundUpper - desired) <
-          Abs(desired - aligned)) {
-    return aBoundUpper;
-  }
-
-  if (aBoundLower == destLower &&
-      static_cast<decltype(Abs(desired))>(desired - aBoundLower) <
-          Abs(aligned - desired)) {
-    return aBoundLower;
-  }
-
-  // Accept the nearest pixel-aligned value if it is within the allowed range.
-  if (aligned >= destLower && aligned <= destUpper) {
-    return aligned;
-  }
-
-  // Check if opposite pixel boundary fits into allowed range.
-  double oppositeLayerVal =
-      nearestLayerVal + ((nearestLayerVal < desiredLayerVal) ? 1.0 : -1.0);
-  nscoord opposite = aRes == 0.0
-                         ? 0.0
-                         : NSToCoordRoundWithClamp(oppositeLayerVal *
-                                                   aAppUnitsPerPixel / aRes);
-  if (opposite >= destLower && opposite <= destUpper) {
-    return opposite;
-  }
-
-  // No alignment available.
-  return desired;
+  return clamped(aDesired, destLower, destUpper);
 }
 
 /**
- * Clamp desired scroll position aPt to aBounds and then snap
- * it to the same layer pixel edges as aCurrent, keeping it within aRange
- * during snapping. aCurrent is the current scroll position.
+ * Clamp desired scroll position aPt to aBounds, keeping it within aRange.
+ * aCurrent is the current scroll position.
  */
-static nsPoint ClampAndAlignWithLayerPixels(const nsPoint& aPt,
-                                            const nsRect& aBounds,
-                                            const nsRect& aRange,
-                                            const nsPoint& aCurrent,
-                                            nscoord aAppUnitsPerPixel,
-                                            const MatrixScales& aScale) {
-  return nsPoint(
-      ClampAndAlignWithPixels(aPt.x, aBounds.x, aBounds.XMost(), aRange.x,
-                              aRange.XMost(), aAppUnitsPerPixel, aScale.xScale,
-                              aCurrent.x),
-      ClampAndAlignWithPixels(aPt.y, aBounds.y, aBounds.YMost(), aRange.y,
-                              aRange.YMost(), aAppUnitsPerPixel, aScale.yScale,
-                              aCurrent.y));
+static nsPoint ClampScrollPosition(const nsPoint& aPt, const nsRect& aBounds,
+                                   const nsRect& aRange) {
+  return nsPoint(ClampScrollPositionAxis(aPt.x, aBounds.x, aBounds.XMost(),
+                                         aRange.x, aRange.XMost()),
+                 ClampScrollPositionAxis(aPt.y, aBounds.y, aBounds.YMost(),
+                                         aRange.y, aRange.YMost()));
 }
 
 /* static */
@@ -2983,11 +2922,7 @@ void nsHTMLScrollFrame::ScrollToImpl(
   }
 
   nsPresContext* presContext = PresContext();
-  nscoord appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel();
-  // 'scale' is our estimate of the scale factor that will be applied
-  // when rendering the scrolled content to its own PaintedLayer.
-  MatrixScales scale = GetPaintedLayerScaleForFrame(mScrolledFrame);
-  nsPoint curPos = GetScrollPosition();
+  const nsPoint curPos = GetScrollPosition();
 
   // Try to align aPt with curPos so they have an integer number of layer
   // pixels between them. This gives us the best chance of scrolling without
@@ -2999,8 +2934,7 @@ void nsHTMLScrollFrame::ScrollToImpl(
   // and are relative to the scrollport top-left. This difference doesn't
   // actually matter since all we are about is that there be an integer number
   // of layer pixels between pt and curPos.
-  nsPoint pt = ClampAndAlignWithLayerPixels(aPt, GetLayoutScrollRange(), aRange,
-                                            curPos, appUnitsPerDevPixel, scale);
+  nsPoint pt = ClampScrollPosition(aPt, GetLayoutScrollRange(), aRange);
   if (pt == curPos) {
     // Even if we are bailing out due to no-op main-thread scroll position
     // change, we might need to cancel an APZ smooth scroll that we already

testing/web-platform/meta/css/css-writing-modes/forms/text-input-vertical-overflow-no-scroll.html.ini

@@ -16,13 +16,6 @@
 
   [input[type=text\] in sideways-lr: typing characters in input should not cause the page to scroll]
     expected:
-      if (os == "win") and (processor == "x86_64") and debug and not swgl: FAIL
-      if (os == "win") and (processor == "x86_64") and debug and swgl: [FAIL, PASS]
-      if (os == "linux") and not debug and (processor == "x86_64") and fission: [FAIL, PASS]
-      if (os == "win") and (processor == "x86") and debug: [FAIL, PASS]
-      if (os == "android") and debug and not swgl: PASS
-      if (os == "linux") and not debug and (processor == "x86"): [FAIL, PASS]
-      if (os == "linux") and debug: FAIL
       if os == "mac": PASS
       [PASS, FAIL]
 
@@ -46,12 +39,6 @@
 
   [input[type=password\] in sideways-lr: typing characters in input should not cause the page to scroll]
     expected:
-      if (os == "win") and not debug and (processor == "x86"): [PASS, FAIL]
-      if (os == "android") and debug and swgl: [PASS, FAIL]
-      if (os == "android") and debug and not swgl: PASS
-      if (os == "linux") and not debug and not fission: [PASS, FAIL]
-      if (os == "android") and not debug: [PASS, FAIL]
-      if (os == "linux") and debug: FAIL
       if os == "mac": PASS
       [FAIL, PASS]
 
@@ -73,12 +60,6 @@
 
   [input[type=search\] in sideways-lr: typing characters in input should not cause the page to scroll]
     expected:
-      if (os == "win") and not swgl and not debug and (processor == "x86"): [PASS, FAIL]
-      if (os == "android") and debug and not swgl: PASS
-      if (os == "android") and debug and swgl: [PASS, FAIL]
-      if (os == "linux") and debug: FAIL
-      if (os == "android") and not debug: [PASS, FAIL]
-      if (os == "win") and swgl: [PASS, FAIL]
       if os == "mac": PASS
       [FAIL, PASS]
 
@@ -97,13 +78,6 @@
 
   [input[type=number\] in sideways-lr: typing characters in input should not cause the page to scroll]
     expected:
-      if (os == "linux") and debug and fission and not swgl: FAIL
-      if (os == "linux") and debug and not fission: FAIL
-      if (os == "android") and debug and swgl: [PASS, FAIL]
-      if (os == "android") and debug and not swgl: PASS
-      if (os == "win") and debug and (processor == "x86"): FAIL
-      if (os == "win") and not debug and (processor == "x86_64"): [PASS, FAIL]
-      if (os == "android") and not debug: [PASS, FAIL]
       if os == "mac": PASS
       [FAIL, PASS]