A number of the tookit/XPFE bindings in our tree assume that the constructor won't fire until the node is inserted in the document. This is already false when the node in question is created via cloneNode(), so every single one of the affected bindings is already broken in some cicrumstances. They will break in more circumstances once bug 265086 is fixed. Unfortunately, that breakage will be severe enough that it really needs fixing preemptively before I can fix that bug. Perhaps we need some sort of event/notification that will be dispatched to the binding when the node is inserted into the tree (when the C++ XBL binding code gets a DocumentChanged() notification, iirc)? I suppose we can try to use mutation listeners for this, but I think they won't fire when some ancestor of the node is inserted... Most of the bindings involved can do without that, I think, but it'll be needed for browser/iframe. The list of affected or possibly affected bindings, per bug 265086 comment 7 is: browser, editor, listbox, menulist, progressmeter-undetermined, radiogroup, radio, scrollbar, toolbar, tabbox, tabs, control, treebody, treecol Now I don't know this code very well, so any help I can get from people who do would be much appreciated... This bug and bug 265086 make XUL-the-platform somewhat less predictable than it should be for would-be developers...

Very high interest for me; feel free to assign to me (though I may not be able to fix all of this). Testcases would be welcomed.

I didn't search the entire tree for bindings when I wrote bug 265086 comment 7. These are the assumptions that I spotted in xpfe global bindings: browser, editor, progressmeter-undetermined and scrollbar (Mac only) assume that they can access their box object. listbox, menulist, radiogroup, tabs assume that their children exist. radio, toolbar, tabbox, treebody, treecol assume that their parent exists. label-control assumes that its control exists.

