Closed Bug 130263 Opened 22 years ago Closed 22 years ago

hidden iframe not always hidden!

Categories

(Core Graveyard :: GFX, defect, P1)

Product:
Core Graveyard
Component:
GFX
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: geraint.edwards, Assigned: roc)

References

(
URL
)

Details

(Keywords: regression, topembed+)

Attachments

(1 file)

Fix: prune all non-visible views and all their children from display lists
1.62 KB, patch
kmcclusk
: review+
attinasi
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
The test URL www.copyn.plus.com/mozTest36.html contains 8 iframes whose style
has been set "visibility:hidden".  However 4 of these IFrames are clearly visible!

The only iframes that are hidden are those where the containing element has a
z-index explicitly set higher than the iframe itself (in the blue box), or where
the iframes are subnodes of something other than the body tag (see the black box).

Is this bug related to the recent fix for bug 91516?  This is a new bug since 0.9.8.
To compositor. Confirming with 2002-03-11-08 trunk.
Assignee: jst → kmcclusk
Status: UNCONFIRMED → NEW
Component: DOM Style → Compositor
Ever confirmed: true
QA Contact: ian → petersen
*** Bug 130291 has been marked as a duplicate of this bug. ***
Keywords: regression
OS: Linux → All
Hardware: PC → All
*** Bug 130191 has been marked as a duplicate of this bug. ***
This is definitely due to my IFRAME handling changes in bug 91516.

dbaron wrote:

> It's not entirely clear to me what *should* happen here, since 'visibility'
> works by inheritance, but the contents of the IFRAME are a separate document
> that doesn't inherit the 'visibility: hidden' from the IFRAME.  One could
> definitely use that to argue that this isn't a bug, although one could also
> argue that it is a bug and the entire IFRAME should be hidden as a block.

I would say that we should treat the IFRAME as a single box in the parent
document which just happens to display a complete subdocument. Marking the
IFRAME with "visibility: hidden" means that its box is hidden, which means the
document is not visible, period.

Any interpretation based on the possible inheritance of 'visibility' into the
IFRAME's subdocument doesn't make a lot of sense, because we never inherit any
styles from the IFRAME into its subdocument.

I will try to produce a patch.
Assignee: kmcclusk → roc+moz
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.0
This should be a really simple fix. nsHTMLFrameInnerFrame already marks its view
as hidden when its CSS visibility is hidden. A hidden view should also hide all
its child views. (For this reason, normally a "visibility: hidden" element does
NOT mark its view as hidden.) The only problem is that the display list builder
doesn't hide visibile child views of a hidden view. Fixing that should be
trivial, although I'll have to do some testing to make sure this doesn't regress
anything that was depending on the old erroneous behavior.
Excellent!  In case someone hasn't said it today, you guys are doing a great job!  

As a web developer it means a lot that we can help you guys make this browser
that much better.  It means even more you listen... with that other company I
feel like a gnat screaming at an elephant.

Thank you and keep up the great work!
By removing nonvisible views *AND* their children from display lists, we ensure
that such views will never be painted nor receive events (unless they get
focused somehow and receive key events? Hmm. If that can happen, that would be
a layout problem.)

What's happening right now on the trunk is that the IFRAME's containing widget
is hidden but the clever view manager just goes ahead and paints the IFRAME
document into its parent's widget. The good news is that this means we won't
have much trouble removing usage of native widgets post-1.0 :-).

I have audited the use of SetViewVisibility and tested the paths that call it.
Basically we hardly ever mark a view hidden: only for a) CSS "visibility:
hidden" leaf frames b) popups and c) some places in XUL. Normal HTML/XML
containers never have their views marked hidden.
*** Bug 131773 has been marked as a duplicate of this bug. ***
Comment on attachment 74041 [details] [diff] [review]
Fix: prune all non-visible views and all their children from display lists

sr=attinasi
Attachment #74041 - Flags: superreview+
Comment on attachment 74041 [details] [diff] [review]
Fix: prune all non-visible views and all their children from display lists

r=kmcclusk@netscape.com
Attachment #74041 - Flags: review+
Comment on attachment 74041 [details] [diff] [review]
Fix: prune all non-visible views and all their children from display lists

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74041 - Flags: approval+
Blocks: 131528
If anybody cares, this also fixes tvguide.com.
Sometimes you see an ad that when you mouseover expands, but when you move the
mouse out it does not shrink back so it leaves it covering content.
With the patch in thus bug the ad properly disappears.
Fix checked in.

Vadim, I care ... thanks for the feedback!
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
*** Bug 132772 has been marked as a duplicate of this bug. ***
Keywords: edt0.9.9+
edt needs this on the 0.9.9 branch.  kevin's going to get it there for us.
mdunn, can you help to get us some verification love?
*** Bug 132877 has been marked as a duplicate of this bug. ***
*** Bug 132503 has been marked as a duplicate of this bug. ***
patch checked into 099 branch
Keywords: fixed0.9.9
This fix is in the trunk as well, correct.
Petersen, Jen: We need some verification help here.
Keywords: topembed+
Yes. The patch was checked into the trunk on 3/18. I checked the patch into the
099 branch on 3/22.
Verified on the March 25th OS X (2002-03-25-08) and Win32 (2002-03-25-03) trunk
builds.
Status: RESOLVED → VERIFIED
Verified on the Linux March 25th build (2002-03-25-08)
Jen, Can we verify this on 099 Win32?
Yep, np. Was waiting for a post 3/22 mfcembed 099 build.. 

Tested today on mfcembed 099 03/28/02 build, can no longer reproduce. Looks good. 

Will test on NS 03/26/02 099, downloading now, I'll post if there's a problem. 

Thanks, jenk 
*** Bug 130178 has been marked as a duplicate of this bug. ***
*** Bug 136409 has been marked as a duplicate of this bug. ***
*** Bug 137180 has been marked as a duplicate of this bug. ***
*** Bug 141427 has been marked as a duplicate of this bug. ***
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: