266590 - Many XUL bindings make bogus assumptions about when constructors fire

Status: NEW

(Core :: XUL, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Alex Vincent [:WeirdAl]]

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.

Comment 2

[[e-mail]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Alex Vincent [:WeirdAl]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Alex Vincent [:WeirdAl]]

Question: are destructors also a possible concern?

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: Early work in progress (radio.xml, tabbox.xml)

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 8

[[e-mail]]

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.

Comment 9

[Alex Vincent [:WeirdAl]]

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

Comment 10

[[e-mail]]

IIRC some themes expect it to be set.

Comment 11

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

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.

Comment 12

[[e-mail]]

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.

Comment 13

[[e-mail]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Alex Vincent [:WeirdAl]]

That would be ideal, bz.  How does your idea differ from
NodeInsertedIntoDocument, though?

neil: you can still review the patch that we have already... :)

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[[e-mail]]

Document insertion/removal? notifications would simplify things considerably.

Comment 18

[alexander :surkov (:asurkov)]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[[e-mail]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[[e-mail]]

Ok, that's fine then.

Comment 23

[alexander :surkov (:asurkov)]

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.

Comment 24

[Alex Vincent [:WeirdAl]]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[[e-mail]]

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

Comment 27

[Alex Vincent [:WeirdAl]]

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.

Comment 28

[Peter Van der Beken [:peterv]]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[Alex Vincent [:WeirdAl]]

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.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

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.