Bug 1372912 - Place the layer local clip in the right spot for fixed-pos layers. r?jrmuizel,mrobinson draft
authorKartikaya Gupta <kgupta@mozilla.com>
Wed, 14 Jun 2017 12:54:33 -0400
changeset 594234 719c693207cfdb2026a7c7b1a69d114f5e807d25
parent 594233 ca71dc2a2b2d26ab6d10af57e4f146a37c53dbdc
child 633356 84e146c8e95ee8585ecca8e31f28c879e3c50a33
push id63957
push userkgupta@mozilla.com
push dateWed, 14 Jun 2017 18:07:43 +0000
reviewersjrmuizel, mrobinson
bugs1372912
milestone56.0a1
Bug 1372912 - Place the layer local clip in the right spot for fixed-pos layers. r?jrmuizel,mrobinson This patch makes the bg-fixed-child-mask.html reftest pass by ensuring the clips for the test case are all in the correct spots. See the comments in the patch for a more detailed explanation. MozReview-Commit-ID: 4ILJ3P8hEkb
gfx/layers/wr/ScrollingLayersHelper.cpp
gfx/layers/wr/ScrollingLayersHelper.h
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@@ -25,28 +25,46 @@ ScrollingLayersHelper::ScrollingLayersHe
   if (!mLayer->WrManager()->AsyncPanZoomEnabled()) {
     // If APZ is disabled then we don't need to push the scrolling clips. We
     // still want to push the layer's local clip though.
     PushLayerLocalClip(aStackingContext);
     return;
   }
 
   Layer* layer = mLayer->GetLayer();
+  Maybe<wr::WrClipId> fixedClipId;
   for (uint32_t i = layer->GetScrollMetadataCount(); i > 0; i--) {
     const ScrollMetadata& metadata = layer->GetScrollMetadata(i - 1);
     // The scroll clip on a given metadata is affected by all async transforms
     // from metadatas "above" it, but not the async transform on the metadata
     // itself. Therefore we need to push this clip before we push the
     // corresponding scroll layer, so that when we set an async scroll position
     // on the scroll layer, the clip isn't affected by it.
     if (const Maybe<LayerClip>& clip = metadata.GetScrollClip()) {
       PushLayerClip(clip.ref(), aStackingContext);
     }
 
     const FrameMetrics& fm = layer->GetFrameMetrics(i - 1);
+    if (layer->GetIsFixedPosition() &&
+        layer->GetFixedPositionScrollContainerId() == fm.GetScrollId()) {
+      // If the layer contents are fixed for this metadata onwards, we need
+      // to insert the layer's local clip at this point in the clip tree,
+      // as a child of whatever's on the stack. However, we really want it
+      // to create a branch in the tree, and then use this branch later when
+      // pushing the layer's contents. That's why we do a push/pop of the clip
+      // at this point, and save the id of the resulting branch into
+      // |fixedClipId|. See the comments below where fixedClipId is used for
+      // more information.
+      if (PushLayerLocalClip(aStackingContext)) {
+        fixedClipId = mBuilder->TopmostClipId();
+        MOZ_ASSERT(fixedClipId.isSome());
+        mBuilder->PopClip(); // for the above PushLayerLocalClip
+      }
+    }
+
     if (!fm.IsScrollable()) {
       continue;
     }
     LayerRect contentRect = ViewAs<LayerPixel>(
         fm.GetExpandedScrollableRect() * fm.GetDevPixelsPerCSSPixel(),
         PixelCastJustification::WebRenderHasUnitResolution);
     // TODO: check coordinate systems are sane here
     LayerRect clipBounds = ViewAs<LayerPixel>(
@@ -73,33 +91,53 @@ ScrollingLayersHelper::ScrollingLayersHe
     PushLayerClip(scrolledClip.ref(), aStackingContext);
   }
 
   // If the layer is marked as fixed-position, it is fixed relative to something
   // (the scroll layer referred to by GetFixedPositionScrollContainerId, hereafter
   // referred to as the "scroll container"). What this really means is that we
   // don't want this content to scroll with any scroll layer on the stack up to
   // and including the scroll container, but we do want it to scroll with any
-  // ancestor scroll layers. So we do a PushClipAndScrollInfo that maintains
+  // ancestor scroll layers.
+  // Also, the local clip on the layer (defined by layer->GetClipRect() and
+  // layer->GetMaskLayer()) also need to be fixed relative to the scroll
+  // container. This is why we inserted it into the clip tree during the
+  // loop above when we encountered the scroll container.
+  // At this point we do a PushClipAndScrollInfo that maintains
   // the current non-scrolling clip stack, but resets the scrolling clip stack
-  // to the ancestor of the scroll container.
+  // to the branch that we created, or the scroll container if there was no
+  // local clip at all (which can be thought of as a "trivial branch").
   if (layer->GetIsFixedPosition()) {
-    FrameMetrics::ViewID fixedFor = layer->GetFixedPositionScrollContainerId();
-    Maybe<FrameMetrics::ViewID> scrollsWith = mBuilder->ParentScrollIdFor(fixedFor);
     Maybe<wr::WrClipId> clipId = mBuilder->TopmostClipId();
-    // Default to 0 if there is no ancestor, because 0 refers to the root scrollframe.
-    mBuilder->PushClipAndScrollInfo(
-        wr::DisplayListBuilder::ScrollNodeId(scrollsWith.valueOr(0)),
-        clipId.ptrOr(nullptr));
+    if (fixedClipId) {
+      mBuilder->PushClipAndScrollInfo(
+          wr::DisplayListBuilder::ScrollNodeId(fixedClipId.value()),
+          clipId.ptrOr(nullptr));
+    } else {
+      // If we enter here, it might be an unhandled case where the "fixedFor"
+      // id points to a scroll id that lives on some other layer. If that happens
+      // we might need a better way to handle this. (The problem is that we weren't
+      // able to create a "branch" using the local clip rect). For now just fall
+      // back and use the scroll container, which effectively discards the layer's
+      // local clip.
+      NS_WARNING("Possibly ignoring a valid layer local clip on a fixed-pos layer");
+      FrameMetrics::ViewID fixedFor = layer->GetFixedPositionScrollContainerId();
+      Maybe<FrameMetrics::ViewID> scrollsWith = mBuilder->ParentScrollIdFor(fixedFor);
+      // Default to 0 if there is no ancestor, because 0 refers to the root scrollframe.
+      mBuilder->PushClipAndScrollInfo(
+          wr::DisplayListBuilder::ScrollNodeId(scrollsWith.valueOr(0)),
+          clipId.ptrOr(nullptr));
+    }
+  } else {
+    MOZ_ASSERT(!fixedClipId);
+    PushLayerLocalClip(aStackingContext);
   }
-
-  PushLayerLocalClip(aStackingContext);
 }
 
