Bug 1373349 - Correct the origin of the content rect when creating scroll layers in WR. r?botond,jrmuizel draft
authorKartikaya Gupta <kgupta@mozilla.com>
Thu, 15 Jun 2017 15:35:04 -0400
changeset 594959 d8785daae47b776cac18df00b6e7e536551ba28b
parent 594958 56d56ee5a01add1d469065220e99b068c0a53517
child 633587 f382aea817e2a8fc2f63eeda27bde50b81f0d12d
push id64207
push userkgupta@mozilla.com
push dateThu, 15 Jun 2017 20:22:56 +0000
reviewersbotond, jrmuizel
bugs1373349
milestone56.0a1
Bug 1373349 - Correct the origin of the content rect when creating scroll layers in WR. r?botond,jrmuizel Also tweaks the documentation in FrameMetrics.h to be more detailed and less misleading. MozReview-Commit-ID: 183V2Q9kY6C
gfx/layers/FrameMetrics.h
gfx/layers/wr/ScrollingLayersHelper.cpp
--- a/gfx/layers/FrameMetrics.h
+++ b/gfx/layers/FrameMetrics.h
@@ -543,20 +543,24 @@ private:
   // The same restrictions for mDisplayPort apply here.
   CSSRect mCriticalDisplayPort;
 
   // The scrollable bounds of a frame. This is determined by reflow.
   // Ordinarily the x and y will be 0 and the width and height will be the
   // size of the element being scrolled. However for RTL pages or elements
   // the x value may be negative.
   //
-  // This is relative to the document. It is in the same coordinate space as
-  // |mScrollOffset|, but a different coordinate space than |mViewport| and
-  // |mDisplayPort|. Note also that this coordinate system is understood by
-  // window.scrollTo().
+  // For scrollable frames that are overflow:hidden the x and y are usually
+  // set to the value of the current scroll offset, and the width and height
+  // will match the composition bounds width and height. In effect this reduces
+  // the scrollable range to 0.
+  //
+  // This is in the same coordinate space as |mScrollOffset|, but a different
+  // coordinate space than |mViewport| and |mDisplayPort|. Note also that this
+  // coordinate system is understood by window.scrollTo().
   //
   // This is valid on any layer unless it has no content.
   CSSRect mScrollableRect;
 
   // The cumulative resolution that the current frame has been painted at.
   // This is the product of the pres-shell resolutions of the document
   // containing this scroll frame and its ancestors, and any css-driven
   // resolution. This information is provided by Gecko at layout/paint time.
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@@ -58,21 +58,23 @@ ScrollingLayersHelper::ScrollingLayersHe
         PixelCastJustification::WebRenderHasUnitResolution);
     // TODO: check coordinate systems are sane here
     LayerRect clipBounds = ViewAs<LayerPixel>(
         fm.GetCompositionBounds(),
         PixelCastJustification::MovingDownToChildren);
     // The content rect that we hand to PushScrollLayer should be relative to
     // the same origin as the clipBounds that we hand to PushScrollLayer - that
     // is, both of them should be relative to the stacking context `aStackingContext`.
-    // However, when we get the scrollable rect from the FrameMetrics, it has
-    // a nominal top-left of 0,0 (maybe different for RTL pages?) and so to
-    // get it in the same coordinate space we're going to shift it by the
-    // composition bounds top-left.
-    contentRect.MoveBy(clipBounds.TopLeft());
+    // However, when we get the scrollable rect from the FrameMetrics, the origin
+    // has nothing to do with the position of the frame but instead represents
+    // the minimum allowed scroll offset of the scrollable content. While APZ
+    // uses this to clamp the scroll position, we don't need to send this to
+    // WebRender at all. Instead, we take the position from the composition
+    // bounds.
+    contentRect.MoveTo(clipBounds.TopLeft());
     mBuilder->PushScrollLayer(fm.GetScrollId(),
         aStackingContext.ToRelativeWrRect(contentRect),
         aStackingContext.ToRelativeWrRect(clipBounds));
   }
 
   // The scrolled clip on the layer is "inside" all of the scrollable metadatas
   // on that layer. That is, the clip scrolls along with the content in
   // child layers. So we need to apply this after pushing all the scroll layers,