From 0d3b6b945339d39de2043d7d0ccbbf5e3b823548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 5 Nov 2019 20:55:40 +0000 Subject: [PATCH] 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/D51805 --HG-- extra : moz-landing-system : lando --- layout/generic/nsFrame.cpp | 55 +++++++++++-------- .../dynamic-visible-to-clip-001.html | 8 ++- 2 files changed, 37 insertions(+), 26 deletions(-) diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp index f6b4c1bff960..f33541e0622b 100644 --- a/layout/generic/nsFrame.cpp +++ b/layout/generic/nsFrame.cpp @@ -2615,18 +2615,17 @@ Maybe nsIFrame::GetClipPropClipRect(const nsStyleDisplay* aDisp, * * 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, aFrame->StyleDisplay())); + nsRect clipRect; bool haveRadii = false; nscoord radii[8]; @@ -2655,7 +2654,6 @@ static bool ApplyOverflowClipping( haveRadii = aFrame->GetBoxBorderRadii(radii, bp, false); aClipState.ClipContainingBlockDescendantsExtra(clipRect, haveRadii ? radii : nullptr); - return true; } #ifdef DEBUG @@ -3902,13 +3900,25 @@ void nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder* aBuilder, } nsIFrame* child = aChild; + auto* placeholder = child->IsPlaceholderFrame() + ? static_cast(child) + : nullptr; + nsIFrame* childOrOutOfFlow = + placeholder ? placeholder->GetOutOfFlowFrame() : child; + + nsIFrame* parent = childOrOutOfFlow->GetParent(); + const bool shouldApplyOverflowClip = + nsFrame::ShouldApplyOverflowClipping(parent, parent->StyleDisplay()); 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); @@ -3939,9 +3949,7 @@ void nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder* aBuilder, nsRect dirty = aBuilder->GetDirtyRect() - offset; nsDisplayListBuilder::OutOfFlowDisplayData* savedOutOfFlowData = nullptr; - const bool isPlaceholder = child->IsPlaceholderFrame(); - if (isPlaceholder) { - nsPlaceholderFrame* placeholder = static_cast(child); + if (placeholder) { if (placeholder->GetStateBits() & PLACEHOLDER_FOR_TOPLAYER) { // If the out-of-flow frame is in the top layer, the viewport frame // will paint it. Skip it here. Note that, only out-of-flow frames @@ -3951,10 +3959,8 @@ void nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder* aBuilder, return; } - child = placeholder->GetOutOfFlowFrame(); - NS_ASSERTION(child, "No out of flow frame?"); - - if (child && aBuilder->IsForPainting()) { + child = childOrOutOfFlow; + if (aBuilder->IsForPainting()) { aBuilder->ClearWillChangeBudgetStatus(child); } @@ -3965,12 +3971,11 @@ void nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder* aBuilder, static const nsFrameState skipFlags = (NS_FRAME_IS_PUSHED_FLOAT | NS_FRAME_TOO_DEEP_IN_FRAME_TREE | NS_FRAME_IS_NONDISPLAY); - if (!child || (child->GetStateBits() & skipFlags) || - nsLayoutUtils::IsPopup(child)) { + if (child->HasAnyStateBits(skipFlags) || nsLayoutUtils::IsPopup(child)) { return; } - MOZ_ASSERT(child->GetStateBits() & NS_FRAME_OUT_OF_FLOW); + MOZ_ASSERT(child->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW)); savedOutOfFlowData = nsDisplayListBuilder::GetOutOfFlowData(child); if (aBuilder->GetIncludeAllOutOfFlows()) { @@ -4043,7 +4048,7 @@ void nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder* aBuilder, child->IsStackingContext(disp, pos, effects, isPositioned); if (pseudoStackingContext || isStackingContext || isPositioned || - isPlaceholder || (!isSVG && disp->IsFloating(child)) || + placeholder || (!isSVG && disp->IsFloating(child)) || (isSVG && effects->mClip.IsRect() && IsSVGContentWithCSSClip(child))) { pseudoStackingContext = true; awayFromCommonPath = true; @@ -4067,8 +4072,8 @@ void nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder* aBuilder, savedOutOfFlowData->mContainingBlockActiveScrolledRoot); MOZ_ASSERT(awayFromCommonPath, "It is impossible when savedOutOfFlowData is true"); - } else if (GetStateBits() & NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO && - isPlaceholder) { + } else if (HasAnyStateBits(NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO) && + placeholder) { NS_ASSERTION(visible.IsEmpty(), "should have empty visible rect"); // Every item we build from now until we descent into an out of flow that // does have saved out of flow data should be invisible. This state gets @@ -4090,10 +4095,12 @@ void nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder* aBuilder, // 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)) { + // + // 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, clipState); awayFromCommonPath = true; } diff --git 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 index 08114d73b8bf..b97701bfb299 100644 --- 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,4 +1,5 @@ + Overflow areas are updated when dynamically changed to overflow: clip @@ -30,7 +31,10 @@ 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"); + })); }