Bug 754556. Ensure that setting scroll positions in CSS pixels doesn't unexpectedly move the scroll position, especially not in the wrong direction. r=matspal

If the current scroll position is fractional, e.g. y=N.4 CSS pixels, and something tries to read the
position in CSS pixels and scroll to that position (e.g. calling window.scrollTo(0, rootElem.scrollTop),
or equivalently window.scrollBy(0, 0)), it can actually end up scrolling backwards. So create a new
method nsIScrollableFrame::ScrollToCSSPixels which ensures that scrolling to a CSS pixel offset tries to
preserve the current fractional scroll position if that's possible, and if that's not possible at least does
not allow the scroll position to move in the wrong direction.

content/base/src/nsGenericElement.cpp

@@ -2133,11 +2133,8 @@ nsGenericElement::SetScrollTop(PRInt32 aScrollTop)
   nsIScrollableFrame* sf = GetScrollFrame();
   if (sf) {
     nsPoint pt = sf->GetScrollPosition();
-    pt.y = nsPresContext::CSSPixelsToAppUnits(aScrollTop);
+    sf->ScrollToCSSPixels(nsIntPoint(nsPresContext::AppUnitsToIntCSSPixels(pt.x),
-    nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
+                                     aScrollTop));
-    // Don't allow pt.y + halfPixel since that would round up to the next CSS pixel.
-    nsRect range(pt.x, pt.y - halfPixel, 0, halfPixel*2 - 1);
-    sf->ScrollTo(pt, nsIScrollableFrame::INSTANT, &range);
   }
   return NS_OK;
 }
@@ -2166,11 +2163,8 @@ nsGenericElement::SetScrollLeft(PRInt32 aScrollLeft)
   nsIScrollableFrame* sf = GetScrollFrame();
   if (sf) {
     nsPoint pt = sf->GetScrollPosition();
-    pt.x = nsPresContext::CSSPixelsToAppUnits(aScrollLeft);
+    sf->ScrollToCSSPixels(nsIntPoint(aScrollLeft,
-    nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
+                                     nsPresContext::AppUnitsToIntCSSPixels(pt.y)));
-    // Don't allow pt.x + halfPixel since that would round up to the next CSS pixel.
-    nsRect range(pt.x - halfPixel, pt.y, halfPixel*2 - 1, 0);
-    sf->ScrollTo(pt, nsIScrollableFrame::INSTANT, &range);
   }
   return NS_OK;
 }

dom/base/nsGlobalWindow.cpp

@@ -5328,12 +5328,7 @@ nsGlobalWindow::ScrollTo(PRInt32 aXScroll, PRInt32 aYScroll)
     if (aYScroll > maxpx) {
       aYScroll = maxpx;
     }
-    nsPoint pt(nsPresContext::CSSPixelsToAppUnits(aXScroll),
+    sf->ScrollToCSSPixels(nsIntPoint(aXScroll, aYScroll));
-               nsPresContext::CSSPixelsToAppUnits(aYScroll));
-    nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
-    // Don't allow pt.x/y + halfPixel since that would round up to the next CSS pixel.
-    nsRect range(pt.x - halfPixel, pt.y - halfPixel, halfPixel*2 - 1, halfPixel*2 - 1);
-    sf->ScrollTo(pt, nsIScrollableFrame::INSTANT, &range);
   }
 
   return NS_OK;

layout/generic/nsGfxScrollFrame.cpp

@@ -1724,6 +1724,31 @@ nsGfxScrollFrameInner::AsyncScrollCallback(void* anInstance, mozilla::TimeStamp
   self->ScrollToImpl(self->mDestination, range);
 }
 
