265086 - XBL should apply to nodes not in documents

Status: RESOLVED WONTFIX

(Core :: XBL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Untested patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: XBL for testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: HTML for testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch, tested on that testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[[e-mail]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[[e-mail]]

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.

Comment 11

[Johnny Stenback (:jst)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 13

[Brian Ryner (not reading)]

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 14

[Boris Zbarsky [:bzbarsky]]

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 15

[Brian Ryner (not reading)]

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

Ah, ok.  Looks fine then.

Comment 16

[Johnny Stenback (:jst)]

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

sr=jst

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Alex Vincent [:WeirdAl]]

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.

Comment 19

[alexander :surkov (:asurkov)]

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

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

For XBL1, probably yes...

Comment 23

[Boris Zbarsky [:bzbarsky]]

And per comment 20 and comment 21...