Open
Bug 266590
Opened 20 years ago
Updated 2 years ago
Many XUL bindings make bogus assumptions about when constructors fire
Categories
(Core :: XUL, defect, P5)
Tracking
()
NEW
mozilla1.9alpha1
People
(Reporter: bzbarsky, Unassigned)
References
Details
(Keywords: helpwanted)
Attachments
(1 file, 1 obsolete file)
9.14 KB,
patch
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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•20 years ago
|
||
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.
Reporter | ||
Comment 3•20 years ago
|
||
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•20 years ago
|
||
(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)
Reporter | ||
Comment 5•20 years ago
|
||
> 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•20 years ago
|
||
Question: are destructors also a possible concern?
Updated•20 years ago
|
Assignee: nobody → ajvincent
Comment 7•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
(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•20 years ago
|
||
IIRC some themes expect it to be set.
Comment 11•20 years ago
|
||
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.
Attachment #164141 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #165935 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 12•20 years ago
|
||
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•20 years ago
|
||
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.
Reporter | ||
Comment 14•20 years ago
|
||
So it's sounding like we really want an "oninsertionintodocument" notification
and want to move most of this junk into that?
Comment 15•20 years ago
|
||
That would be ideal, bz. How does your idea differ from
NodeInsertedIntoDocument, though?
neil: you can still review the patch that we have already... :)
Reporter | ||
Comment 16•20 years ago
|
||
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•20 years ago
|
||
Document insertion/removal? notifications would simplify things considerably.
Comment 18•20 years ago
|
||
When will "oninsertionintodocument" event be fired? After node insertion into
document only or after node insertion into document and applying of binding?
Reporter | ||
Comment 19•20 years ago
|
||
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•20 years ago
|
||
That sounds wonderful, but we probably need something for node removal too...
Reporter | ||
Comment 21•20 years ago
|
||
Aren't bindings detached on node removal at the moment? I don't really plan to
change that...
Comment 22•20 years ago
|
||
Ok, that's fine then.
Comment 23•20 years ago
|
||
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•19 years ago
|
||
:( 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?
Status: NEW → ASSIGNED
Reporter | ||
Comment 25•19 years ago
|
||
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•19 years ago
|
||
(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•18 years ago
|
||
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.
Flags: blocking1.9a1?
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Updated•18 years ago
|
Keywords: helpwanted
Updated•18 years ago
|
Flags: blocking1.9a1? → blocking1.9-
Comment 28•18 years ago
|
||
So regarding comment 14, maybe we want to cherry-pick http://www.mozilla.org/projects/xbl/xbl2.html#handling from XBL2?
Reporter | ||
Comment 29•18 years ago
|
||
Yeah. Now that XBL2 has decided on an approach I absolutely think we should cherry-pick it.
Updated•17 years ago
|
Assignee: ajvincent → nobody
Status: ASSIGNED → NEW
QA Contact: xptoolkit.xul
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Comment 30•15 years ago
|
||
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.
Attachment #165935 -
Flags: review?(neil)
Comment 31•6 years ago
|
||
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.
Priority: P1 → P5
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•