Closed Bug 52334 Opened 22 years ago Closed 20 years ago

Problems with contentDocument and 'display:none' in IFRAMES.


(Core :: DOM: Core & HTML, defect, P3)






(Reporter: val, Assigned: jst)


(Blocks 1 open bug, )


(5 keywords, Whiteboard: [Hixie-P2] [ADT2] [FIXED ON TRUNK])


(9 files, 7 obsolete files)

2.23 KB, text/html
337 bytes, text/html
3.59 KB, text/html
705 bytes, text/html
2.95 KB, text/plain
2.36 KB, text/plain
303.72 KB, patch
: review+
Details | Diff | Splinter Review
744 bytes, patch
: review+
Details | Diff | Splinter Review
1.36 KB, patch
: review+
Details | Diff | Splinter Review
From Bugzilla Helper:
User-Agent: Mozilla/4.6 [en-gb]C-CCK-MCD  (Win98; I)
BuildID:    2000080712


I was about to submit the following report, but discovered a further problem 
when trying my attachment:
When the IFRAME src is simply 'blue.html' on my desktop, the testcase performs 
as described.
But when I change the testcase to instead use
the content of the IFRAME is not accessed.

If this is also the case for you with the *FIRST attachment*, then I suggest 
saving 'blue.html' and the *SECOND attachment* to the same folder and trying the 
testcase offline offline.


Three problems which may be related:

1. If an IFRAME has the property 'display:none', then contentDocument returns 

2. If an IFRAME has the property 'display:none', then immediately after changing 
this to 'display:inline', contentDocument will still be null. But it will return 
the document on a second attempt.

3. If a normally displayed IFRAME is changed to 'display:none', then an 
immediate attempt to access the contentDocument will crash the browser.

Reproducible: Always
Steps to Reproduce:

The attachment consists of a control example, followed by an example of each of 
the above:

Example0 - iframe normally displayed:

