367009 - Radio elements always assume to be in a radiogroup

Status: RESOLVED WORKSFORME

(Core :: XUL, defect)

Description

[Wladimir Palant]

The constructors and destructors of <radio> elements in XUL always assume that they are in a document and part of a <radiogroup>. If this assumption fails (which it regularly does) we get the following error for the constructor:

Error: null has no properties
Source File: chrome://global/content/bindings/radio.xml
Line: 383

And that one for the destructor:

Error: this.radioGroup has no properties
Source File: chrome://global/content/bindings/radio.xml
Line: 388

The line numbers above are for Firefox 2.0.0.1, this is an issue on trunk as well however. In Firefox 1.5 this is less of an issue since bindings are only applied when the node is inserted into a document - simply cloning a radio element won't cause these errors.

Comment 1

[Wladimir Palant]

Attachment: Testcase

This testcase has a radio element without a radiogroup in the document. When it loads, another radio element is created using cloneNode() - not inserted into the document however. Both elements cause the errors mentioned above, and these errors cannot be caught.

Comment 2

[Wladimir Palant]

Attachment: Testcase for broken functionality

This tests the functionality that the code in the constructor/destructor should provide but doesn't. We make sure that radiogroup.mRadioChildren is initialized by setting the value property. When we add another radio element mRadioChildren should be cleared - but it doesn't in this case, consequently setting value to the new element fails, also the alert with radiogroup._getRadioChildren().length shows 1 instead of the expected 2 children.

Comment 3

[Wladimir Palant]

Attachment: Proposed patch

Moving the operations to update mRadioChildren into the radiogroup itself, triggered by DOMNodeInserted and DOMNodeRemoved events instead of XBL construction. Both testcases work with this change, also removing a radio element from the group removes it from mRadioChildren as well.

Comment 4

[[e-mail]]

bz, do mutation events still send perf through the floor these days?

trev, some thoughts:
1. Just unconditionally clear the cache, without checking the inserted node
2. Listen for the mutation events on the radio itself (bz: does this work?)
3. Write C++ code to special-case mutation events targeted on the element
   (i.e. without having to turn on mutation events globally).

Comment 5

[Wladimir Palant]

Neil, the node itself doesn't receive mutation events, only its parent nodes. Looking at http://www.w3.org/TR/DOM-Level-3-Events/events.html#event-DOMNodeInserted I don't see why it should behave like this so this is probably a bug itself.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> bz, do mutation events still send perf through the floor these days?

Good question.  Sicking's cleaned them up some, but DOM event dispatch is pretty expensive.  So I suspect that adding a listener for some operation does significantly slow down that operation.  One could test to find the number, I suppose.

> Listen for the mutation events on the radio itself (bz: does this work?)

It should.  If it doesn't, we should fix it.  Last I checked this did work, though I wasn't testing XUL.

Is there a testcase that shows it not working?  Also, does it depend on whether a capturing handler is used (not that it should, in our impl; just checking)?

Comment 7

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

Mutation events are unfortunately still very slow, so we should try to avoid them.

Comment 8

[Wladimir Palant]

Attachment: Testcase for mutation events

This tests whether mutation events fire for the container and the child being added/removed. Actually, when we have listeners attached to the container the events fire for the child as well. If you comment out these listeners, the listeners for the child won't fire any more.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Indeed.  Filed bug 367164 on that issue.

Comment 10

[Wladimir Palant]

(In reply to comment #4)
> trev, some thoughts:
> 1. Just unconditionally clear the cache, without checking the inserted node
> 2. Listen for the mutation events on the radio itself (bz: does this work?)
> 3. Write C++ code to special-case mutation events targeted on the element
>    (i.e. without having to turn on mutation events globally).

Sorry Neil, that will need more time than I can spend at the moment. Feel free to take this bug.

Comment 11

[[e-mail]]

Comment on attachment 251502 [details] [diff] [review]
Proposed patch

Minusing as per comment #7.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #4)

> 3. Write C++ code to special-case mutation events targeted on the element
>    (i.e. without having to turn on mutation events globally).
> 

Can you clarify this? I suppose http://lxr.mozilla.org/mozilla/source/content/base/src/nsContentUtils.cpp#2844 is too slowly, right? How it can be improved for special-case mutation events targeted on the element?

Comment 13

[[e-mail]]

I mean that it may be possible for the XBL binding manager to synthetically trigger handlers even though mutation events haven't been enabled.

Comment 14

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

Yes, it should be possible to add code to XBL that implements some mutation events without registering a true mutation-event-handler (and thus triggering the global flag). I plan to do so for XBL2, but I haven't looked into how doable it is in the current XBL1 implementation.

Comment 15

[Alex Vincent [:WeirdAl]]

Please note bug 209555, which I believe this is a dupe of.

Comment 16

[:aceman]

Can you see if the issue was fixed via bug 209555 and we can close this?

Comment 17

[Wladimir Palant]

I worked around that issue a long time ago but judging from the current radio.xml code the issue is indeed resolved. The relevant changes are bug 371260 and bug 606215, bug 209555 on the other hand addressed a different issue. So - WORKSFORME.