Bug 1590550 - Don't do the "simple display list" optimization when we have overflow clips. r=mattwoodrow
☠☠ backed out by c85ae1289d33 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 04 Nov 2019 19:30:24 +0000
changeset 500439 e75d9dbb4853462d60ad18a589b67e379725c41f
parent 500438 e3f92a06d6adbf2abcecf8cf1a0e4e1f8909bdab
child 500440 6ccc66eeb3326131775768cbd3d4b9216b85b151
push id114164
push useraiakab@mozilla.com
push dateTue, 05 Nov 2019 10:06:15 +0000
treeherdermozilla-inbound@4d585c7edc76 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow
bugs1590550
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1590550 - Don't do the "simple display list" optimization when we have overflow clips. r=mattwoodrow The previous code tried to do it, but it did it wrongly, as the overflow clip comes from the parent, not the child. Thus when we change a style that influences it, we weren't invalidating the SIMPLE_DISPLAY_LIST bit, and such. Make the reftest that caught this fail more reliable. Differential Revision: https://phabricator.services.mozilla.com/D51683
layout/generic/nsFrame.cpp
testing/web-platform/tests/css/css-overflow/dynamic-visible-to-clip-001.html
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -2610,28 +2610,27 @@ Maybe<nsRect> nsIFrame::GetClipPropClipR
 /**
  * If the CSS 'overflow' property applies to this frame, and is not
  * handled by constructing a dedicated nsHTML/XULScrollFrame, set up clipping
  * for that overflow in aBuilder->ClipState() to clip all containing-block
  * descendants.
  *
  * Return true if clipping was applied.
  */
