Bug 1238564 - When building a fixed/sticky display item, don't restore the clip until we're ready to build that item so that inner items aren't unnecessarily clipped. r=roc a=lizzard
authorMarkus Stange <mstange@themasta.com>
Tue, 23 Feb 2016 18:22:21 +0100
changeset 323484 c18e583d488d94bafef3e0c2e9482929b32cf81e
parent 323483 d7db5f19e4bbd8b2a798e8ef8557afeac9695ff5
child 323485 8c7f3ce9707752ed669d671fd0d6ecb76ee34f85
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc, lizzard
bugs1238564
milestone47.0a2
Bug 1238564 - When building a fixed/sticky display item, don't restore the clip until we're ready to build that item so that inner items aren't unnecessarily clipped. r=roc a=lizzard This isn't really about regular clips, it's about scroll clips. If the inner item has an unnecessary scroll clip (one that's already on the parent), this can cause the inner item's bounds to be larger than necessary because, after the first patch in bug 1238564, it will include the whole bounds of async-scrollable scroll frames. Also, if e.g. the inner item is an opacity item, and it has a scroll clip that's not just the innermost ancestor scroll clip of all of its children, then FrameLayerBuilder's mContainerBounds == mAccumulatedChildBounds assertion can fail if the opacity item gets flattened away, because the child bounds won't have been enlarged for the scroll clip. There must be a better way to do the clip resetting in nsFrame::BuildDisplayListForStackingContext - the code is not pretty at all. But I'd rather get the tests passing first before we figure out how to clean it up. MozReview-Commit-ID: E1DdpN546va
layout/generic/nsFrame.cpp
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -2191,17 +2191,17 @@ nsIFrame::BuildDisplayListForStackingCon
   if (aBuilder->ContainsBlendMode()) {
     DisplayListClipState::AutoSaveRestore blendContainerClipState(aBuilder);
     blendContainerClipState.Clear();
     resultList.AppendNewToTop(
       new (aBuilder) nsDisplayBlendContainer(aBuilder, this, &resultList,
                                              containerItemScrollClip));
   }
 
-  if (!isTransformed) {
+  if (!isTransformed && !useFixedPosition && !useStickyPosition) {
     // Restore saved clip state now so that any display items we create below
     // are clipped properly.
     clipState.Restore();
     if (didResetClip) {
       containerItemScrollClip = aBuilder->ClipState().GetCurrentInnermostScrollClip();
     }
   }
 
@@ -2273,17 +2273,17 @@ nsIFrame::BuildDisplayListForStackingCon
         }
       }
       WrapSeparatorTransform(aBuilder, this, dirtyRect,
                              &nonparticipants, &participants, index++);
       resultList.AppendToTop(&participants);
     }
 
     // Restore clip state now so nsDisplayTransform is clipped properly.
-    if (!HasPerspective()) {
+    if (!HasPerspective() && !useFixedPosition && !useStickyPosition) {
       clipState.Restore();
       if (didResetClip) {
         containerItemScrollClip = aBuilder->ClipState().GetCurrentInnermostScrollClip();
       }
     }
     // Revert to the dirtyrect coming in from the parent, without our transform
     // taken into account.
     buildingDisplayList.SetDirtyRect(dirtyRectOutsideTransform);
@@ -2298,35 +2298,44 @@ nsIFrame::BuildDisplayListForStackingCon
     buildingDisplayList.SetReferenceFrameAndCurrentOffset(outerReferenceFrame,
       GetOffsetToCrossDoc(outerReferenceFrame));
 
     nsDisplayTransform *transformItem =
       new (aBuilder) nsDisplayTransform(aBuilder, this, &resultList, dirtyRect);
     resultList.AppendNewToTop(transformItem);
 
     if (HasPerspective()) {
-      clipState.Restore();
-      if (didResetClip) {
-        containerItemScrollClip = aBuilder->ClipState().GetCurrentInnermostScrollClip();
+      if (!useFixedPosition && !useStickyPosition) {
+        clipState.Restore();
+        if (didResetClip) {
+          containerItemScrollClip = aBuilder->ClipState().GetCurrentInnermostScrollClip();
+        }
       }
       resultList.AppendNewToTop(
         new (aBuilder) nsDisplayPerspective(
           aBuilder, this,
           GetContainingBlock()->GetContent()->GetPrimaryFrame(), &resultList));
     }
 
     /* If we need an opacity item, but didn't do it earlier, add it now on the
      * outside of the transform.
      */
     if (useOpacity && !usingSVGEffects) {
       CreateOpacityItem(aBuilder, this, resultList, opacityItemForEventsOnly,
                         containerItemScrollClip);
     }
   }
 
+  if (useFixedPosition || useStickyPosition) {
+    clipState.Restore();
+    if (didResetClip) {
+      containerItemScrollClip = aBuilder->ClipState().GetCurrentInnermostScrollClip();
+    }
+  }
+
   /* If we have sticky positioning, wrap it in a sticky position item.
    */
   if (useFixedPosition) {
     resultList.AppendNewToTop(
         new (aBuilder) nsDisplayFixedPosition(aBuilder, this, &resultList));
   } else if (useStickyPosition) {
     resultList.AppendNewToTop(
         new (aBuilder) nsDisplayStickyPosition(aBuilder, this, &resultList));