Bug 1411249 - Handle yet more clipping cases. r?mstange draft
authorKartikaya Gupta <kgupta@mozilla.com>
Fri, 27 Oct 2017 13:22:16 -0400
changeset 687749 a948521b4701f2c2591e0cf8e59b30cdd41c4ebb
parent 687700 7eb18fd26eaf0b1e120da801b7353a08d91fa877
child 737728 5cb7cdcbfb075ef1326eb24aa7c500b35f8d5681
push id86590
push userkgupta@mozilla.com
push dateFri, 27 Oct 2017 17:27:36 +0000
reviewersmstange
bugs1411249, 1373802
milestone58.0a1
Bug 1411249 - Handle yet more clipping cases. r?mstange This extends the fix in bug 1373802 to account for extra levels of display item nesting. If those extra intermediate display items don't push any clips we still want to pick up the ClipAndScroll from the enclosing ancestor that has it. MozReview-Commit-ID: AmxRz4fBKnX
gfx/layers/wr/ScrollingLayersHelper.cpp
gfx/layers/wr/ScrollingLayersHelper.h
layout/reftests/async-scrolling/fixed-pos-scrolled-clip-5-ref.html
layout/reftests/async-scrolling/fixed-pos-scrolled-clip-5.html
layout/reftests/async-scrolling/reftest.list
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@@ -113,24 +113,25 @@ ScrollingLayersHelper::BeginItem(nsDispl
   // If the leafmost ASR is not the same as the item's ASR then we are dealing
   // with a case where the item's clip chain is scrolled by something other than
   // the item's ASR. So for those cases we need to use the ClipAndScroll API.
   bool needClipAndScroll = (leafmostId != scrollId);
 
   // The other scenario where we need to push a ClipAndScroll is when we are
   // in a nested display item where the enclosing item pushed a ClipAndScroll,
   // and our clip chain extends from that item's clip chain. To check this we
-  // want to make sure that (a) we are InsideClipAndScroll(), and (b) nothing
+  // want to make sure that (a) we are inside a ClipAndScroll, and (b) nothing
   // else was pushed onto mBuilder's stack since that ClipAndScroll.
   if (!needClipAndScroll &&
-      InsideClipAndScroll() &&
       mBuilder->TopmostScrollId() == scrollId &&
       !mBuilder->TopmostIsClip()) {
-    MOZ_ASSERT(mItemClipStack.back().mClipAndScroll->first == scrollId);
-    needClipAndScroll = true;
+    if (auto cs = EnclosingClipAndScroll()) {
+      MOZ_ASSERT(cs->first == scrollId);
+      needClipAndScroll = true;
+    }
   }
 
   // If we don't need a ClipAndScroll, ensure the item's ASR is at the top of
   // the scroll stack
   if (!needClipAndScroll && mBuilder->TopmostScrollId() != scrollId) {
     MOZ_ASSERT(leafmostId == scrollId); // because !needClipAndScroll
     clips.mScrollId = Some(scrollId);
   }
@@ -287,30 +288,30 @@ ScrollingLayersHelper::RecurseAndDefineC
         // pointer will be null for the nested item but the containing wrapper's
         // clip will be on the stack already and we can pick it up from there.
         // Another way of thinking about this is that if the clip chain were
         // "fully completed" then aChain->mParent wouldn't be null but would point
         // to the clip corresponding to mBuilder->TopmostClipId(), and we would
         // have gone into the |aChain->mParent->mASR == aAsr| branch above.
         ancestorIds.first = Nothing();
         ancestorIds.second = mBuilder->TopmostClipId();
-      } else if (InsideClipAndScroll()) {
+      } else if (auto cs = EnclosingClipAndScroll()) {
         // If aChain->mASR is already the topmost scroll layer on the stack, but
         // it was pushed as part of a "clip and scroll" entry (i.e. because an
         // item had a clip scrolled by a different ASR than the item itself),
         // then we have need to propagate that behaviour as well. For example if
         // the enclosing display item pushed a ClipAndScroll with (scrollid=S,
         // clipid=C), then then clip we're defining here (call it D) needs to be
         // defined as a child of C, and we'll need to push the ClipAndScroll
         // (S, D) for this item. This hunk of code ensures that we define D
         // as a child of C, and when we set the needClipAndScroll flag elsewhere
         // in this file we make sure to set it for this scenario.
-        MOZ_ASSERT(mItemClipStack.back().mClipAndScroll->first == scrollId);
+        MOZ_ASSERT(cs->first == scrollId);
         ancestorIds.first = Nothing();
-        ancestorIds.second = mItemClipStack.back().mClipAndScroll->second;
+        ancestorIds.second = cs->second;
       }
     }
   }
   // At most one of the ancestor pair should be defined here, and the one that
   // is defined will be the parent clip for the new clip that we're defining.
   MOZ_ASSERT(!(ancestorIds.first && ancestorIds.second));
 
   LayoutDeviceRect clip = LayoutDeviceRect::FromAppUnits(
@@ -440,20 +441,31 @@ ScrollingLayersHelper::RecurseAndDefineA
   mBuilder->DefineScrollLayer(scrollId, ancestorIds.first, ancestorIds.second,
       aSc.ToRelativeLayoutRect(contentRect),
       aSc.ToRelativeLayoutRect(clipBounds));
 
   ids.first = Some(scrollId);
   return ids;
 }
 