function switchId(){
document.getElementById('divId').innerHTML = 

Example1 - iframe 'display:none':

function switchId1(){
document.getElementById('divId1').innerHTML = 

Example2 - iframe 'display:none':

function switchId2(){
document.getElementById('divId2').innerHTML = 

Example3 - iframe normally displayed:

function switchId3(){
document.getElementById('divId3').innerHTML = 
The second testcase (with blue.html saved) behaves as described on Windows 98, 

Hyatt's 2000-07-11 16:06 comment on bug 44437 may explain why example 1 (the 
second div) isn't able to grab information from a display-none iframe, and 
why "If an IFRAME has the property 'display:none', then contentDocument returns 

cc Hyatt because he probably knows what to do with this bug :)
setting bug status to New.
Ever confirmed: true
IMO display: none not loading the contents of the iframe is correct.  If it 
isn't correct, we'll definitely nneed to add a new kind of option to prevent 
iframes from loading their docs.

Changing this would kill chrome performance.
*** Bug 60960 has been marked as a duplicate of this bug. ***
Example 0 of testcase doesn't work!
See for this Bug 59243 .
It look's like it has to do with bug 34297
'form controls with style="display: none;" unsuccessful in Mozilla'

So not only the iframe also the form elemts have this problem!
The form control and frame problems might appear similar but they are unrelated.
AFAIK the crash mentioned here is fixed and loading documents into iframes with
display:none is not something that will be fixed for the next release, if ever.
Marking Future for now.
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: --- → Future
Keywords: dom2
Component: DOM Level 2 → DOM HTML
*** Bug 67774 has been marked as a duplicate of this bug. ***
Most importantly, if a decision is ever made to do this, we'll have to break out
XUL, since it cannot behave that way.  (Keeping the document for a display: none
iframe is grossly inefficient and IMHO wrong anyway).

Using visibility: collapse in a table cell would be a way to hide an iframe but
keep the content around.
I agree with David, marking WONTFIX.
Closed: 21 years ago
Resolution: --- → WONTFIX
ccing djoham. Is this the bug you were talking about on mozillazine?
If so, please comment here why you think the current behaviour is incorrect. If
you have a good point, this bug may need to be reopened.

Thanks for responding. Actually, the two bugs that are right now causing me to
loose sleep with display:none are bug 55987 and bug 34297. My question about XBL
comes from a comment in bug 34279. To be honest, I'm not really sure I
understand the nature of the bug here...

At the moment, Mozilla is very broken as far as advanced HTML application
development is concerned. display:none is critical for layout and any form of
sophisticated development. The workarounds to the problem are beginning to make
my code unmaintainable. I must point out that IE handles display:none perfectly
and exactly as HTML developers would expect.

Thanks for your reply. If there is any other information that you need, please
either post to one of the bugs or Email me directly.

Best regards,

oops. Sorry for the spam. The XBL comment was in bug 34297
Why do you claim that this display: none behavior is correct?  I contend that it
is actually incorrect.  display: none is supposed to mean that the front end
object is destroyed.  In effect, according to CSS, the iframe has no display.

This is why CSS has visibility: collapse and visibility: hidden.  Those are
properties that preserve the front end object while hiding or collapsing the
visual representation of the object.  Mozilla *does* preserve the
contentDocument when these properties are used.

I think this is a case of you seeing that IE works a certain way and simply
assuming that it is the correct way, when that isn't necessarily the case.
From :

> The values of [the 'display'] property have the following meanings:
> [...]
> none 
>      This value causes an element to generate no boxes in the formatting
>      structure [1] (i.e., the element has no effect on layout). Descendant 
>      elements do not generate any boxes either; this behavior cannot be
>      overridden by setting the 'display' property on the descendants. 
>      Please note that a display of 'none' does not create an invisible box; 
>      it creates no box at all. CSS includes mechanisms that enable an element
>      to generate boxes in the formatting structure that affect formatting but
>      are not visible themselves. Please consult the section on visibility [2]
>      for details. 

[1] :

> In this model, a user agent processes a source by going through the following
> steps:
> [...]
> 5.From the annotated document tree, generate a formatting structure. Often,
> the formatting structure closely resembles the document tree, but it may
> also differ significantly, notably when authors make use of pseudo-elements 
> and generated content. First, the formatting structure need not be 
> "tree-shaped" at all -- the nature of the structure depends on the 
> implementation. Second, the formatting structure may contain more or less 
> information than the document tree. For instance, if an element in the
> document tree has a value of 'none' for the 'display' property, that element 
> will generate nothing in the formatting structure. A list element, on the 
> other hand, may generate more information in the formatting structure: the
> list element's content and list style information (e.g., a bullet image). 

> Note that the CSS user agent does not alter the document tree during this
> phase. In particular, content generated due to style sheets is not fed back to
> the document language processor (e.g., for reparsing). 


> The 'visibility' property specifies whether the boxes generated by an element
> are rendered. Invisible boxes still affect layout (set the 'display' property 
> to 'none' to suppress box generation altogether). [...]

I think what David J. wants is an easy cross-browser way to achieve IE's
behaviour for display:none. He tried the "visibility: collapse in a table cell"
workaround and it seems to work in Mozilla but not in IE. If that's true, it's
probably not a good solution. Hyatt, how would you resolve the dataloss reported
in bug 55987? (Or is that bug unrelated?)
If visibility: collapse doesn't work in IE, then IE is in violation of the CSS2
specification, and a bug should be filed against IE.
That's exactly right!

Keep in mind that I'm coming from the point of view of an application developer.
That means I want a consistant and logical API to code to. When I read the specs
that were just posted, I don't see anywhere that states that when I set
"display: none" the object is destroyed. As a matter of fact, I feel that my
reading of the specs is strengthened since the rendering of the object is the
primary focus of what was presented.

My biggest beef with Mozilla is the fact that it is 100% inconsistant in how it
renders things between elements. Using CSS and width: 100% will layout a
textarea within the borders of a TD element. Using the same CSS rule on an
iframe or select will render the element beyond the border of the cell and make
the page look unprofessional. I have the same problem with consistancy and
display: none. Let me give a few examples of what I have to deal with with the
current situation...

Let's start with an input element. I create the page with 
<input type="text" value="foo">. A button is pressed which then takes this
element and sets its display to be none and then back. David H, in your
interperitation, I should loose the value that I put in and end up with a blank
text box. But Mozilla does keep the value for these elements. However, if you
use the same logic with an iframe, you loose all of your contents. As far as
HTML is concerned, an iframe is just another element. Why doesn't it act as the
others do?

Take another example. I have set the display property of the text box to be none
and then try to set its value. If the object is supposed to be destroyed, why
does Mozilla let me do this without throwing an error? As a matter of fact, I
can read the new value of the "display: none" text box. I can read it, that is,
until I change its display back, in which case the new values are destroyed and
the old values restored. However, trying to do the same thing with iframes (this
bug) generates an error. 

Finally, consider the case in which I want to send the data contained in a form
element with display: none to the server. Nope, not allowed. Even though I can
read and write to the values of these elements from the client side. I have to
unset the CSS display before I send the values to the server. Well, that makes
the page look really stupid. OK, lets set the value to width and height 0. Oops,
still looks stupid (bug 58220). Better do something else. visibility: collapse?
Doesn't work with IE. How many hacks are we at now just to send something to the
server and retain page layout?

David H, your workaround is OK in Mozilla, but do you realize what you're asking
me to do? First of all, your work around doesn't work in IE. If I do a 

document.getElementById("textarea").style.visibility = "collapse" 

in IE I get the run time error "Could not get the visibility property. Invalid
Argument" So now I need to browser sniff and do display: none for IE (which I
*have* to support, standards compliant or not) and a visibility: collapse for

Second of all, visibility: collapse does not reflow in the same manner (here we
go again) as display: none. For example, if I have a textarea inside a table and
set the text area's visibility to collapse, the table does not resize and I
don't get a page reflow. If I set display: none, the table does resize. This
difference is very limiting in what it allows me to build while still providing
maintainable code. The fact that IE doesn't support it makes it even more of a
pain in the neck. 

Do you see my point? Mozilla's handling of display:none does not make sense from
a developer's standpoint. It is inconsistant and illogical. Its also a pain in
the *ss to work around as evidenced by the number of complaints these bugs have


David H

I'm sorry, but what world do you live in? In my world, the browser with the 80%
market share is the reference implementation for all of the others. I *HAVE* to
support IE. I *don't* have to support you. In fact, many of my customers have
Netscape/Mozilla support simply because I believe in open platforms.

Might I suggest you get off your horse a little bit and think things through
from the perspective of your customers? Sure, IE has a problem. OK, now what? I
can't fix it. You can't fix it. We have to do something. So what's it going to be?

Removing support for IE is *NOT* an option. The only other options are fixing
the mozilla code to match IE's or dropping support for Netscape/Mozilla
altogether. Guess which option most business are going to choose? Hint: your
product is *not* the one with the commanding market share...

Ok, so you think your implementation is correct. Fine. I'd like you to defend
that opinion. I've told you why I think its wrong and the pain its causing me.
In my opinion, you have an implementation that is fundamentally broken, if for
no other reason that then inconsistency of the implementation itself. A quick
glance to the other bugs that I've linked to will indicate I'm not alone in my
opinions. I would be very much interested in how you justify forcing developers,
who and are your only hope for relevancy in the future, to jump through
countless undocumented hoops just to do something that IE does correctly (even
if only in perception) right out of the box. 

I would also be very interested in understanding why changing the behavior of a
silly CSS rule is such a big deal. I don't see it, but lets assume for a minute
that you are correct and the mozilla implementation is the proper one. Great.
Now we have a solution that is 100% compliant to the specifications and is 100%
useless because the *implementation* of the specifications is different between
the competing solutions. IE is already out there. We can't change that. We're
talking the implementation of standards here. Standards are useless if the
implementations are different between platforms. Like it or not, IE got here
first. They have the *right* to define the reference implementation. Even if you
don't agree with how IE's implementation works, how can you believe that
deliberately being incompatible with the most common browser is right? Is that
not just as presumptuous as Microsoft trying to set its own standards?

djoham, please calm down. There is no need to shout at hyatt (or any other
mozilla developer). He only said that if there's a bug in IE, that bug should be
filed against IE, which is correct. Of course, this does not offer a solution to
your problem yet.
When adding comments to bugs, it is essential to keep different issues
separated. Nobody has confirmed yet that this bug and bug 55987 are indeed so
closely related that this bug is the right place for this discussion. (The only
reason why I assume it's the right place is that the workaround described in
this bug seems to be a workaround for bug 55987, too.) From your point of view
there may be "countless undocumented hoops", but this doesn't help solve this
particular bug, which is concerned with IFRAMEs and display:none.
Also note that it is not Mozilla's goal to be a perfect IE clone. Instead, I
think the strategy always was to follow the standards completely _and_ at the
same time provide a cross-browser for web developers.
Certainly noone wants to make life unnecessary hard for web developers.
dbaron, hixie: maybe one of you css experts can interpret the spec for us here?
Second try (both dbaron and hixie were excluded from the last notification).

dbaron, hixie: can you help us here?
you are, of course correct. hyatt, I apologize.

I want Mozilla to succeed. Mozilla has been my daily browser since M8. I 
believe strongly in the cause.

But it is extremely frustrating to me when I have to contend with what I 
perceive as an elitist attitude among some of the developers. I was once told 
by a Mozilla engineer that since CSS didn't specifically mention HTML form 
controls, he didn't see any reason to even have CSS apply to them because it 
didn't follow the standard. Can you imagine?

Hyatt, when I read your response, I took it as a rude dismissal to a valid 
complaint. that may not be what you intended, but read what you wrote again. 
Can you see where I could have gotten that from?

I guess all I'm saying is that people like me have to use your product. Some of 
the mozilla developers are making my life very difficult in their pursuit of 
perfection. Please, try to remember the "little" people when you are making 
some of these decisions.

Apologies again,

Just FYI,

For the record, this bug is not the issue that I am championing, although I can 
see myself hitting it in the future. I am more concerned with bug 55987. Just 
thought I would say that to make sure I was clear...

David J
Andreas: er, at the risk of sounding stupid, what's the question here? All I see
is pages of advocacy comments and pastes of document markup, I can't find a
description of the alleged bug anywhere... :-(
QA Contact: vidur → desale
Hi Ian, 

Maybe I can help. At issue here is Mozilla's behavior when the CSS rule 
display:none is used. There are a number of bugs logged in Bugzilla 
(specifically this one, bug 55987 and bug 34297) that web developers have logged 
asking for this behavior to be changed. 

It seems to me that the real issue is a respectful disagreement about how 
display:none is supposed to work. This bug has kind of turned into the debate 
bug about this issue. I believe that you were asked to look at these bugs to get 
your interperatation of the specifications in order to help us better understand 
what is the right thing to do and what we must do in order to be the best 
brower we can be....


I guess I'm being inconsistent, since I do want display: none to work with form
Hyatt, what do you mean by that? Can you be more specific please?
Even when a form control has display: none, all of the properties should be
valid, i.e., you should be able to get/set the value, checked, disabled, etc.
properties.  This all works with my XBL form controls.  To say that I don't want
this to work for iframes is inconsistent.
I don't see any reason why IFRAMEs shouldn't have a full document loaded and
live in the DOM, regardless of whether the document is actually shown or of
any styles set on the element.

For example, if I create a new 'document' object, insert an 'IFRAME' element,
and set the 'src' attribute, it should work. Even though the document has no
related view or stylesheet or whatever.

Resolution: WONTFIX → ---
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
*** Bug 87058 has been marked as a duplicate of this bug. ***
As as 87058 has been reopened, attachment 39480 [details] is no longer relevant to bug 
52334 and should be ignored
BTW, this is indirectly the cause of some skinability problems.
Whiteboard: [Hixie-P2]
Blocks: 94623
*** Bug 95136 has been marked as a duplicate of this bug. ***
Un-Futuring, re-targetting for mozilla0.9.7.
Target Milestone: Future → mozilla0.9.7
Rod, here's the iframe/frame display:none bug...
jst, would you consider bug 55987 to be a dup of this bug?
*** Bug 55987 has been marked as a duplicate of this bug. ***
Blocks: 103997
*** Bug 87058 has been marked as a duplicate of this bug. ***
*** Bug 103997 has been marked as a duplicate of this bug. ***
Blocks: 108105
Any progress on this bug? 
No concrete progress on this yet, I know what needs to be done (at least I think
I do) but I haven't had any time to write any code for this yet. I did create a
branch for this work last week, the branch name is IFRAME_20011127_BRANCH and
once I start working on this I'll start landing things on that branch (hopefully
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Blocks: 105405
*** Bug 121226 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Keywords: nsbeta1
Keywords: topembed+
I just saw the topembed+ keyword being added to this bug and I want to make sure
that one thing is clear to everyone. The impact of the coming fix for this bug
is huge, this is a very large change in how frames and iframes are loaded and
dealt with, this is something that will need to bake on the trunk for quite some
time before we can be sure that everything still works after the patch has
landed. For a preliminaty look at the coming fix, check out:

  cvs diff -r IFRAME_20011127_BASE -r IFRAME_20011127_BRANCH

Most of the changes are isolated to the content, layout, and docshell directories.
crashed when changing the stylesheet to alternative as described in the url 
Incident ID: 2557290, build 2002-02-04-08, win2k. 
Reason: access Voilation.
adding the crash keyword

[d:\builds\seamonkey\mozilla\layout\html\base\src\nsLineBox.cpp, line 275]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 1959]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 1553]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 822]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLFrame.cpp, line 564]
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxToBlockAdaptor.cpp, line 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxToBlockAdaptor.cpp, line 
nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsScrollBoxFrame.cpp, line 395]
nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsContainerBox.cpp, line 654]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 
nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 991]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsViewportFrame.cpp, line 574]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLReflowCommand.cpp, line 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6188]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6243]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5039]
[d:\builds\seamonkey\mozilla\content\base\src\nsDocument.cpp, line 3480]
[d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLDocument.cpp, line 
[d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 4326]
[d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 2515]
line 106]
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 
line 1299]
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 834]
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 925]
js_GetProperty [d:\builds\seamonkey\mozilla\js\src\jsobj.c, line 2448]
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2635]
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 850]
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappedjsclass.cpp, line 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappedjs.cpp, line 430]
line 117]
line 139]
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
[d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 635]
[d:\builds\seamonkey\mozilla\content\base\src\nsDocument.cpp, line 3236]
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5983]
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5911]
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 390]
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 347]
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 347]
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1900]
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 83]
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 854]
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 871]
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4737]
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3575]
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1116]
Keywords: crash
Keywords: testcase
This is a brutal one to handle when you use hidden iframes for updating dynamic
content :)
We got around this problem by using the "style.height" CSS attribute instead of 
the "style.display" attribute.  Just set style.height to "0pt" to hide, 
and "auto" to show.  It also works for other display:none issues like checkbox 
loss-of-data.  The content effectively hides, but the data (or IFRAME) is still 
there and appears to be treated as if it was visible.
A fix for this one is on its way, iframes already load their documents even if
they're not displayed in my tree. Just need to irno out the last ramining issues...
Awesome- I've been trying to get this to work for quite some time now.  Without
this, we'd only be able to support IE5+ in our web app.  If you've got a binary
build you need tested, I could probably help out.
Great, I might put up a build once I get that far, but for now I can't even
start mozilla with my changes :-) I know the iframe loading code works in
viewer/mfcEmbed, but there's still issues with loading XUL iframes n' all that.
Maybe next week...
Ok, new branch created (IFRAME_20020207_BRANCH), mozilla now starts and appears
to run, but there are still some issues left. I see exceptions on the JS console
about some getters not returning what they used to return in all cases in the
chrome code. Looking into that now...
*** Bug 95136 has been marked as a duplicate of this bug. ***
Pushing to mozilla1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Keywords: mozilla1.0+
Blocks: 129474
ADT3 per ADT triage.
Whiteboard: [Hixie-P2] → [Hixie-P2][ADT3]
Blocks: 127969
Finally fixed the last known problems, this patch is ready for reviews.
Attachment #74097 - Attachment is obsolete: true
This is the output of, attached so that I don't have to do that
sort of review :)  Most of them seem pretty valid to me, you can decide for
yourself on some of the long lines issue if you want.  Keep in mind that some
of the whitespace problems are whitespace at the end of the line. if you want to run it
yourself :)  For such a large patch, there are remarkably few problems.
Now for my review.  I've gotten up to nsDocumentViewer, 1/3 of the way through
(I will resume in the morning).  A couple of these are just questions.