The more I think about it, the more I think we need a "inserted into document" event (to be fired right after the constructor if the node is already in the document, or upon insertion in the document if it's not). I've sent mail to www-svg suggesting this for sXBL.

(In reply to comment #3) > The more I think about it, the more I think we need a "inserted into document" > event Don't we already have that? http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html#Events- MutationEvent (scroll down about two pages)

> Don't we already have that? Not in a useful way. We don't implement NodeInsertedIntoDocument, to be precise. More importantly, mutation events are rather a performance drag in Mozilla. I don't think we want to use them here.

Question: are destructors also a possible concern?

This patch is not for a formal review. I'd like feedback on whether the changes made herein are the sort you're looking for, or if I'm doing something you do not want to have happen. I'm particularly concerned about changes to tabbox.xml, where instead of throwing an exception with no child nodes, I've modified the code to return -1. Other minor things tweaked, including * partial inline code documentation a la javadoc/doxygen * slight optimization of DOM API usage in tabbox.xml

Comment on attachment 164141 [details] [diff] [review] Early work in progress (radio.xml, tabbox.xml) >+ >+ // Force update of this.mRadioChildren >+ void(this.resetRadioChildren()); >+ The whole point of this._getRadioChildren() was that we didn't update this.mRadioChildren until we actually needed it; if we knew (via the bogus radio binding assumption) that the list of child nodes had changed then we simply cleared the cache until such time as we needed it. Furthermore these are just convenience methods, there is no requirement for radio buttons to be direct descendents of radio groups. Look at pref-cookies.xul for example. >- if (!this.disabled) >- this.radioGroup.selectedItem = this; >+ var group = this.radioGroup; >+ if ((!this.disabled)&&(group)) >+ group.selectedItem = this; I think we can assume that a clickable radio is in a document... > <constructor> > switch (this.getAttribute("eventnode")) { > case "parent": this._eventNode = this.parentNode; break; > case "window": this._eventNode = window; break; > case "document": this._eventNode = document; break; > } >- this._eventNode.addEventListener("keypress", this._keyEventHandler, false); >+ if (this._eventNode) >+ this._eventNode.addEventListener("keypress", this._keyEventHandler, false); Even a newly constructed tabbox will always have an eventNode (since the attribute will not have been set yet its eventNode default to itself). >+ // XXX should this be this.hasAttribute? > var o = this.getAttribute("orient"); > if (!o) > this.setAttribute("orient", "horizontal"); You could do that, or you could just use this.orient instead (XUL element property, see nsIDOMXULElement.idl). >+ this.selectedIndex = -1; // no tab selected (this can happen if there are no child nodes) Tabs are special... remember we have a deck to switch too.

(In reply to comment #8) :p This is why I stopped to get a sanity check. :) > >+ // XXX should this be this.hasAttribute? > > var o = this.getAttribute("orient"); > > if (!o) > > this.setAttribute("orient", "horizontal"); > You could do that, or you could just use this.orient instead (XUL element > property, see nsIDOMXULElement.idl). Well, why are we setting the orient attribute at all, then?

IIRC some themes expect it to be set.

radiogroup, radio, tabbox, tabs, toolbar, treebody, treecol-base are fixed herein. I did not see any problems with menulist. listbox I'm uncertain of. The others I really don't have much of an understanding of at this point, on where the problems manifest themselves.

Unfortunately the whole point of many of these constructors is that they need to find parent/child objects which will never be the case if they fire on creation.

As for the other elements, they also assume a document, although for other reasons; progressmeter-undetermined assumes that it can find out its dimensions and browser and editor assume they have a subdocument, while mac scrollbars use their box objects to access the look and feel preference.

So it's sounding like we really want an "oninsertionintodocument" notification and want to move most of this junk into that?

That would be ideal, bz. How does your idea differ from NodeInsertedIntoDocument, though? neil: you can still review the patch that we have already... :)

NodeInsertedIntoDocument is a mutation event. Hence inherently slower than doing a function call on the binding (which would not be a real DOMEvent, hence would not capture or bubble or any of that jazz). also, NodeInsertedIntoDocument would need to be implemented, since we don't implement it. My suggestion would be an XBL-only hack for now...

Document insertion/removal? notifications would simplify things considerably.

When will "oninsertionintodocument" event be fired? After node insertion into document only or after node insertion into document and applying of binding?

I was thinking not an event but something like <initializer> (similar to <constructor>). It would be called when the binding has been attached and the node is in the document (triggered by the latter of these to happen). Neil, does that seem reasonable?

That sounds wonderful, but we probably need something for node removal too...

Aren't bindings detached on node removal at the moment? I don't really plan to change that...

Ok, that's fine then.

I guess it's needed to add more convenient events to fix this bug. I guess it's comfortable to have events like "childrenload", "parentload". Maybe it's needed to add parameters to constructor of binding, in instance, call-after-children-loading-with-listitem-tagname and so-on. If developers will have such tools then bug https://bugzilla.mozilla.org/show_bug.cgi?id=231913 can be closed.

:( This patch has sat for over a year, and now probably is not good enough; it doesn't cover toolkit code. Neil, bz, any thoughts on getting this reviewed? Or should I try to submit a new patch?

Seems to me like we should decide whether we need back end support first. If so, do it. If not, then go ahead and start patching bindings.

(In reply to comment #25) >Seems to me like we should decide whether we need back end support first. To do that I need to know what we can do about the assumptions from comment 2.

This bug needs a new owner, or at least someone to knock the heads of the people involved (including me) and drive this effort to completion. bz has been blocked for 18 months on a r+/sr+'d patch because of this bug. This is inexcusable, and totally my fault. I'd propose that we refresh the bindings, go ahead & start patching bindings and take our lumps on the head with regard to trunk builds. That is, we fix regressions as they come up.

So regarding comment 14, maybe we want to cherry-pick http://www.mozilla.org/projects/xbl/xbl2.html#handling from XBL2?

Yeah. Now that XBL2 has decided on an approach I absolutely think we should cherry-pick it.

Comment on attachment 165935 [details] [diff] [review] patch, v1 no way is a patch five years old going to get an r+, unless it's trivial.

Decreasing the priority as no update for the last 2 years on this bug. See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage about the priority meaning.