+void
+nsGfxScrollFrameInner::ScrollToCSSPixels(nsIntPoint aScrollPosition)
+{
+  nsPoint current = GetScrollPosition();
+  nsPoint pt(nsPresContext::CSSPixelsToAppUnits(aScrollPosition.x),
+             nsPresContext::CSSPixelsToAppUnits(aScrollPosition.y));
+  nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
+  nsRect range(pt.x - halfPixel, pt.y - halfPixel, 2*halfPixel - 1, 2*halfPixel - 1);
+  if (nsPresContext::AppUnitsToIntCSSPixels(current.x) == aScrollPosition.x) {
+    pt.x = current.x;
+    range.x = pt.x;
+    range.width = 0;
+  } else {
+    // current.x must be outside 'range', so we must move in the correct direction.
+  }
+  if (nsPresContext::AppUnitsToIntCSSPixels(current.y) == aScrollPosition.y) {
+    pt.y = current.y;
+    range.y = pt.y;
+    range.height = 0;
+  } else {
+    // current.y must be outside 'range', so we must move in the correct direction.
+  }
+  ScrollTo(pt, nsIScrollableFrame::INSTANT, &range);
+}
+
 /*
  * this method wraps calls to ScrollToImpl(), either in one shot or incrementally,
  *  based on the setting of the smoothness scroll pref

layout/generic/nsGfxScrollFrame.h

@@ -195,6 +195,7 @@ public:
                 const nsRect* aRange = nsnull) {
     ScrollToWithOrigin(aScrollPosition, aMode, nsGkAtoms::other, aRange);
   }
+  void ScrollToCSSPixels(nsIntPoint aScrollPosition);
   void ScrollToImpl(nsPoint aScrollPosition, const nsRect& aRange);
   void ScrollVisual(nsPoint aOldScrolledFramePosition);
   void ScrollBy(nsIntPoint aDelta, nsIScrollableFrame::ScrollUnit aUnit,
@@ -505,6 +506,9 @@ public:
                         const nsRect* aRange = nsnull) {
     mInner.ScrollTo(aScrollPosition, aMode, aRange);
   }
+  virtual void ScrollToCSSPixels(nsIntPoint aScrollPosition) {
+    mInner.ScrollToCSSPixels(aScrollPosition);
+  }
   virtual void ScrollBy(nsIntPoint aDelta, ScrollUnit aUnit, ScrollMode aMode,
                         nsIntPoint* aOverflow, nsIAtom *aOrigin = nsnull) {
     mInner.ScrollBy(aDelta, aUnit, aMode, aOverflow, aOrigin);
@@ -747,6 +751,9 @@ public:
                         const nsRect* aRange = nsnull) {
     mInner.ScrollTo(aScrollPosition, aMode, aRange);
   }
+  virtual void ScrollToCSSPixels(nsIntPoint aScrollPosition) {
+    mInner.ScrollToCSSPixels(aScrollPosition);
+  }
   virtual void ScrollBy(nsIntPoint aDelta, ScrollUnit aUnit, ScrollMode aMode,
                         nsIntPoint* aOverflow, nsIAtom *aOrigin = nsnull) {
     mInner.ScrollBy(aDelta, aUnit, aMode, aOverflow, aOrigin);

layout/generic/nsIScrollableFrame.h

@@ -156,6 +156,15 @@ public:
    */
   virtual void ScrollTo(nsPoint aScrollPosition, ScrollMode aMode,
                         const nsRect* aRange = nsnull) = 0;
+  /**
+   * Scrolls to a particular position in integer CSS pixels.
+   * Keeps the exact current horizontal or vertical position if the current
+   * position, rounded to CSS pixels, matches aScrollPosition. If
+   * aScrollPosition.x/y is different from the current CSS pixel position,
+   * makes sure we only move in the direction given by the difference.
+   * The scroll mode is INSTANT.
+   */
+  virtual void ScrollToCSSPixels(nsIntPoint aScrollPosition) = 0;
   /**
    * When scrolling by a relative amount, we can choose various units.
    */

layout/reftests/scrolling/reftest.list

@@ -8,6 +8,7 @@ random-if(Android) HTTP == image-1.html image-1.html?ref
 HTTP == opacity-mixed-scrolling-1.html opacity-mixed-scrolling-1.html?ref
 random-if(cocoaWidget) HTTP == opacity-mixed-scrolling-2.html opacity-mixed-scrolling-2.html?ref # see bug 625357
 HTTP == simple-1.html simple-1.html?ref
+HTTP == subpixel-1.html#d subpixel-1-ref.html#d
 HTTP == text-1.html text-1.html?ref
 HTTP == text-2.html?up text-2.html?ref
 HTTP == transformed-1.html transformed-1.html?ref

layout/reftests/scrolling/subpixel-1-ref.html

@@ -0,0 +1,7 @@
+<!DOCTYPE HTML>
+<html>
+<body style="margin:0">
+<div style="height:1205.6px"></div>
+<div id="d" style="height:0.6px; background:red"></div>
+</body>
+</html>

layout/reftests/scrolling/subpixel-1.html

@@ -0,0 +1,12 @@
+<!DOCTYPE HTML>
+<html>
+<body style="margin:0" onload="doTest()">
+<div style="height:1205.6px"></div>
+<div id="d" style="height:0.6px; background:red"></div>
+<script>
+function doTest() {
+  window.scrollTo(0, document.documentElement.scrollTop);
+}
+</script>
+</body>
+</html>