This can be done by using GetOwnerDoc() instead of GetDocument() appropriately, I would think. I have a patch, untested thus far.

Comment on attachment 162655 [details] [diff] [review] Patch, tested on that testcase Thoughts? This is a big step towards being able to resolve bug 53901...

Actually, it looks like this breaks tabbed browsing. The problem is that the addTab() method in tabbrowser creates a new <browser> node. With my patch, the browser binding is attached to this node at creation time (by classinfo). Then the binding constructor tries to set up session history stuff and fails because there is no docshell (because XUL iframes create the docshell on initial reflow, which has clearly not happened yet). So thereafter we have no session history and tabbrowser just sorta breaks in that case (eg doesn't show the right label on the tab, etc). I can try to hack the browser binding to hold off init if the constructor runs when it's not in the document (and then to init when it's inserted in the document, if I can figure out a decent way to detect this from XBL). Alternately, I could make nsIFrameLoaderOwner/nsIFrameLoader scriptable so that the binding itself can implement nsIFrameLoaderOwner. Then the docshell would exist just fine and all that (and we can remove the hack whereby we go through the box object to get at the docshell; if we have an nsIFrameLoader pointer we can just get the docshell off of that). We could probably also remove the hacks we have in XBL where we force reflow on binding attachment just so XUL iframes will work. Finally, I suppose that I could make browser/iframe be a separate class inheriting from nsXULElement. This is probably hard. I don't really see other options... are there any?

We have a number of bindings whose constructors expect or appear to assume to be in a document: browser, editor, listbox, menulist, progressmeter-undetermined, radiogroup, radio, scrollbar, toolbar, tabbox, tabs, control, treebody, treecol.

Then all those bindings are _already_ broken for XUL elements that are being cloned (since those elements get bindings attached immediately). They need to be fixed. Note that I just checked the radio binding, and it deals with not being in a document just fine. The constructor just does nothing. I suppose that could screw up if radiogroup doesn't handle kids being appended properly. In case it wasn't clear why this bug got filed, right now: foo = bar.cloneNode(); will attach the binding immediately, while: foo = document.createElement(bar); will not attach the binding. This patch fixes this inconsistency.

Comment on attachment 162655 [details] [diff] [review] Patch, tested on that testcase Note to self: This also breaks the little dropdown widgets in DOM inspector. Reviewers, I would still like to land this without the classinfo patch, so this capability will not be enabled quite yet. Then I'll fix the various bindings to deal and land the classinfo change.

The way the radiogroup handles it is that it assumes that the radio's binding will fire when it is added to the document, thus clearing the radiogroup's cache. So I assume that is already broken for cloned radios.

(In reply to comment #6) > Alternately, I could make nsIFrameLoaderOwner/nsIFrameLoader scriptable so that > the binding itself can implement nsIFrameLoaderOwner. Then the docshell would > exist just fine and all that (and we can remove the hack whereby we go through > the box object to get at the docshell; if we have an nsIFrameLoader pointer we > can just get the docshell off of that). We could probably also remove the hacks > we have in XBL where we force reflow on binding attachment just so XUL iframes > will work. This sounds reasonable to me.

Ugh. We can't create a frameloader if we're not in the document (because the window can't be hooked up). So the frameloader thing (while desirable in its own right) won't help much here. I've filed bug 266590 on fixing up the various XBL bindings. Once that's fixed, we can throw the classinfo switch in this bug.

Comment on attachment 162655 [details] [diff] [review] Patch, tested on that testcase Since you noted that this breaks things, minusing to get off of my list.

Comment on attachment 162655 [details] [diff] [review] Patch, tested on that testcase Brian, please see comment 9. The XBL changes on their own don't break anything and are correct, and are blocking other work on GetDocument removal and removing SetDocument/etc in favor of BindToTree(). I'd really like to get those changes in. I can post a patch without the one classinfo chunk if you prefer, but that'll be the only difference...

Comment on attachment 162655 [details] [diff] [review] Patch, tested on that testcase Ah, ok. Looks fine then.

Comment on attachment 162655 [details] [diff] [review] Patch, tested on that testcase sr=jst

Checked in the XBL part of the patch. The bug's not fixed, but the rest will have to wait for now, till bug 266590 is resolved.

If someone wants to help me on bug 266590, I'd appreciate reviews on the patch that's there. It's a partial fix to the problem of bogus constructors that's blocking this bug.

I guess thease bugs can be related with one: bug 250123 and bug 229703.

Given the latest patch in bug 53901, I think we should WONTFIX this?

For XBL1, probably yes...

And per comment 20 and comment 21...