>>> nsAccessibilityService::CreateIFrameAccessible
if (outerPresShell) {
does not need to be there at all since it's been checked already

2. I am not sure if it is good for us to be returning an accessible *frame* for
something that does not have a frame.

>>> content/base/src/
3. A bunch of extra tabs at the end of lines for no apparent reason (they don't
seem to line things up, and it makes them inconsistent with the rest of the file

4. Also, don't forget and Mac build changes.

>>> nsDocument::ResetToURI
  PRInt32 indx = mStyleSheets.Count();
  while (--indx >= 0) {
This should just be a for loop, and use i ...

>>> nsDocument::FindContentForSubDocument
6. Are you sure document -> content is used less than content -> document?  It
seems to me document -> content is more often.  There seem to be a similar
number of calls in the code, but maybe this isn't an issue.

>>> DocumentViewerImpl::SyncParentSubDocMap

+  nsCOMPtr<nsIContent> content;
+  if (mDocument && win) {
+    nsCOMPtr<nsIDOMElement> frame_element;
+    win->GetFrameElement(getter_AddRefs(frame_element));
+    content = do_QueryInterface(frame_element);
+  }
+  if (content) {

Should be more like:
+    ...
+    nsCOMPtr<nsIContent> content = do_QueryInterface(frame_element);
+    if (content) {

>>> nsDocument::SetContainer
8. Why call SyncParentSubDocMap here?  Will DocumentViewer::SetDOMDocument() not
always be called?

>>> DocumentViewerImpl::Init
9. Just a question.  If the pres context isn't created in Init(), then where
will it be created (when the frame is actually created)?

+    // We must do this before we tell the script global object about
+    // this new document since doing that will cause us to re-enter
+    // into nsHTMLFrameInnerFrame code through reflows caused by
+    // FlushPendingNotifications() calls down the road...
What is causing this that wasn't there before?

+      // XXX - we observe the docuement always, see above after preshell
+      // is created
Spelling.  Also XXX style, I'd remove the -.  These aren't your fault but you're
going through it anyway :)

-    if(!erP)
-      return NS_ERROR_FAILURE;
I think you missed this error condition; do_QueryInterface(mDocument, &rv)
should suffice though, I would think.

>>> DocumentViewerImpl::SetDOMDocument

+        parent_doc->SetSubDocumentFor(content, mDocument);
Should check and return rv

14. Same question as Init().  Does this code happen again when the frame is created?

>>> DocumentViewerImpl::Show

15.  Ewww, it looks like a lot of this is the answer for questions #9 and #14. 
Is there useful consolidation possible?  (It looks like at least the stuff from
Init() can be consolidated.  Large portions of that are duplicated precisely.)
Is it possible to make this a publicly-available test build?  We've got some
heavy IFRAME stuff that is totally broken under Moz right now and would really
exercise this and possible iron out any quirks before 1.0.
Someone should check that this does not regress the testcases in bug 91516.
Comment on attachment 75275 [details] [diff] [review]
This is "The One" (modulo and mac project file changes)

Review: The Saga Continues

I am over 2/3 done now, reviewing layout.

>>> nsFrameLoader::LoadFrame
+  if (!doc) {
+    // Can't find owner doc, don't load the frame...
+    return NS_OK;
Shouldn't this return a failure?

>>> nsFrameLoader::EnsureDocShell
   nsCOMPtr<nsISupports> container;

-  if (container) {
-    nsCOMPtr<nsIDocShellTreeNode> parentAsNode(do_QueryInterface(container));
-    if (parentAsNode) {
-      nsCOMPtr<nsIDocShellTreeItem> parentAsItem =
-	 do_QueryInterface(parentAsNode);
-      PRInt32 parentType;
-      parentAsItem->GetItemType(&parentType);
+  nsCOMPtr<nsIDocShellTreeNode> parentAsNode(do_QueryInterface(container));
+  if (parentAsNode) {
+    nsCOMPtr<nsIDocShellTreeItem> parentAsItem =
+      do_QueryInterface(parentAsNode);
Are we guaranteed container will be there?  If we should be guaranteed, this is

>>> nsHTMLIFrameElement::EnsureFrameLoader
18. This method will return NS_OK and not load the mFrameLoader if the parent
is null, and most callers aren't checking for that condition (they will crash).
 Probably when !mParent it needs to return NS_ERROR_FAILURE or some such.

>>> nsHTMLIFrameElement::SetSrc
+nsHTMLIFrameElement::SetSrc(const nsAReadableString& aSrc)
+  SetAttribute(NS_LITERAL_STRING("src"), aSrc);
+  return LoadSrc(aSrc);
Hate to bring it up, but what will happen if, in the DOM, setAttribute("src")
occurs?  Will the document ever be loaded?

>>> content/html/document/src/nsHTMLContentSink.cpp
20. Please don't make those first few formatting changes to select and friends,
just put that in your sr of my bug 108309 patch :)  You might consider not
making some of the other formatting fixes (like comment line length) so that
you don't conflict unnecessarily.

>>> nsXULElement::HandleDOMEvent
-    else if (aEvent->message == NS_IMAGE_LOAD)
+    else if (aEvent->message == NS_IMAGE_LOAD ||
+	      aEvent->message == NS_PAGE_LOAD)
       return NS_OK; // Don't let these events bubble or be captured.  Just
allow them
		     // on the target image.
Comments needs updating.

>>> extensions/inspector/base/src/inLayoutUtils.cpp
+  if (parent_doc) {
+    nsCOMPtr<nsIContent> content;
+    doc->FindContentForSubDocument(doc, getter_AddRefs(content));
+    node = do_QueryInterface(content);
+  }
It seems to me the third line should start with parent_doc instead of doc ...
Comment on attachment 75275 [details] [diff] [review]
This is "The One" (modulo and mac project file changes)

>>> nsHTMLFrameInnerFrame::Init
24. The print and print preview changes in this function looks wrong to me, I
can't find anywhere in this patch that it get picked back up in either.  Is
this tested with print and print preview?

With these issues fixed and questions answered or fixed, you have r=jkeiser. 
Good solid patch with lots of cleanup, I like :)

I will actually patch and run once you get the new patch in, too.
And no, there is no 23. :)
Robert, thanks, I tested the testcase in bug 91516 and this does not regress
that. I wouldn't have expected anything like that either since this doesn't
really change anything in the case where an iframe is actually displayed.

New patch that fixes rpotts' sr issues on its way.
Matthew, I'll try to push out windows builds with these changes in them to, if that doesn't happen, I can always email you a zip file.
This fixes the issues that were found by rpotts while sr'ing (mostly minor
issues). I didn't have time to go through jkeiser's review comments yet, more
jst: Sounds good...  I'll keep my eye on the latest directories on
Answers to jkeiser's questions n' comments:

6. I think the current map is the more efficient of the two, but that's more a
guess than anything else. This is an old map though, this patch just moves it.

7. If I'd make the suggested change I'd need to have frame_element in a wider
scope, and I don't see the point in doing that.

8. DocumentViewerImpl::SetDOMDocument() isn't always called.

9. If the prescontext isn't created in ::Init(), it's created in ::Show().

More later...
So in case it hasn't been clear from other comments: all known functional
regressions have been resolved.

Johnny is still investigating strange page load numbers: pages that usually
differ the most (randomly) from the baseline seem to differ even more with jst's
patch. Other numbers don't seem to be affected, and concentrating on the pages
(like voodooextreme) alone do not cause this randomness to increase.
which server are you testing pageload with? (cowtools should be fine).
Let me know if you need anything from me on the pageload questions.
Not sure if this is currently being addressed so I'll mention it just in case...

Using Build 2002032708 on Win2k (SP2sr1)

I am getting a consistent crash at:

If one follows the instructions and highlights half the nunmber in the IFrame
and then switches the stylesheet to "alternate", Mozilla will crash every time.
 The highlighting of text is key because Mozilla doesn't crash if the text is
*not* highlighted during the switching of the stylesheet.

Is this part of what is being addressed in this bug or is it a different bug?

Ok, first of all, all known functionality issues with these changes have been
taken care of with the latest version (yet to be posted here), the performance
problems are also fixed so these changes should be ready to go. QA has been
running tests, test builds have been posted and everything seems fine, except
for the crash reported above. That crash, however, is not due to these changes
directly. The crash that happens with the above testcase (note, you don't need
to select anything in the iframe, just giving it focus is enough) is due to a
re-entrancy problem in the layout frame destruction code that is much easier to
trigger with these changes than w/o them. We end up destroying a frame, and
while doing so we end up destroying a window which passes focus along to one of
the frames we're destroying, and that ends up reflowing that frame and since
that frame is being destroyed we end up getting references to deleted frames n'
other nice things that lead to the crash. I'm investigating that problem right now.
Ok, I just grabbed a build that didn't have my changes in it and did some more
testing with it. Turns out that the crash reported above is just as
reproduceable w/o my changes, I crash in the exact same place due to the exact
same reasons with a trunk build w/o my changes. Because of this fact, I won't be
spending any more time on that crash as part of this bug, we need a new bug for
that crash.

I'm about to post a new complete diff for these iframe loading changes, AFAIK
there are no problems with these changes, pageload numbers look good, no known
Attached patch Same as above, but diff -w (obsolete) — Splinter Review
Attachment #75275 - Attachment is obsolete: true
Attachment #75545 - Attachment is obsolete: true
I reported the crash mentioned in comment 79 as bug 134437

I added a talkback ID there and comments that jst had about the crash in this 
bug.  This one definitely needs fixing before 1.0!

Comment on attachment 76806 [details] [diff] [review]
Hopefully final patch, all issues resolved.

My comments are mainly style nits. I'll do the rest another day.
I have a hard time understanding the logic you use for the pointer-indentation:
nsIFoo* foo or nsIFoo *foo. Do you use both?

>Index: accessible/src/base/nsAccessibilityService.cpp
> void nsAccessibilityService::GetOwnerFor(nsIPresShell *aPresShell, nsIPresShell **aOwnerShell, nsIContent **aOwnerContent)

Is it possible to wrap that line?

>Index: content/base/public/nsIDocument.h
>+  /**
>+   * Find the content node for which aDocument is a sub docuemnt
docuemnt -> document

>Index: content/base/src/nsDocument.cpp
>   mRootContent = nsnull;
>   PRUint32 count, i;
>   mChildren->Count(&count);
>   for (i = 0; i < count; i++) {
>-    nsCOMPtr<nsIContent> content(dont_AddRef(NS_STATIC_CAST(nsIContent*,mChildren->ElementAt(i))));
>+    nsCOMPtr<nsIContent> content =
>+      dont_AddRef(NS_STATIC_CAST(nsIContent *, mChildren->ElementAt(i)));
>     content->SetDocument(nsnull, PR_TRUE, PR_TRUE);
>-    ContentRemoved(nsnull, content, indx);
>+    ContentRemoved(nsnull, content, i);

Are you telling me this has been wrong all along?
How close are we on this one? Waht are the chances it is gonna make it 1.0 early
next week? What's the new ETA?
Whiteboard: [Hixie-P2][ADT3] → [Hixie-P2][ADT3][ETA ?]
Jamie, this patch is ready to go, approval requested from
and positive feedback from drivers received (but no a= yet), so yes, it looks
like this could land any day now.

Fabian, long line wrapped, comment typo fixed and the answer to your last
question is yes, it's been wrong all along.

There are a few minor details still being worked on in the diff based on
jkeisers detaild re-review, but nothing that won't be fixed before landing.
Blocks: 83576
Whiteboard: [Hixie-P2][ADT3][ETA ?] → [Hixie-P2][ADT3][ETA 04/01]
Could we get a list of real world sites with this problem, as well as list of
real world sites/applications this is blocking?

To start with, I went through the duplicates and their duplicates etc. and came
up with this list:

From the dupes it looks like several webmail applications are affected by this.
This is the final patch as of this moment, all reviewer comments addressed.
This one's ready to go.
Attachment #76806 - Attachment is obsolete: true
Attachment #76807 - Attachment is obsolete: true
Comment on attachment 77124 [details] [diff] [review]
Final full diff, all reported issues resolved.


Get your r/sr noted here before checking in. Thanks!!!
Attachment #77124 - Flags: approval+
Comment on attachment 77124 [details] [diff] [review]
Final full diff, all reported issues resolved.

rpotts says his sr= still applies.
Attachment #77124 - Flags: superreview+
Comment on attachment 77124 [details] [diff] [review]
Final full diff, all reported issues resolved.

Attachment #77124 - Flags: review+
Keywords: adt1.0.0
adt1.0.0- (on behalf of ADT). Pls land this on the trunk, after Mozilla 1.0
branches. We will consider this later for MachV RTM, based on testing on the trunk.
Keywords: adt1.0.0adt1.0.0-
Whiteboard: [Hixie-P2][ADT3][ETA 04/01] → [Hixie-P2] [ADT3 RTM] [ETA 04/01]
Reversing ourselves, on this one - to a +. adt1.0.0+ (on ADT's behalf) for
checkin into 1.0.
Keywords: adt1.0.0-adt1.0.0+
Please check this in as soon as possible. Thanks!
This was not a good day. I checked this in, but it turns out that gcc 2.95 and
egcs is broken and causes a crash on startup with these changes. Debug builds
work fine, and gcc 2.96 works fine too, so it's a compiler bug, and I don't have
a workaround.

I had no other choice than to back the change out :-(

I'll see what I can do about this tomorrow...
Scrolling mail/news' 3-pane down to get the newest unread mail with my 
scrollwheel, I get the crash stack that I'll attach next.
the second
nsEventStateManager::DoWheelScroll(nsIPresContext * 0x00000000, nsIFrame *
0x00000000, nsMouseScrollEvent * 0x0012f74c, int 3, int 0, int 1) line 1370 + 35
is called with aTargetFrame == 0.
This is set by
in line 1514
This comes from the
 if (!parentDoc) {
     return NS_OK;
patchlet in nsEventStateManager.
Ok, so it turns out that the crash was not actually due to a compiler bug, it
was due to an uninitialized variable in my code
(DocumentViewerImpl::InitInternal(), |rv| was returned w/o ever being set in
some cases). It amazes me how consistently things worked/didn't work even with
the uninitialized variable though...

Problem fixed, I'm looking into the scrollwheel crash right now...
Add another bug to track down with this patch:

1) go to the SeaMonkey tinderbox and note the existance of the background colors
in the Ports iframe
2) take a link off the page (I used the Testerbox link)
3) hit back
4) note the lack of background colors

I noted this before the backout tonight and after I updated and rebuilt the
behavior is gone.  That's fairly strong evidence its this patch.  Caillon also
observed the same behavior and the same timeframes.
I tried this on win32, with and without the patch, and I see similarly elevated
times for those pages which use iframe/frame, although the overall delta in 
times between the two builds is less than what was seen on Linux.

I reviewed the access_logs and (just to rule out a hunch) the same content urls 
are being loaded for both builds/runs (i.e., no redundant data is fetched for 
those (i)frames).
When do we believe this will be landing? Can we get an updated ETA in the Status
Whiteboard: [Hixie-P2] [ADT3 RTM] [ETA 04/01] → [Hixie-P2] [ADT3 RTM] [Need ETA ??/??]
Removing adt1.0.0+.
Keywords: adt1.0.0+
Tinderbox iframe problem fixed, see nsGenericHTMLElement::InNavQuirksMode() for
the fix for that.

Returning from print preview loosing iframe content fixed, see
DocumentViewerImpl::Hide() and DocumentViewerImpl::GetDoingPrintPreview() for
the fix for that.

The fix for the scrollwheel crash is in the first two hunks of the
nsEventStateManager.cpp changes.

nsHTMLDocument::Reset() also changed since that was causing mAttrStyleSheet and
mStyleAttrStyleSheet to be added twice to mStyleSheets in HTML documents for no
good reason.

The rest is unchanged modulo trivial merging problem fixes.

New patch coming up once cvs diff is done...
The pageload regression fix (that I forgot to mention in my earlier comment) is
in nsHTMLFrameInnerFrame::ShowDocShell(), look for is_document_synthetic and
Attachment #77124 - Attachment is obsolete: true
Comment on attachment 79498 [details] [diff] [review]
All issues resolved, including pageload time regression

1. nsGenericHTMLElement::InNavQuirksMode(nsIDocument* aDoc)
+    if (eDTDMode_quirks == mode) {

Switch sides, por favor.

2. is_in_print_preview ... what up with the new naming convention? :)

3. nsHTMLDocument::Reset doesn't need to be there at all, now.

r=jkeiser pending discussion with rods on whether Hide() can legitimately be
called during print preview, ever.
Attachment #79498 - Flags: review+
Depends on: 127891
Depends on: 138007
Depends on: 138138
Depends on: 138540
Whiteboard: [Hixie-P2] [ADT3 RTM] [Need ETA ??/??] → [Hixie-P2] [ADT3 RTM] [FIXED ON TRUNK]
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1-
Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.
Keywords: nsbeta1+
So is there any chance that this gets on the 1.0 branch at some point, so that
the next generation of commercial releases include this fix?
Changing to [adt2 RTM]/nsbeta1+ because there are real sites using this on top
US sites. Let's try to get this in the trunk asap.
Keywords: nsbeta1-nsbeta1+
Whiteboard: [Hixie-P2] [ADT3 RTM] [FIXED ON TRUNK] → [Hixie-P2] [ADT2 RTM] [FIXED ON TRUNK]
Depends on: 138900
Depends on: 138676
<iframe src="" onLoad="alert('');"

The above is not alerting anything due to display:none;
Does this bug fix that too or should I open a new bug?

Using build 20020512 Windows 2000 SP2.
Comment on attachment 83730 [details] [diff] [review]
Let onload events fire on iframes even if they're hidden

"if this windows is not"
spelling, otherwise
Attachment #83730 - Flags: review+
Comment on attachment 83730 [details] [diff] [review]
Let onload events fire on iframes even if they're hidden

Attachment #83730 - Flags: superreview+
Blocks: 143047
Keywords: adt1.0.0, approval
Whiteboard: [Hixie-P2] [ADT2 RTM] [FIXED ON TRUNK] → [Hixie-P2] [ADT2 RTM] [FIXED ON TRUNK] [Needs a= & ADT +]
Keywords: nsbeta1+nsbeta1-
Whiteboard: [Hixie-P2] [ADT2 RTM] [FIXED ON TRUNK] [Needs a= & ADT +] → [Hixie-P2] [ADT2] [FIXED ON TRUNK] [Needs a= & ADT +]
marking adt1.0.0- per discussion last week.
Keywords: adt1.0.0adt1.0.0-
Whiteboard: [Hixie-P2] [ADT2] [FIXED ON TRUNK] [Needs a= & ADT +] → [Hixie-P2] [ADT2] [FIXED ON TRUNK]
Is comment 114 a fix for the comment 113? Because I still cannot make it work.
Yes, but that wasn't checked in yet. Should land any day now. I'll update the
bug once it's checked in.
Another thing that I noticed is that the onLoad is not triggered if you set a
flash file a source on the iframe:
<iframe src="generic_popup.swf" onLoad="alert('')"></iframe>

If this bug does not deal with that let me know so I can file a new one.

José Jeria, could you please file that as a separate bug, that problem is
unrelated to this bug. The reason for that not working is that the fullpage
plugin code is not doing the right thing wrt on[un?]load handlers.
Comment on attachment 85459 [details] [diff] [review]
Slight modification of the above, don't ever bother getting a pres context since we don't need one here. 

Carrying forward,
Attachment #85459 - Flags: superreview+
Attachment #85459 - Flags: review+
Comment on attachment 85459 [details] [diff] [review]
Slight modification of the above, don't ever bother getting a pres context since we don't need one here. 

This fix has now been checked in on the trunk.
stummala - Can you verify this one on the trunk, and check around for any
No longer blocks: 143047
I believe stummala is out on sick leave at the moment. Prashant, who could we
get to verify this one on the trunk?
Ok, the last word from ADT on this one after discussing this at some length is
that we won't take this for MachV RTM unless someone comes up with further
reasons to do so, and that better happen soon, we can't take this as a last
minute fix.

Marking FIXED since this is now fixes on all branches (only on the trunk really)
where it'll be fixed.
Closed: 21 years ago20 years ago
Resolution: --- → FIXED
I will verify this one soon.

I tested this one with 05-30-08-trunk on win-95.
Here are results I found.

The First testcase (FIRST ATTACHMENT) attached shows following results.
Example 0: normal IFRAME                : FAILS.
Example 1: 'display:none' IFRAME        : FAILS.
Example 2: 'display:none' IFRAME.       : FAILS.
Example 3: normal IFRAME                : FAILS.

4th testcase attached (4TH ATTACHMENT)Shows following results

Example 0: normal IFRAME                : PASSES.
Example 1: 'display:none' IFRAME        : PASSES.
Example 2: 'display:none' IFRAME        : PASSES.
Example 3: normal IFRAME                : FAILS.

Then I tested it with testcase shown at URL

So I'm re-opening this bug since it fails in certain tests I mentioned.

Resolution: FIXED → ---
Comment on attachment 14522 [details]
FIRST attachment - online test - contentDocumnet/display:none

This testcase doesn't work because it relies on cross origin access to
document.documentElement which we don't allow for security reasons.
Attachment #14522 - Attachment is obsolete: true
Comment on attachment 20717 [details]
The new Testcase which have some changes, after bug 59243 are cleared. Also some notes from me are inserted. I hope it will help well!

The last testcase in this attachment is broken, it tries to find an iframe by
id (iframeId3) but there is no element with such an id, there's an iframe with
the name 'iframeId3', but that's not the same as id... Fix that and this
testcase will work.
Attachment #20717 - Flags: needs-work+
Marking FIXED again, this bug is fixed, the testcases were broken.
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Renominate for branch after Mach V RTM
Doesn't look this one is gonna make 1.0.1, so pls re-target for Mozilla1.1alpha.
Given that it's already in, I don't feel bad setting the milestone for Johnny here.
Target Milestone: mozilla1.0 → mozilla1.1alpha
Perfect. Marking it verified on trunk then.
Filed bug 148992 for comment 121.
*** Bug 145841 has been marked as a duplicate of this bug. ***
Will this be checked into the Mozilla 1.0.x branch? Would be very nice...
No, not unless someone has a *really* *really* huge reason to do so, this is a
scary change, it caused numrous regressions when checked in, and merging this to
the 1.0 branch would not be fun, and that would be very error prone.
*** Bug 65956 has been marked as a duplicate of this bug. ***
Depends on: 390088
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.