From Bugzilla Helper: User-Agent: Mozilla/4.6 [en-gb]C-CCK-MCD NetscapeOnline.co.uk (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 http://members.netscapeonline.co.uk/valeriegsharp/misc/blue.html 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 null. 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 = document.getElementById('iframeId').contentDocument.documentElement.innerHTML; } Example1 - iframe 'display:none': function switchId1(){ document.getElementById('divId1').innerHTML = document.getElementById('iframeId1').contentDocument.documentElement.innerHTML; } Example2 - iframe 'display:none': function switchId2(){ document.getElementById('iframeId2').style.display="inline"; document.getElementById('divId2').innerHTML = document.getElementById('iframeId2').contentDocument.documentElement.innerHTML; } Example3 - iframe normally displayed: function switchId3(){ document.getElementById('iframeId3').style.display="none"; document.getElementById('divId3').innerHTML = document.getElementById('iframeId3').contentDocument.documentElement.innerHTML; }

The second testcase (with blue.html saved) behaves as described on Windows 98, 2000091808. 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 null." cc Hyatt because he probably knows what to do with this bug :)

setting bug status to New.

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.

*** 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.

ccing djoham. Is this the bug you were talking about on mozillazine? http://www.mozillazine.org/talkback.html?article=1884&message=20#20 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.

Andreas, 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, David

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 http://www.w3.org/TR/REC-CSS2/visuren.html#display-prop : > 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] http://www.w3.org/TR/REC-CSS2/intro.html#formatting-structure : > 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). [2] http://www.w3.org/TR/REC-CSS2/visufx.html#visibility > 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 Mozilla. 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 generated. Comments? David

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? David

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, David

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... :-(

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.... HTH David

I guess I'm being inconsistent, since I do want display: none to work with form controls.

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. REOPENing.

Updating QA contact to Shivakiran Tummala.

*** 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.

See also http://www.hixie.ch/tests/evil/state/001.html

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

Un-Futuring, re-targetting for 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. ***

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

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

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 soon).

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

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 above. Incident ID: 2557290, build 2002-02-04-08, win2k. Reason: access Voilation. adding the crash keyword nsLineBox::IndexOf [d:\builds\seamonkey\mozilla\layout\html\base\src\nsLineBox.cpp, line 275] nsBlockFrame::FindLineFor [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 1959] nsBlockFrame::PrepareChildIncrementalReflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 1553] nsBlockFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsBlockFrame.cpp, line 822] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 776] CanvasFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLFrame.cpp, line 564] nsBoxToBlockAdaptor::Reflow [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxToBlockAdaptor.cpp, line 845] nsBoxToBlockAdaptor::DoLayout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxToBlockAdaptor.cpp, line 622] nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 1052] nsScrollBoxFrame::DoLayout [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 1052] nsContainerBox::LayoutChildAt [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsContainerBox.cpp, line 654] nsGfxScrollFrameInner::LayoutBox [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 1072] nsGfxScrollFrameInner::Layout [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 1231] nsGfxScrollFrame::DoLayout [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 1080] nsBox::Layout [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBox.cpp, line 1052] nsBoxFrame::Reflow [d:\builds\seamonkey\mozilla\layout\xul\base\src\nsBoxFrame.cpp, line 991] nsGfxScrollFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsGfxScrollFrame.cpp, line 789] nsContainerFrame::ReflowChild [d:\builds\seamonkey\mozilla\layout\html\base\src\nsContainerFrame.cpp, line 776] ViewportFrame::Reflow [d:\builds\seamonkey\mozilla\layout\html\base\src\nsViewportFrame.cpp, line 574] nsHTMLReflowCommand::Dispatch [d:\builds\seamonkey\mozilla\layout\html\base\src\nsHTMLReflowCommand.cpp, line 217] PresShell::ProcessReflowCommand [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6188] PresShell::ProcessReflowCommands [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6243] PresShell::FlushPendingNotifications [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5039] nsDocument::FlushPendingNotifications [d:\builds\seamonkey\mozilla\content\base\src\nsDocument.cpp, line 3480] nsHTMLDocument::FlushPendingNotifications [d:\builds\seamonkey\mozilla\content\html\document\src\nsHTMLDocument.cpp, line 1496] GlobalWindowImpl::FlushPendingNotifications [d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 4326] GlobalWindowImpl::GetFrames [d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 2515] XPTC_InvokeByIndex [d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, line 106] XPCWrappedNative::CallMethod [d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2000] XPC_WN_GetterSetter [d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp, 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] nsXPCWrappedJSClass::CallMethod [d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappedjsclass.cpp, line 1203] nsXPCWrappedJS::CallMethod [d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappedjs.cpp, line 430] PrepareAndDispatch [d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp, line 117] SharedStub [d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp, line 139] nsEventListenerManager::HandleEventSubType [d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line 1206] nsEventListenerManager::HandleEvent [d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line 1725] nsXULElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3383] nsXULElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3364] nsXULElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3364] nsXULElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3364] nsXULElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3364] nsXULElement::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 3364] nsXULElement::HandleChromeEvent [d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 4606] GlobalWindowImpl::HandleDOMEvent [d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 635] nsDocument::HandleDOMEvent [d:\builds\seamonkey\mozilla\content\base\src\nsDocument.cpp, line 3236] nsEventStateManager::PreHandleEvent [d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 468] PresShell::HandleEventInternal [d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5983] PresShell::HandleEvent [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] nsViewManager::DispatchEvent [d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1900] HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 83] nsWindow::DispatchEvent [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 854] nsWindow::DispatchWindowEvent [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 871] nsWindow::DispatchFocus [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4737] nsWindow::ProcessMessage [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3575] nsWindow::WindowProc [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1116]

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

