Bug 1458598 - Override scrollframes with their descendant reference frames. r?mstange draft
authorKartikaya Gupta <kgupta@mozilla.com>
Tue, 08 May 2018 09:16:29 -0400
changeset 792514 054250286925451e5ee7ba4b83eb3bd261216b24
parent 792513 f98505abd0d032338df4d1709a08770b4d8e4d1d
push id109118
push userkgupta@mozilla.com
push dateTue, 08 May 2018 13:17:18 +0000
reviewersmstange
bugs1458598
milestone62.0a1
Bug 1458598 - Override scrollframes with their descendant reference frames. r?mstange The test case has a fixed item A inside a scrollframe B which is inside a reference frame C which is inside the root scrollframe D. The ClipManager code currently uses D's scrollid as the scrolling ancestor for A, because the gecko display list's ASR is set up that way. However, we really want to set C as the scrolling ancestor, because otherwise the item A gets hoisted out of C and the transform from C doesn't get applied to it. This patch ensures that when we enter C, we install an override so that anything that would have used D's scrollid ends up using C's, which results in the correct behaviour. MozReview-Commit-ID: 31tscfT4xWW
gfx/layers/wr/ClipManager.cpp
gfx/layers/wr/StackingContextHelper.cpp
layout/reftests/async-scrolling/reftest.list
--- a/gfx/layers/wr/ClipManager.cpp
+++ b/gfx/layers/wr/ClipManager.cpp
@@ -54,17 +54,17 @@ ClipManager::EndBuild()
 }
 
 void
 ClipManager::BeginList(const StackingContextHelper& aStackingContext)
 {
   if (aStackingContext.AffectsClipPositioning()) {
     PushOverrideForASR(
         mItemClipStack.empty() ? nullptr : mItemClipStack.top().mASR,
-        Nothing());
+        aStackingContext.ReferenceFrameId());
   }
 
   ItemClips clips(nullptr, nullptr);
   if (!mItemClipStack.empty()) {
     clips.CopyOutputsFrom(mItemClipStack.top());
   }
   mItemClipStack.push(clips);
 }
@@ -212,17 +212,17 @@ ClipManager::BeginItem(nsDisplayItem* aI
     // If the clip's ASR is different, then we need to set the scroll id
     // explicitly to match the desired ASR.
     FrameMetrics::ViewID viewId = asr
         ? asr->GetViewId()
         : FrameMetrics::NULL_SCROLL_ID;
     Maybe<wr::WrClipId> scrollId =
         mBuilder->GetScrollIdForDefinedScrollLayer(viewId);
     MOZ_ASSERT(scrollId.isSome());
-    clips.mScrollId = scrollId;
+    clips.mScrollId = ClipIdAfterOverride(scrollId);
   } else {
     // If we don't have a clip at all, then we don't want to explicitly push
     // the ASR either, because as with the first clause of this if condition,
     // the item might get hoisted out of a stacking context that was pushed
     // between the |asr| and this |aItem|. Instead we just leave clips.mScrollId
     // empty and things seem to work out.
     // XXX: there might be cases where things don't just "work out", in which
     // case we might need to do something smarter here.
--- a/gfx/layers/wr/StackingContextHelper.cpp
+++ b/gfx/layers/wr/StackingContextHelper.cpp
@@ -68,20 +68,18 @@ StackingContextHelper::StackingContextHe
           aTransformPtr,
           aIsPreserve3D ? wr::TransformStyle::Preserve3D : wr::TransformStyle::Flat,
           aPerspectivePtr,
           wr::ToMixBlendMode(aMixBlendMode),
           aFilters,
           aBackfaceVisible,
           rasterSpace);
 
-  mAffectsClipPositioning =
-      (aTransformPtr && !aTransformPtr->IsIdentity()) ||
-      (aBounds.TopLeft() != LayoutDevicePoint()) ||
-      (aAnimation && aAnimation->effect_type == wr::WrAnimationType::Transform);
+  mAffectsClipPositioning = mReferenceFrameId.isSome() ||
+      (aBounds.TopLeft() != LayoutDevicePoint());
 }
 
 StackingContextHelper::~StackingContextHelper()
 {
   if (mBuilder) {
     mBuilder->PopStackingContext();
   }
 }
--- a/layout/reftests/async-scrolling/reftest.list
+++ b/layout/reftests/async-scrolling/reftest.list
@@ -53,17 +53,17 @@ fuzzy-if(Android,7,4) skip-if(!asyncPan)
 fuzzy-if(Android,7,4) fails-if(webrender) skip-if(!asyncPan) == perspective-scrolling-3.html perspective-scrolling-3-ref.html # bug 1361720 for webrender
 fuzzy-if(Android,7,4) skip-if(!asyncPan) == perspective-scrolling-4.html perspective-scrolling-4-ref.html
 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,20) 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) skip-if(!asyncPan) == fixed-pos-scrolled-clip-2.html fixed-pos-scrolled-clip-2-ref.html
-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 1458598 for webrender
+fuzzy-if(Android,6,8) skip-if(!asyncPan) == fixed-pos-scrolled-clip-3.html fixed-pos-scrolled-clip-3-ref.html
 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
 skip-if(!asyncPan) == position-sticky-bug1434250.html position-sticky-bug1434250-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
 skip-if(!asyncPan) == curtain-effect-1.html curtain-effect-1-ref.html
 
 # for the following tests, we want to disable the low-precision buffer