RenderLayer hasVisibleContent() has inconsistent semantics causing disappearing composited layers

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

Attached test case reproduces on both Safari webkit nightly and chromium.

Simon - can you please let me know which of the following semantics we actually want?   I think we want semantics defined by (b).


Problem statement:
(a) In some places in code, RenderLayer::hasVisibleContent() is assumed to only consider the visibility status of RenderObjects that contribute to that specific RenderLayer.
(b) In other places in code, RenderLayer::hasVisibleContent() is assumed to also consider the visibility status of all RenderObjects in all non-composited descendant RenderLayers.

For example:
  RenderLayer::setHasVisibleContent()  -- does not bubble up the hasVisibleContent=true to ancestor layers.
  RenderLayer::updateDescendantDependentFlags() -- the manual tree walk which recomputes m_hasVisibleContent skips any part of the subtree that is a new RenderLayer.
  On the other hand, RenderLayerBacking::hasVisibleNonCompositingDescendantLayers() -- assumes that hasVisibleContent() is true if a (non-composited) RenderLayer descendant has visible content.

Which semantics do we prefer?   That determines the solution we should implement.

Assuming we want semantics defined in (b), I think the correct fix is to modify RenderLayer::setHasVisibleContent() so that it bubbles up hasVisibleContent=true to all ancestor RenderLayers until it reaches a composited RenderLayer.   I've already checked locally that it would fix the attached test case.

[Shawn Singh]

Created attachment 185072 [details]
Reduced test case

[Simon Fraser (smfr)]

(In reply to comment #0)
> Attached test case reproduces on both Safari webkit nightly and chromium.
> 
> Simon - can you please let me know which of the following semantics we actually want?   I think we want semantics defined by (b).
> 
> 
> Problem statement:
> (a) In some places in code, RenderLayer::hasVisibleContent() is assumed to only consider the visibility status of RenderObjects that contribute to that specific RenderLayer.

It's really just about the visibility property, and 'visible content' applies to just this layer, not descendants.

> (b) In other places in code, RenderLayer::hasVisibleContent() is assumed to also consider the visibility status of all RenderObjects in all non-composited descendant RenderLayers.

I think this code is wrong.

> For example:
>   RenderLayer::setHasVisibleContent()  -- does not bubble up the hasVisibleContent=true to ancestor layers.

I think that's OK; hasVisibleDescendant is about child layers. This function does dirty the 'has visible descendant' bit on ancestors.

>   RenderLayer::updateDescendantDependentFlags() -- the manual tree walk which recomputes m_hasVisibleContent skips any part of the subtree that is a new RenderLayer.

What do you mean by "a new RenderLayer"?

>   On the other hand, RenderLayerBacking::hasVisibleNonCompositingDescendantLayers() -- assumes that hasVisibleContent() is true if a (non-composited) RenderLayer descendant has visible content.

I think this is wrong; it should look at the visibleDescendant bit also, but probably can't because that doesn't take compositing into account.

[Shawn Singh]

Created attachment 185321 [details]
Patch

[Shawn Singh]

> >   RenderLayer::updateDescendantDependentFlags() -- the manual tree walk which recomputes m_hasVisibleContent skips any part of the subtree that is a new RenderLayer.
> 
> What do you mean by "a new RenderLayer"?
> 

I just meant that the tree walk only looks at RenderObjects that contribute to the RenderLayer in question, and skips parts of the subtree that are enclosed by a different RenderLayer.

Based on the discussion, this code looks correct as it is;  the patch I just uploaded fixes the RenderLayerBacking helper function.

[Simon Fraser (smfr)]

Comment on attachment 185321 [details]
Patch

I'm not sure I like the "check children then do the recursive deep check" approach; I think just doing the recursive check up front would be OK. It's pretty rare to have visibility:hidden with a visible descendant.

[Shawn Singh]

Created attachment 185341 [details]
Patch

[Shawn Singh]

(In reply to comment #5)
> (From update of attachment 185321 [details])
> I'm not sure I like the "check children then do the recursive deep check" approach; I think just doing the recursive check up front would be OK. It's pretty rare to have visibility:hidden with a visible descendant.

Second patch puts the recursive check up front.  Thanks!

[Simon Fraser (smfr)]

Comment on attachment 185341 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185341&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:1433
> -    // FIXME: We shouldn't be called with a stale z-order lists. See bug 85512.
> -    m_owningLayer->updateLayerListsIfNeeded();
> -
> -#if !ASSERT_DISABLED
> -    LayerListMutationDetector mutationChecker(m_owningLayer);
> -#endif
> -
> -    if (Vector<RenderLayer*>* normalFlowList = m_owningLayer->normalFlowList()) {
> +    // Common case: Check the layer lists for any children that have visible content.
> +    if (Vector<RenderLayer*>* normalFlowList = parent->normalFlowList()) {

I think you should early return based on !hasVisibleDescendant() and remove the hasVisibleDescendant() checks elsewhere. You're missing the hasVisibleDescendant() on the starting layer.

[Shawn Singh]

Created attachment 185381 [details]
moved the parent->hasVisibleDescendant() check

[Shawn Singh]

Hi Simon,  could you please take a look at the most recent patch?   Thanks in advance!

[Simon Fraser (smfr)]

Comment on attachment 185381 [details]
moved the parent->hasVisibleDescendant() check

View in context: https://bugs.webkit.org/attachment.cgi?id=185381&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:1433
> +    if (!parent->hasVisibleDescendant())
> +        return false;

I misread the previous patch and may have mislead you here. Is hasVisibleDescendant() only valid on stacking containers? If so, the previous patch was better.

[Shawn Singh]

(In reply to comment #11)
> (From update of attachment 185381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185381&action=review
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:1433
> > +    if (!parent->hasVisibleDescendant())
> > +        return false;
> 
> I misread the previous patch and may have mislead you here. Is hasVisibleDescendant() only valid on stacking containers? If so, the previous patch was better.

As far as I can tell, the latest patch is still correct:

- RenderLayer::m_hasVisibleDescendant is computed by walking through all children recursively, regardless of which flow list, I think it will have a correct value even for non-stacking-context layers.   

- I did wonder why it was originally only for stacking contexts, and did some archaeology. https://bugs.webkit.org/show_bug.cgi?id=38829 --  probably it was just reasonable that way, while writing that patch, and now the improvement is more apparent to us.

[Shawn Singh]

Any update on review?   Quick summary - I do feel the new placement in code is correct.  I'm happy to change it back, if you would rather use the previous patch, though.

[Simon Fraser (smfr)]

Let's go back to the previous one.

[Shawn Singh]

Created attachment 186732 [details]
Moved early exit back to original location

[WebKit Review Bot]

Comment on attachment 186732 [details]
Moved early exit back to original location

Clearing flags on attachment: 186732

Committed r142012: <http://trac.webkit.org/changeset/142012>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.