Bug 4377 - visibility:hidden can't be overridden in child for positioned element
Summary: visibility:hidden can't be overridden in child for positioned element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Antti Koivisto
URL:
Keywords:
: 5614 7909 9286 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-08-10 17:09 PDT by Antti Koivisto
Modified: 2007-04-14 12:47 PDT (History)
5 users (show)

See Also:


Attachments
testcase (536 bytes, text/html)
2005-08-10 17:10 PDT, Antti Koivisto
no flags Details
patch (7.71 KB, patch)
2005-09-09 14:07 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
better test case (3.66 KB, text/html)
2005-09-09 14:12 PDT, Antti Koivisto
no flags Details
updated patch (10.62 KB, patch)
2005-09-23 21:01 PDT, Antti Koivisto
hyatt: review-
Details | Formatted Diff | Diff
another updated patch (11.74 KB, patch)
2006-09-26 21:36 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated tests (6.05 KB, text/html)
2006-09-26 21:38 PDT, Antti Koivisto
no flags Details
cosmetic fixes (11.75 KB, patch)
2006-09-27 19:34 PDT, Antti Koivisto
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2005-08-10 17:09:12 PDT
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.
Comment 1 Antti Koivisto 2005-08-10 17:10:30 PDT
Created attachment 3327 [details]
testcase
Comment 2 Oliver Hunt 2005-09-08 00:12:29 PDT
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...
Comment 3 Antti Koivisto 2005-09-09 14:07:12 PDT
Created attachment 3829 [details]
patch

keep track of child visibility in renderlayer
Comment 4 Antti Koivisto 2005-09-09 14:12:58 PDT
Created attachment 3830 [details]
better test case
Comment 5 Dave Hyatt 2005-09-09 15:38:03 PDT
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).
Comment 6 Antti Koivisto 2005-09-23 21:01:43 PDT
Created attachment 4026 [details]
updated patch

updated patch based on daves comments
Comment 7 Darin Adler 2005-10-08 23:17:16 PDT
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.
Comment 8 Dave Hyatt 2005-10-17 20:38:12 PDT
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.
Comment 9 Dave Hyatt 2005-10-17 20:59:51 PDT
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.
Comment 10 Antti Koivisto 2005-10-17 22:58:43 PDT
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.
Comment 11 Antti Koivisto 2006-09-26 21:36:44 PDT
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
Comment 12 Antti Koivisto 2006-09-26 21:38:58 PDT
Created attachment 10791 [details]
updated tests
Comment 13 Dave Hyatt 2006-09-26 22:10:54 PDT
I'll review this (soon).
Comment 14 Antti Koivisto 2006-09-27 19:34:37 PDT
Created attachment 10812 [details]
cosmetic fixes
Comment 15 Dave Hyatt 2006-09-27 22:13:27 PDT
Comment on attachment 10812 [details]
cosmetic fixes

r=me, looks good.
Comment 16 Antti Koivisto 2006-10-11 08:20:16 PDT
r16988
Comment 17 mitz 2006-12-16 12:38:05 PST
*** Bug 5614 has been marked as a duplicate of this bug. ***
Comment 18 mitz 2006-12-16 13:23:41 PST
*** Bug 9286 has been marked as a duplicate of this bug. ***
Comment 19 mitz 2006-12-19 11:02:15 PST
*** Bug 7909 has been marked as a duplicate of this bug. ***
Comment 20 rahul abrol 2007-04-07 17:23:37 PDT
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.
Comment 21 rahul abrol 2007-04-14 12:47:48 PDT
(In reply to comment #20)
> see #nest on atptennis.com, for example.

looks like they changed/fixed this.