-bool
-ScrollingLayersHelper::InsideClipAndScroll() const
+Maybe<ScrollingLayersHelper::ClipAndScroll>
+ScrollingLayersHelper::EnclosingClipAndScroll() const
 {
-  return !mItemClipStack.empty() && mItemClipStack.back().mClipAndScroll.isSome();
+  for (auto it = mItemClipStack.rbegin(); it != mItemClipStack.rend(); it++) {
+    if (it->mClipAndScroll) {
+      return it->mClipAndScroll;
+    }
+    // If an entry in the stack pushed a single clip or scroll without pushing
+    // a mClipAndScroll, we abort because we are effectively no longer inside
+    // a ClipAndScroll
+    if (it->mClipId || it->mScrollId) {
+      break;
+    }
+  }
+  return Nothing();
 }
 
 ScrollingLayersHelper::~ScrollingLayersHelper()
 {
   MOZ_ASSERT(!mBuilder);
   MOZ_ASSERT(mCache.empty());
   MOZ_ASSERT(mItemClipStack.empty());
 }
--- a/gfx/layers/wr/ScrollingLayersHelper.h
+++ b/gfx/layers/wr/ScrollingLayersHelper.h
@@ -39,16 +39,18 @@ public:
   void BeginList();
   void EndList();
 
   void BeginItem(nsDisplayItem* aItem,
                  const StackingContextHelper& aStackingContext);
   ~ScrollingLayersHelper();
 
 private:
+  typedef std::pair<FrameMetrics::ViewID, Maybe<wr::WrClipId>> ClipAndScroll;
+
   std::pair<Maybe<FrameMetrics::ViewID>, Maybe<wr::WrClipId>>
   DefineClipChain(nsDisplayItem* aItem,
                   const ActiveScrolledRoot* aAsr,
                   const DisplayItemClipChain* aChain,
                   int32_t aAppUnitsPerDevPixel,
                   const StackingContextHelper& aStackingContext);
 
   std::pair<Maybe<FrameMetrics::ViewID>, Maybe<wr::WrClipId>>
@@ -60,17 +62,17 @@ private:
 
   std::pair<Maybe<FrameMetrics::ViewID>, Maybe<wr::WrClipId>>
   RecurseAndDefineAsr(nsDisplayItem* aItem,
                       const ActiveScrolledRoot* aAsr,
                       const DisplayItemClipChain* aChain,
                       int32_t aAppUnitsPerDevPixel,
                       const StackingContextHelper& aSc);
 
-  bool InsideClipAndScroll() const;
+  Maybe<ClipAndScroll> EnclosingClipAndScroll() const;
 
   // Note: two DisplayItemClipChain* A and B might actually be "equal" (as per
   // DisplayItemClipChain::Equal(A, B)) even though they are not the same pointer
   // (A != B). In this hopefully-rare case, they will get separate entries
   // in this map when in fact we could collapse them. However, to collapse
   // them involves writing a custom hash function for the pointer type such that
   // A and B hash to the same things whenever DisplayItemClipChain::Equal(A, B)
   // is true, and that will incur a performance penalty for all the hashmap
