Bug 1544325 - Clamp the app units' scroll position to the current position if the given scroll position is the same as the current scroll position in CSS pixels. r=botond

There are conditions that the given scroll position in app units differs from
the current position in app units even if those values are the same in CSS
pixels.  Setting Element.scrollTop with Element.scrollTop in a callback of
scroll event is one of such cases.

The change here looks messy, but I am totally unsure that we can clamp the
values before initializing the range for the DISABLE_SNAP case, so I'd like to
preserve the behavior there.  Once we ship the new scroll snap, we can drop
the DISABLE_SNAP case there so that it could be less messy at the time.

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

--HG--
extra : moz-landing-system : lando

layout/generic/nsGfxScrollFrame.cpp

@@ -2173,35 +2173,42 @@ void ScrollFrameHelper::ScrollToCSSPixels(
   nsPoint current = GetScrollPosition();
   CSSIntPoint currentCSSPixels = GetScrollPositionCSSPixels();
   nsPoint pt = CSSPoint::ToAppUnits(aScrollPosition);
-  nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
-
   if (aSnap == nsIScrollableFrame::DEFAULT) {
     aSnap = DefaultSnapMode();
   }
 
-  nsRect range;
-  if (aSnap != nsIScrollableFrame::ENABLE_SNAP) {
-    range = nsRect(pt.x - halfPixel, pt.y - halfPixel, 2 * halfPixel - 1,
-                   2 * halfPixel - 1);
-    // XXX I don't think the following blocks are needed anymore, now that
-    // ScrollToImpl simply tries to scroll an integer number of layer
-    // pixels from the current position
-    if (currentCSSPixels.x == aScrollPosition.x) {
-      pt.x = current.x;
-      range.x = pt.x;
-      range.width = 0;
-    }
-    if (currentCSSPixels.y == aScrollPosition.y) {
-      pt.y = current.y;
-      range.y = pt.y;
-      range.height = 0;
-    }
-  }
   if (aOrigin == nullptr) {
     aOrigin = nsGkAtoms::other;
   }
-  ScrollTo(pt, aMode, aOrigin,
-           aSnap == nsIScrollableFrame::ENABLE_SNAP ? nullptr : &range, aSnap);
+
+  if (aSnap == nsIScrollableFrame::ENABLE_SNAP) {
+    if (currentCSSPixels.x == aScrollPosition.x) {
+      pt.x = current.x;
+    }
+    if (currentCSSPixels.y == aScrollPosition.y) {
+      pt.y = current.y;
+    }
+    ScrollTo(pt, aMode, aOrigin, nullptr /* range */, aSnap);
+    return;
+  }
+
+  nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
+  nsRect range(pt.x - halfPixel, pt.y - halfPixel, 2 * halfPixel - 1,
+               2 * halfPixel - 1);
+  // XXX I don't think the following blocks are needed anymore, now that
+  // ScrollToImpl simply tries to scroll an integer number of layer
+  // pixels from the current position
+  if (currentCSSPixels.x == aScrollPosition.x) {
+    pt.x = current.x;
+    range.x = pt.x;
+    range.width = 0;
+  }
+  if (currentCSSPixels.y == aScrollPosition.y) {
+    pt.y = current.y;
+    range.y = pt.y;
+    range.height = 0;
+  }
+  ScrollTo(pt, aMode, aOrigin, &range, aSnap);
   // 'this' might be destroyed here
 }
 
@@ -4257,36 +4264,43 @@ void ScrollFrameHelper::ScrollByCSSPixels(
     nsIScrollbarMediator::ScrollSnapMode aSnap) {
   nsPoint current = GetScrollPosition();
   nsPoint pt = current + CSSPoint::ToAppUnits(aDelta);
-  nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
 
   if (aSnap == nsIScrollableFrame::DEFAULT) {
     aSnap = DefaultSnapMode();
   }
 
-  nsRect range;
-  if (aSnap != nsIScrollableFrame::ENABLE_SNAP) {
-    range = nsRect(pt.x - halfPixel, pt.y - halfPixel, 2 * halfPixel - 1,
-                   2 * halfPixel - 1);
-    // XXX I don't think the following blocks are needed anymore, now that
-    // ScrollToImpl simply tries to scroll an integer number of layer
-    // pixels from the current position
-    if (aDelta.x == 0.0f) {
-      pt.x = current.x;
-      range.x = pt.x;
-      range.width = 0;
-    }
-    if (aDelta.y == 0.0f) {
-      pt.y = current.y;
-      range.y = pt.y;
-      range.height = 0;
-    }
-  }
   if (aOrigin == nullptr) {
     aOrigin = nsGkAtoms::other;
   }
-  ScrollToWithOrigin(
-      pt, aMode, aOrigin,
-      aSnap == nsIScrollableFrame::ENABLE_SNAP ? nullptr : &range, aSnap);
+
+  if (aSnap == nsIScrollableFrame::ENABLE_SNAP) {
+    if (aDelta.x == 0.0f) {
+      pt.x = current.x;
+    }
+    if (aDelta.y == 0.0f) {
+      pt.y = current.y;
+    }
+    ScrollToWithOrigin(pt, aMode, aOrigin, nullptr /* range */, aSnap);
+    return;
+  }
+
+  nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
+  nsRect range(pt.x - halfPixel, pt.y - halfPixel, 2 * halfPixel - 1,
+               2 * halfPixel - 1);
+  // XXX I don't think the following blocks are needed anymore, now that
+  // ScrollToImpl simply tries to scroll an integer number of layer
+  // pixels from the current position
+  if (aDelta.x == 0.0f) {
+    pt.x = current.x;
+    range.x = pt.x;
+    range.width = 0;
+  }
+  if (aDelta.y == 0.0f) {
+    pt.y = current.y;
+    range.y = pt.y;
+    range.height = 0;
+  }
+  ScrollToWithOrigin(pt, aMode, aOrigin, &range, aSnap);
   // 'this' might be destroyed here
 }