-static bool ApplyOverflowClipping(
+static void ApplyOverflowClipping(
     nsDisplayListBuilder* aBuilder, const nsIFrame* aFrame,
     const nsStyleDisplay* aDisp,
     DisplayListClipState::AutoClipMultiple& aClipState) {
   // Only -moz-hidden-unscrollable is handled here (and 'hidden' for table
   // frames, and any non-visible value for blocks in a paginated context).
   // We allow -moz-hidden-unscrollable to apply to any kind of frame. This
   // is required by comboboxes which make their display text (an inline frame)
   // have clipping.
-  if (!nsFrame::ShouldApplyOverflowClipping(aFrame, aDisp)) {
-    return false;
-  }
+  MOZ_ASSERT(nsFrame::ShouldApplyOverflowClipping(aFrame, aDisp));
+
   nsRect clipRect;
   bool haveRadii = false;
   nscoord radii[8];
   auto* disp = aFrame->StyleDisplay();
   // Only deflate the padding if we clip to the content-box in that axis.
   auto wm = aFrame->GetWritingMode();
   bool cbH = (wm.IsVertical() ? disp->mOverflowClipBoxBlock
                               : disp->mOverflowClipBoxInline) ==
@@ -2650,17 +2649,16 @@ static bool ApplyOverflowClipping(
   bp += aFrame->GetUsedBorder();
   bp.ApplySkipSides(aFrame->GetSkipSides());
   nsRect rect(nsPoint(0, 0), aFrame->GetSize());
   rect.Deflate(bp);
   clipRect = rect + aBuilder->ToReferenceFrame(aFrame);
   haveRadii = aFrame->GetBoxBorderRadii(radii, bp, false);
   aClipState.ClipContainingBlockDescendantsExtra(clipRect,
                                                  haveRadii ? radii : nullptr);
-  return true;
 }
 
 #ifdef DEBUG
 static void PaintDebugBorder(nsIFrame* aFrame, DrawTarget* aDrawTarget,
                              const nsRect& aDirtyRect, nsPoint aPt) {
   nsRect r(aPt, aFrame->GetSize());
   int32_t appUnitsPerDevPixel = aFrame->PresContext()->AppUnitsPerDevPixel();
   Color blueOrRed(aFrame->HasView() ? Color(0.f, 0.f, 1.f, 1.f)
@@ -3897,23 +3895,33 @@ void nsIFrame::BuildDisplayListForChild(
                                         uint32_t aFlags) {
   AutoCheckBuilder check(aBuilder);
 
   if (ShouldSkipFrame(aBuilder, aChild)) {
     return;
   }
 
   nsIFrame* child = aChild;
+  const nsStyleDisplay* ourDisp = StyleDisplay();
+
+  nsIFrame* parent = child->GetParent();
+  const nsStyleDisplay* parentDisp =
+      parent == this ? ourDisp : parent->StyleDisplay();
+  const bool shouldApplyOverflowClip =
+      nsFrame::ShouldApplyOverflowClipping(parent, parentDisp);
 
   const bool isPaintingToWindow = aBuilder->IsPaintingToWindow();
   const bool doingShortcut =
       isPaintingToWindow &&
       (child->GetStateBits() & NS_FRAME_SIMPLE_DISPLAYLIST) &&
       // Animations may change the stacking context state.
-      !(child->MayHaveTransformAnimation() || child->MayHaveOpacityAnimation());
+      // ShouldApplyOverflowClipping is affected by the parent style, which does
+      // not invalidate the NS_FRAME_SIMPLE_DISPLAYLIST bit.
+      !(shouldApplyOverflowClip || child->MayHaveTransformAnimation() ||
+       child->MayHaveOpacityAnimation());
 
   if (aBuilder->IsForPainting()) {
     aBuilder->ClearWillChangeBudgetStatus(child);
   }
 
   if (StaticPrefs::layout_css_scroll_anchoring_highlight()) {
     if (child->FirstContinuation()->IsScrollAnchor()) {
       nsRect bounds = child->GetContentRectRelativeToSelf() +
@@ -4007,18 +4015,16 @@ void nsIFrame::BuildDisplayListForChild(
 
   if (!pseudoStackingContext && !isSVG && (aFlags & DISPLAY_CHILD_INLINE) &&
       !child->IsFrameOfType(eLineParticipant)) {
     // child is a non-inline frame in an inline context, i.e.,
     // it acts like inline-block or inline-table. Therefore it is a
     // pseudo-stacking-context.
     pseudoStackingContext = true;
   }
-
-  const nsStyleDisplay* ourDisp = StyleDisplay();
   // REVIEW: Taken from nsBoxFrame::Paint
   // Don't paint our children if the theme object is a leaf.
   if (IsThemed(ourDisp) &&
       !PresContext()->GetTheme()->WidgetIsContainer(ourDisp->mAppearance))
     return;
 
   // Since we're now sure that we're adding this frame to the display list
   // (which means we're painting it, modulo occlusion), mark it as visible
@@ -4085,21 +4091,22 @@ void nsIFrame::BuildDisplayListForChild(
   // Setup clipping for the parent's overflow:-moz-hidden-unscrollable,
   // or overflow:hidden on elements that don't support scrolling (and therefore
   // don't create nsHTML/XULScrollFrame). This clipping needs to not clip
   // anything directly rendered by the parent, only the rendering of its
   // children.
   // Don't use overflowClip to restrict the dirty rect, since some of the
   // descendants may not be clipped by it. Even if we end up with unnecessary
   // display items, they'll be pruned during ComputeVisibility.
-  nsIFrame* parent = child->GetParent();
-  const nsStyleDisplay* parentDisp =
-      parent == this ? ourDisp : parent->StyleDisplay();
-  if (ApplyOverflowClipping(aBuilder, parent, parentDisp, clipState)) {
-    awayFromCommonPath = true;
+  //
+  // FIXME(emilio): Why can't we handle this more similarly to `clip` (on the
+  // parent, rather than on the children)? Would ClipContentDescendants do what
+  // we want?
+  if (shouldApplyOverflowClip) {
+    ApplyOverflowClipping(aBuilder, parent, parentDisp, clipState);
   }
 
   nsDisplayList list;
   nsDisplayList extraPositionedDescendants;
   const ActiveScrolledRoot* wrapListASR;
   bool builtContainerItem = false;
   if (isStackingContext) {
     // True stacking context.
--- a/testing/web-platform/tests/css/css-overflow/dynamic-visible-to-clip-001.html
+++ b/testing/web-platform/tests/css/css-overflow/dynamic-visible-to-clip-001.html
@@ -1,9 +1,10 @@
 <!doctype html>
+<html class="reftest-wait">
 <meta charset="utf-8">
 <title>Overflow areas are updated when dynamically changed to overflow: clip</title>
 <link rel="help" href="https://drafts.csswg.org/css-overflow/#valdef-overflow-clip">
 <link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
 <link rel="author" title="Mozilla" href="https://mozilla.org">
 <link rel="match" href="clip-001-ref.html">
 <style>
   #target {
@@ -25,12 +26,15 @@
   <div id="target">
     <div id="fill"></div>
   </div>
 </div>
 <script>
 onload = function() {
   let target = document.getElementById("target");
   window.unused = target.getBoundingClientRect(); // Update layout
-  target.style.overflow = "-moz-hidden-unscrollable";
-  target.style.overflow = "clip";
+  requestAnimationFrame(() => requestAnimationFrame(() => {
+    target.style.overflow = "-moz-hidden-unscrollable";
+    target.style.overflow = "clip";
+    document.documentElement.removeAttribute("class");
+  }));
 }
 </script>