Bug 4377 – visibility:hidden can't be overridden in child for positioned element

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antti Koivisto]

render_layer completely ignores (as optimization) boxes with visibility:hidden.
This means however that the child element of a positioned box can't override the
visibility, as it should. http://www.moposivut.com/

Works in both IE and Gecko.

[Antti Koivisto]

Created attachment 3327 [details]
testcase

[Oliver Hunt]

From hyatt, this'll be a pain to fix -- part of the reason for webkit doing DHTML faster 

Will be looking at adding a bit field in the next day or two...

[Antti Koivisto]

Created attachment 3829 [details]
patch

keep track of child visibility in renderlayer

[Antti Koivisto]

Created attachment 3830 [details]
better test case

[Dave Hyatt]

I would put the updates in appendChildNode and insertChildNode, since addChild is not called for all 
RenderTree manipulations (e.g., when generated content is used or anonymous blocks and continuations 
cause reshuffling of the tree).

[Antti Koivisto]

Created attachment 4026 [details]
updated patch

updated patch based on daves comments

[Darin Adler]

Comment on attachment 4026 [details]
updated patch

I know this is an entirely trivial comment, but there's no need for a return;
at the end of RenderLayer::checkVisibility.

[Dave Hyatt]

See this link:

http://dbaron.org/log/2005-10#e20051013a

We should try S5, since it sounds like it's a good test case for this.

[Dave Hyatt]

Comment on attachment 4026 [details]
updated patch

+    if (style()->visibility() != VISIBLE &&
newChild->style()->visibility()==VISIBLE && !newChild->layer()) {

Style nit with the above, needs spaces before and after the ==.  

+    if (!object->firstChild() && object->style()) {
+	 m_visibilityKnown = true;
+	 m_hasVisibleContent = object->style()->visibility() == VISIBLE;
+    }

This seems like a needlessly aggressive optimization to me.  checkVisibility()
will quickly determine the same thing, right?

Your change to collectLayers results in the entire layer tree being walked now.
 The old code would not descend into child layers if they weren't visible.  You
could further optimize by propagating bits up the layer tree (i.e., rather than
just a "hasVisibleContent" in this layer bit, you could have a
"hasVisibleContent" bit for the layer or its descendant layers.

[Antti Koivisto]

I think I would have to add another bit for tracking if child layer visibility is known (or something 
comparable). I'm still not convinced this is worth it. Yes, we loop through child layers of an invisible layer 
which we did not do before, but only thing we do there is test the visibility bit. We do substantially more 
work if the test is positive, but that is the case we are fixing here in the first place.

[Antti Koivisto]

Created attachment 10790 [details]
another updated patch

- update patch to current webkit
- added m_hasVisibleDescendant bit to track visibility of child layers, avoiding the need to crawl through the entire layer tree to find visible layers

[Antti Koivisto]

Created attachment 10791 [details]
updated tests

[Dave Hyatt]

I'll review this (soon).

[Antti Koivisto]

Created attachment 10812 [details]
cosmetic fixes

[Dave Hyatt]

Comment on attachment 10812 [details]
cosmetic fixes

r=me, looks good.

[Antti Koivisto]

r16988

*** Bug 5614 has been marked as a duplicate of this bug. ***

*** Bug 9286 has been marked as a duplicate of this bug. ***

*** Bug 7909 has been marked as a duplicate of this bug. ***

[rahul abrol]

there's still a problem when the parent element has its z-index set to a number.  adding z-index:0 to #main in antti's original attachment hides #monkoolia.

see #nest on atptennis.com, for example.

[rahul abrol]

(In reply to comment #20)
> see #nest on atptennis.com, for example.

looks like they changed/fixed this.