Bug 1410527 - Update how we tell WR about position:sticky elements. r?mstange draft
authorKartikaya Gupta <kgupta@mozilla.com>
Wed, 25 Oct 2017 15:21:32 -0400
changeset 686346 caaa7f1a3b9678d844e5e5921d94ad0f7674e4c7
parent 686335 a97fb1c39d51a7337b46957bde4273bd308b796a
child 737363 b6f56ff384e475f88fe6462d88ef4b2c445cb66c
push id86171
push userkgupta@mozilla.com
push dateWed, 25 Oct 2017 19:21:50 +0000
reviewersmstange
bugs1410527
milestone58.0a1
Bug 1410527 - Update how we tell WR about position:sticky elements. r?mstange My original understanding of the API was flawed, and so while I had position:sticky working in some cases it didn't work properly in a lot of other cases. This patch corrects the usage of the API to match what WR is expecting and fixes a lot of test cases. MozReview-Commit-ID: AdMux19Fk9U
gfx/layers/wr/StackingContextHelper.cpp
gfx/layers/wr/StackingContextHelper.h
layout/painting/nsDisplayList.cpp
layout/reftests/position-sticky/reftest.list
--- a/gfx/layers/wr/StackingContextHelper.cpp
+++ b/gfx/layers/wr/StackingContextHelper.cpp
@@ -55,22 +55,16 @@ StackingContextHelper::StackingContextHe
 
 StackingContextHelper::~StackingContextHelper()
 {
   if (mBuilder) {
     mBuilder->PopStackingContext();
   }
 }
 
-void
-StackingContextHelper::AdjustOrigin(const LayoutDevicePoint& aDelta)
-{
-  mOrigin += aDelta;
-}
-
 wr::LayoutRect
 StackingContextHelper::ToRelativeLayoutRect(const LayoutDeviceRect& aRect) const
 {
   return wr::ToLayoutRect(RoundedToInt(aRect - mOrigin));
 }
 
 } // namespace layers
 } // namespace mozilla
--- a/gfx/layers/wr/StackingContextHelper.h
+++ b/gfx/layers/wr/StackingContextHelper.h
@@ -41,18 +41,16 @@ public:
   // of the tree, so that we have a StackingContextHelper to pass down into
   // the RenderLayer traversal, but don't actually want it to push a stacking
   // context on the display list builder.
   StackingContextHelper();
 
   // Pops the stacking context, if one was pushed during the constructor.
   ~StackingContextHelper();
 
-  void AdjustOrigin(const LayoutDevicePoint& aDelta);
-
   // When this StackingContextHelper is in scope, this function can be used
   // to convert a rect from the layer system's coordinate space to a LayoutRect
   // that is relative to the stacking context. This is useful because most
   // things that are pushed inside the stacking context need to be relative
   // to the stacking context.
   // We allow passing in a LayoutDeviceRect for convenience because in a lot of
   // cases with WebRender display item generate the layout device space is the
   // same as the layer space. (TODO: try to make this more explicit somehow).
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -7271,107 +7271,128 @@ nsDisplayStickyPosition::BuildLayer(nsDi
                           aContainerParameters.mXScale,
                         NSAppUnitsToFloatPixels(inner.height, factor) *
                           aContainerParameters.mYScale);
   layer->SetStickyPositionData(scrollId, stickyOuter, stickyInner);
 
   return layer.forget();
 }
 