-void
+bool
 ScrollingLayersHelper::PushLayerLocalClip(const StackingContextHelper& aStackingContext)
 {
   Layer* layer = mLayer->GetLayer();
   Maybe<ParentLayerRect> clip;
   if (const Maybe<ParentLayerIntRect>& rect = layer->GetClipRect()) {
     clip = Some(IntRectToRect(rect.ref()));
   } else if (layer->GetMaskLayer()) {
     // this layer has a mask, but no clip rect. so let's use the transformed
@@ -108,16 +146,17 @@ ScrollingLayersHelper::PushLayerLocalCli
   }
   if (clip) {
     Maybe<WrImageMask> mask = mLayer->BuildWrMaskLayer(aStackingContext);
     LayerRect clipRect = ViewAs<LayerPixel>(clip.ref(),
         PixelCastJustification::MovingDownToChildren);
     mBuilder->PushClip(aStackingContext.ToRelativeWrRect(clipRect), mask.ptrOr(nullptr));
     mPushedLayerLocalClip = true;
   }
+  return mPushedLayerLocalClip;
 }
 
 void
 ScrollingLayersHelper::PushLayerClip(const LayerClip& aClip,
                                      const StackingContextHelper& aSc)
 {
   LayerRect clipRect = IntRectToRect(ViewAs<LayerPixel>(aClip.GetClipRect(),
         PixelCastJustification::MovingDownToChildren));
@@ -136,21 +175,20 @@ ScrollingLayersHelper::~ScrollingLayersH
   Layer* layer = mLayer->GetLayer();
   if (!mLayer->WrManager()->AsyncPanZoomEnabled()) {
     if (mPushedLayerLocalClip) {
       mBuilder->PopClip();
     }
     return;
   }
 
-  if (mPushedLayerLocalClip) {
-    mBuilder->PopClip();
-  }
   if (layer->GetIsFixedPosition()) {
     mBuilder->PopClipAndScrollInfo();
+  } else if (mPushedLayerLocalClip) {
+    mBuilder->PopClip();
   }
   if (layer->GetScrolledClip()) {
     mBuilder->PopClip();
   }
   for (uint32_t i = 0; i < layer->GetScrollMetadataCount(); i++) {
     const FrameMetrics& fm = layer->GetFrameMetrics(i);
     if (fm.IsScrollable()) {
       mBuilder->PopScrollLayer();
--- a/gfx/layers/wr/ScrollingLayersHelper.h
+++ b/gfx/layers/wr/ScrollingLayersHelper.h
@@ -24,17 +24,17 @@ class MOZ_RAII ScrollingLayersHelper
 {
 public:
   ScrollingLayersHelper(WebRenderLayer* aLayer,
                         wr::DisplayListBuilder& aBuilder,
                         const StackingContextHelper& aSc);
   ~ScrollingLayersHelper();
 
 private:
-  void PushLayerLocalClip(const StackingContextHelper& aStackingContext);
+  bool PushLayerLocalClip(const StackingContextHelper& aStackingContext);
   void PushLayerClip(const LayerClip& aClip,
                      const StackingContextHelper& aSc);
 
   WebRenderLayer* mLayer;
   wr::DisplayListBuilder* mBuilder;
   bool mPushedLayerLocalClip;
 };