@@ -87,17 +89,17 @@ private:
     ItemClips(const ActiveScrolledRoot* aAsr,
               const DisplayItemClipChain* aChain);
 
     const ActiveScrolledRoot* mAsr;
     const DisplayItemClipChain* mChain;
 
     Maybe<FrameMetrics::ViewID> mScrollId;
     Maybe<wr::WrClipId> mClipId;
-    Maybe<std::pair<FrameMetrics::ViewID, Maybe<wr::WrClipId>>> mClipAndScroll;
+    Maybe<ClipAndScroll> mClipAndScroll;
 
     void Apply(wr::DisplayListBuilder* aBuilder);
     void Unapply(wr::DisplayListBuilder* aBuilder);
     bool HasSameInputs(const ItemClips& aOther);
   };
 
   std::vector<ItemClips> mItemClipStack;
 };
new file mode 100644
--- /dev/null
+++ b/layout/reftests/async-scrolling/fixed-pos-scrolled-clip-5-ref.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html lang="en-US"><head>
+<meta http-equiv="content-type" content="text/html; charset=UTF-8"><title>The scrolled clip on the fixed element is not respected</title>
+
+</head><body><div style="
+    position: absolute;
+    top: 0;
+    left: 0;
+    background-color: lime;
+    width: 100%;
+    height: 200px;">
+</div>
+</body></html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/async-scrolling/fixed-pos-scrolled-clip-5.html
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html lang="en-US"><head>
+<meta http-equiv="content-type" content="text/html; charset=UTF-8"><title>The scrolled clip on the fixed element is not respected</title>
+
+</head><body><div style="
+    position: absolute;
+    top: 0;
+    left: 0;
+    clip: rect(auto auto auto auto);
+    width: 100%;
+    height: 200px;">
+
+    <div style="background-image: linear-gradient(lime, lime);
+        position: fixed;
+        top: 0;
+        left: 0;
+        height: 100%;
+        width: 100%;
+        transform: translateZ(0px)">
+    </div>
+
+</div>
+</body></html>
--- a/layout/reftests/async-scrolling/reftest.list
+++ b/layout/reftests/async-scrolling/reftest.list
@@ -49,16 +49,17 @@ fuzzy-if(Android,7,4) skip-if(!asyncPan)
 pref(apz.disable_for_scroll_linked_effects,true) skip-if(!asyncPan) == disable-apz-for-sle-pages.html disable-apz-for-sle-pages-ref.html
 fuzzy-if(browserIsRemote&&d2d,1,19) skip-if(!asyncPan) == background-blend-mode-1.html background-blend-mode-1-ref.html
 skip-if(Android||!asyncPan) != opaque-fractional-displayport-1.html about:blank
 skip-if(Android||!asyncPan) != opaque-fractional-displayport-2.html about:blank
 fuzzy-if(Android,6,4) skip-if(!asyncPan) == fixed-pos-scrolled-clip-1.html fixed-pos-scrolled-clip-1-ref.html
 fuzzy-if(Android,6,8) fails-if(webrender) skip-if(!asyncPan) == fixed-pos-scrolled-clip-2.html fixed-pos-scrolled-clip-2-ref.html   # bug 1377187 for webrender
 fuzzy-if(Android,6,8) fails-if(webrender) skip-if(!asyncPan) == fixed-pos-scrolled-clip-3.html fixed-pos-scrolled-clip-3-ref.html   # bug 1377187 for webrender
 fuzzy-if(Android,6,8) skip-if(!asyncPan) == fixed-pos-scrolled-clip-4.html fixed-pos-scrolled-clip-4-ref.html
+skip-if(!asyncPan) == fixed-pos-scrolled-clip-5.html fixed-pos-scrolled-clip-5-ref.html
 fuzzy-if(Android,6,4) skip-if(!asyncPan) == position-sticky-scrolled-clip-1.html position-sticky-scrolled-clip-1-ref.html
 fuzzy-if(Android,6,4) skip == position-sticky-scrolled-clip-2.html position-sticky-scrolled-clip-2-ref.html # bug ?????? - incorrectly applying clip to sticky contents
 
 # for the following tests, we want to disable the low-precision buffer
 # as it will expand the displayport beyond what the test specifies in
 # its reftest-displayport attributes, and interfere with where we expect
 # checkerboarding to occur
 default-preferences pref(layers.low-precision-buffer,false)