ADT3 per ADT triage.

Finally fixed the last known problems, this patch is ready for reviews.

This is the output of jst-review.pl, 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. http://www.johnkeiser.com/cgi-bin/jst-review.tar.gz 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 1. 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/makefile.win 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 Makefile.in and Mac build changes. >>> nsDocument::ResetToURI 5. 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 7. + 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)? 10. + // 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? 11. + // 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 :) 12. - 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 13. + 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 Makefile.in and mac project file changes) Review: The Saga Continues I am over 2/3 done now, reviewing layout. >>> nsFrameLoader::LoadFrame 16. + if (!doc) { + // Can't find owner doc, don't load the frame... + + return NS_OK; Shouldn't this return a failure? >>> nsFrameLoader::EnsureDocShell 17. nsCOMPtr<nsISupports> container; presContext->GetContainer(getter_AddRefs(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 fine. >>> 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 19. +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 21. - 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 22. + 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 Makefile.in 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 ftp.mozilla.org, 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 tomorrow...

jst: Sounds good... I'll keep my eye on the latest directories on ftp.mozilla.org.

Win32 testbuild now available at: http://ftp.mozilla.org/pub/mozilla/nightly/experimental/bug-52334/iframe.zip

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: http://www.hixie.ch/tests/evil/state/001.html 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? Jake

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 regressions.

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! Jake

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?

Jamie, this patch is ready to go, approval requested from drivers@mozilla.org 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.

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: http://www.businessweek.com/ http://webmail.thirdtower.com/ http://webmail.integrity.hu/ http://mail.host-sites.com http://fred.dnsalias.org/ http://www.ifrance.com/ 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.

Comment on attachment 77124 [details] [diff] [review] Final full diff, all reported issues resolved. a=roc+moz Get your r/sr noted here before checking in. Thanks!!!

Comment on attachment 77124 [details] [diff] [review] Final full diff, all reported issues resolved. rpotts says his sr= still applies.

Comment on attachment 77124 [details] [diff] [review] Final full diff, all reported issues resolved. r=jkeiser

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.

Reversing ourselves, on this one - to a +. adt1.0.0+ (on ADT's behalf) for checkin into 1.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 bytes is called with aTargetFrame == 0. This is set by GetParentScrollingView 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?

Removing 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 is_document_loading.

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.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt. If you have any questions about this, please email adt@netscape.com. You can search for "changing adt3 bugs" to quickly find and delete these bug mails.

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.

[Internal Reference] http://bugscape.netscape.com/show_bug.cgi?id=11872

<iframe src="http://www.mozilla.org" onLoad="alert('');" style="display:none;"></iframe> 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 r=axel@pike.org

Comment on attachment 83730 [details] [diff] [review] Let onload events fire on iframes even if they're hidden sr=vidur

marking adt1.0.0- per discussion last week.

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 r=axel@pike.org, sr=vidur@netscape.com

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 regressions?

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.

I will verify this one soon.

Johnney, 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 http://www.hixie.ch/tests/evil/state/001.html RESULTS: FAILS. So I'm re-opening this bug since it fails in certain tests I mentioned.

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.

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.

Marking FIXED again, this bug is fixed, the testcases were broken.

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.

Perfect. Marking it verified on trunk then.

FYI: 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. ***