+// Returns the smallest distance from "0" to the range [min, max] where
+// min <= max.
+static nscoord DistanceToRange(nscoord min, nscoord max) {
+  MOZ_ASSERT(min <= max);
+  if (max < 0) {
+    return max;
+  }
+  if (min > 0) {
+    return min;
+  }
+  MOZ_ASSERT(min <= 0 && max >= 0);
+  return 0;
+}
+
 bool
 nsDisplayStickyPosition::CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,
                                                  mozilla::wr::IpcResourceUpdateQueue& aResources,
                                                  const StackingContextHelper& aSc,
                                                  WebRenderLayerManager* aManager,
                                                  nsDisplayListBuilder* aDisplayListBuilder)
 {
-  LayoutDevicePoint scTranslation;
   StickyScrollContainer* stickyScrollContainer = StickyScrollContainer::GetStickyScrollContainerForFrame(mFrame);
   if (stickyScrollContainer) {
     float auPerDevPixel = mFrame->PresContext()->AppUnitsPerDevPixel();
 
     bool snap;
     nsRect itemBounds = GetBounds(aDisplayListBuilder, &snap);
 
-    // The itemBounds here already take into account the main-thread
-    // position:sticky implementation, so we need to unapply that.
-    nsIFrame* firstCont = nsLayoutUtils::FirstContinuationOrIBSplitSibling(mFrame);
-    nsPoint translation = stickyScrollContainer->ComputePosition(firstCont) - firstCont->GetNormalPosition();
-    itemBounds.MoveBy(-translation);
-    scTranslation = LayoutDevicePoint::FromAppUnits(translation, auPerDevPixel);
-
-    LayoutDeviceRect bounds = LayoutDeviceRect::FromAppUnits(itemBounds, auPerDevPixel);
-
     Maybe<wr::StickySideConstraint> top;
     Maybe<wr::StickySideConstraint> right;
     Maybe<wr::StickySideConstraint> bottom;
     Maybe<wr::StickySideConstraint> left;
 
     nsRect outer;
     nsRect inner;
     stickyScrollContainer->GetScrollRanges(mFrame, &outer, &inner);
 
+    nsIFrame* scrollFrame = do_QueryFrame(stickyScrollContainer->ScrollFrame());
+    nsPoint offset = scrollFrame->GetOffsetTo(ReferenceFrame());
+
+    // Adjust the scrollPort coordinates to be relative to the reference frame,
+    // so that it is in the same space as everything else.
     nsRect scrollPort = stickyScrollContainer->ScrollFrame()->GetScrollPortRect();
+    scrollPort += offset;
 
     // The following computations make more sense upon understanding the
     // semantics of "inner" and "outer", which is explained in the comment on
     // SetStickyPositionData in Layers.h.
 
     if (outer.YMost() != inner.YMost()) {
       // Question: How far will itemBounds.y be from the top of the scrollport
-      // when we have scrolled down from the current scroll position of "0" to a
-      // scroll position of "inner.YMost()" (which is >= 0 since we are
-      // scrolling down)?
-      // Answer: (itemBounds.y - 0) - (inner.YMost() - 0)
-      //      == itemBounds.y - inner.YMost()
-      float margin = NSAppUnitsToFloatPixels(itemBounds.y - inner.YMost(), auPerDevPixel);
-      // The scroll distance during which the item should remain "stuck"
-      float maxOffset = NSAppUnitsToFloatPixels(outer.YMost() - inner.YMost(), auPerDevPixel);
+      // when we have scrolled from the current scroll position of "0" to
+      // reach the range [inner.YMost(), outer.YMost()] where the item gets
+      // stuck?
+      // Answer: the current distance is "itemBounds.y - scrollPort.y". That
+      // needs to be adjusted by the distance to the range. If the distance is
+      // negative (i.e. inner.YMost() <= outer.YMost() < 0) then we would be
+      // scrolling upwards (decreasing scroll offset) to reach that range,
+      // which would increase itemBounds.y and make it farther away from the
+      // top of the scrollport. So in that case the adjustment is -distance.
+      // If the distance is positive (0 < inner.YMost() <= outer.YMost()) then
+      // we would be scrolling downwards, itemBounds.y would decrease, and we
+      // again need to adjust by -distance. If we are already in the range
+      // then no adjustment is needed and distance is 0 so again using
+      // -distance works.
+      nscoord distance = DistanceToRange(inner.YMost(), outer.YMost());
+      float margin = NSAppUnitsToFloatPixels(itemBounds.y - scrollPort.y - distance, auPerDevPixel);
+      // Question: Given the current state, what is the range during which
+      // WR will have to apply an adjustment to the item (in order to prevent
+      // the item from visually moving) as a result of async scrolling?
+      // Answer: [inner.YMost(), outer.YMost()]. But right now the WR API
+      // doesn't allow us to provide the whole range; it just takes one side
+      // of the range and assumes it has a particular sign. Bug 1411627 will
+      // fix this more completely but for now we do the best we can. Note that
+      // this value also needs to be converted from being relative to the
+      // scrollframe to being relative to the reference frame, so we have to
+      // adjust it by |offset|.
+      float maxOffset = NSAppUnitsToFloatPixels(std::max(0, outer.YMost() + offset.y), auPerDevPixel);
       top = Some(wr::StickySideConstraint { margin, maxOffset });
     }
     if (outer.y != inner.y) {
-      // Question: How far will itemBounds.YMost() be from the bottom of the
-      // scrollport when we have scrolled up from the current scroll position of
-      // "0" to a scroll position of "inner.y" (which is <= 0 since we are
-      // scrolling up)?
-      // Answer: (scrollPort.height - itemBounds.YMost()) - (0 - inner.y)
-      //      == scrollPort.height - itemBounds.YMost() + inner.y
-      float margin = NSAppUnitsToFloatPixels(scrollPort.height - itemBounds.YMost() + inner.y, auPerDevPixel);
-      // The scroll distance during which the item should remain "stuck"
-      float maxOffset = NSAppUnitsToFloatPixels(outer.y - inner.y, auPerDevPixel);
+      // Similar logic as in the previous section, but this time we care about
+      // the distance from itemBounds.YMost() to scrollPort.YMost().
+      nscoord distance = DistanceToRange(outer.y, inner.y);
+      float margin = NSAppUnitsToFloatPixels(scrollPort.YMost() - itemBounds.YMost() + distance, auPerDevPixel);
+      // And here WR will be moving the item upwards rather than downwards so
+      // again things are inverted from the previous block.
+      float maxOffset = NSAppUnitsToFloatPixels(std::min(0, outer.y + offset.y), auPerDevPixel);
       bottom = Some(wr::StickySideConstraint { margin, maxOffset });
     }
     // Same as above, but for the x-axis
     if (outer.XMost() != inner.XMost()) {
-      float margin = NSAppUnitsToFloatPixels(itemBounds.x - inner.XMost(), auPerDevPixel);
-      float maxOffset = NSAppUnitsToFloatPixels(outer.XMost() - inner.XMost(), auPerDevPixel);
+      nscoord distance = DistanceToRange(inner.XMost(), outer.XMost());
+      float margin = NSAppUnitsToFloatPixels(itemBounds.x - scrollPort.x - distance, auPerDevPixel);
+      float maxOffset = NSAppUnitsToFloatPixels(std::max(0, outer.XMost() + offset.x), auPerDevPixel);
       left = Some(wr::StickySideConstraint { margin, maxOffset });
     }
     if (outer.x != inner.x) {
-      float margin = NSAppUnitsToFloatPixels(scrollPort.width - itemBounds.XMost() + inner.x, auPerDevPixel);
-      float maxOffset = NSAppUnitsToFloatPixels(outer.x - inner.x, auPerDevPixel);
+      nscoord distance = DistanceToRange(outer.x, inner.x);
+      float margin = NSAppUnitsToFloatPixels(scrollPort.XMost() - itemBounds.XMost() + distance, auPerDevPixel);
+      float maxOffset = NSAppUnitsToFloatPixels(std::min(0, outer.x + offset.x), auPerDevPixel);
       right = Some(wr::StickySideConstraint { margin, maxOffset });
     }
 
+    LayoutDeviceRect bounds = LayoutDeviceRect::FromAppUnits(itemBounds, auPerDevPixel);
     wr::WrStickyId id = aBuilder.DefineStickyFrame(aSc.ToRelativeLayoutRect(bounds),
         top.ptrOr(nullptr), right.ptrOr(nullptr), bottom.ptrOr(nullptr), left.ptrOr(nullptr));
 
     aBuilder.PushStickyFrame(id, GetClipChain());
   }
 
-  // All the things inside this position:sticky item also have the main-thread
-  // translation already applied, so we need to make sure that gets unapplied.
-  // The easiest way to do it is to just create a new stacking context with an
-  // adjusted origin and use that for the nested items. This way all the
-  // ToRelativeLayoutRect calls on this StackingContextHelper object will
-  // include the necessary adjustment.
-  StackingContextHelper sc(aSc, aBuilder);
-  sc.AdjustOrigin(scTranslation);
-
-  nsDisplayOwnLayer::CreateWebRenderCommands(aBuilder, aResources, sc,
+  nsDisplayOwnLayer::CreateWebRenderCommands(aBuilder, aResources, aSc,
       aManager, aDisplayListBuilder);
 
   if (stickyScrollContainer) {
     aBuilder.PopStickyFrame(GetClipChain());
   }
 
   return true;
 }
--- a/layout/reftests/position-sticky/reftest.list
+++ b/layout/reftests/position-sticky/reftest.list
@@ -8,44 +8,44 @@ fuzzy-if(Android,2,1568) == top-6.html t
 == bottom-2a.html bottom-2-ref.html
 == bottom-2b.html bottom-2-ref.html
 == bottom-2c.html bottom-2-ref.html
 == bottom-3.html bottom-3-ref.html
 == bottom-4.html bottom-4-ref.html
 fuzzy-if(Android,2,4) == left-1.html left-1-ref.html
 fuzzy-if(Android,2,4) == left-2.html left-2-ref.html
 == left-3.html left-3-ref.html
-fails-if(webrender) == right-1.html right-1-ref.html
-fuzzy-if(Android,2,4) fails-if(webrender) == right-2.html right-2-ref.html
+== right-1.html right-1-ref.html
+fuzzy-if(Android,2,4) == right-2.html right-2-ref.html
 fuzzy-if(Android,2,4) == right-3.html right-3-ref.html
 == margin-1.html margin-1-ref.html
 == padding-1.html padding-1-ref.html
 == padding-2.html padding-2-ref.html
-fails-if(webrender) == padding-3.html padding-3-ref.html
+== padding-3.html padding-3-ref.html
 == overcontain-1.html overcontain-1-ref.html
 == initial-1.html initial-1-ref.html
 == initial-scroll-1.html initial-scroll-1-ref.html
 == scrollframe-reflow-1.html scrollframe-reflow-1-ref.html
 == scrollframe-reflow-2.html scrollframe-reflow-2-ref.html
 == scrollframe-auto-1.html scrollframe-auto-1-ref.html
 fuzzy-if(Android,2,3) == stacking-context-1.html stacking-context-1-ref.html
 == top-bottom-1.html top-bottom-1-ref.html
 == top-bottom-2.html top-bottom-2-ref.html
 == top-bottom-3.html top-bottom-3-ref.html
 == left-right-1.html left-right-1-ref.html
 == left-right-2.html left-right-2-ref.html
 == left-right-3.html left-right-3-ref.html
-fuzzy-if(Android,4,810) fails-if(webrender) == containing-block-1.html containing-block-1-ref.html
+fuzzy-if(Android,4,810) == containing-block-1.html containing-block-1-ref.html
 == overconstrained-1.html overconstrained-1-ref.html
 == overconstrained-2.html overconstrained-2-ref.html
 == overconstrained-3.html overconstrained-3-ref.html
 == inline-1.html inline-1-ref.html
 == inline-2.html inline-2-ref.html
 fuzzy-if(OSX,99,210) == inline-3.html inline-3-ref.html
-fails-if(webrender) skip-if(!asyncPan) == inline-4.html inline-4-ref.html   # bug 1366295 for webrender
+skip-if(!asyncPan) == inline-4.html inline-4-ref.html
 fails == column-contain-1a.html column-contain-1-ref.html
 == column-contain-1b.html column-contain-1-ref.html
 == column-contain-2.html column-contain-2-ref.html
 == block-in-inline-1.html block-in-inline-1-ref.html
 fuzzy-if(skiaContent,1,22) fuzzy-if(winWidget&&!layersGPUAccelerated,116,1320) fuzzy-if(Android,8,1533) == block-in-inline-2.html block-in-inline-2-ref.html
 fuzzy-if(Android,8,630) fuzzy-if(OSX,1,11) fuzzy-if(skiaContent,1,220) fuzzy-if(winWidget&&!layersGPUAccelerated,116,1320) == block-in-inline-3.html block-in-inline-3-ref.html
 == block-in-inline-continuations.html block-in-inline-continuations-ref.html
 fuzzy-if(winWidget&&!layersGPUAccelerated,140,140) == inner-table-1.html inner-table-1-ref.html