Bug 108118 - RenderLayer hasVisibleContent() has inconsistent semantics causing disappearing composited layers
Summary: RenderLayer hasVisibleContent() has inconsistent semantics causing disappeari...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-28 14:50 PST by Shawn Singh
Modified: 2013-02-06 11:37 PST (History)
7 users (show)

See Also:


Attachments
Reduced test case (1.52 KB, text/html)
2013-01-28 14:53 PST, Shawn Singh
no flags Details
Patch (10.65 KB, patch)
2013-01-29 15:30 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2013-01-29 16:40 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
moved the parent->hasVisibleDescendant() check (9.41 KB, patch)
2013-01-29 19:16 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Moved early exit back to original location (9.39 KB, patch)
2013-02-05 17:38 PST, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2013-01-28 14:50:59 PST
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.
Comment 1 Shawn Singh 2013-01-28 14:53:22 PST
Created attachment 185072 [details]
Reduced test case
Comment 2 Simon Fraser (smfr) 2013-01-28 15:22:04 PST
(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.
Comment 3 Shawn Singh 2013-01-29 15:30:41 PST
Created attachment 185321 [details]
Patch
Comment 4 Shawn Singh 2013-01-29 15:33:37 PST
> >   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.
Comment 5 Simon Fraser (smfr) 2013-01-29 15:45:56 PST
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.
Comment 6 Shawn Singh 2013-01-29 16:40:01 PST
Created attachment 185341 [details]
Patch
Comment 7 Shawn Singh 2013-01-29 16:41:19 PST
(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!
Comment 8 Simon Fraser (smfr) 2013-01-29 16:47:21 PST
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.
Comment 9 Shawn Singh 2013-01-29 19:16:46 PST
Created attachment 185381 [details]
moved the parent->hasVisibleDescendant() check
Comment 10 Shawn Singh 2013-01-31 15:17:05 PST
Hi Simon,  could you please take a look at the most recent patch?   Thanks in advance!
Comment 11 Simon Fraser (smfr) 2013-01-31 15:20:04 PST
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.
Comment 12 Shawn Singh 2013-01-31 16:15:57 PST
(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.
Comment 13 Shawn Singh 2013-02-05 15:09:22 PST
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.
Comment 14 Simon Fraser (smfr) 2013-02-05 15:43:24 PST
Let's go back to the previous one.
Comment 15 Shawn Singh 2013-02-05 17:38:15 PST
Created attachment 186732 [details]
Moved early exit back to original location
Comment 16 WebKit Review Bot 2013-02-06 11:36:57 PST
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>
Comment 17 WebKit Review Bot 2013-02-06 11:37:01 PST
All reviewed patches have been landed.  Closing bug.