209555 - <radio> must be children of <radiogroup>

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Ben Bucksch (:BenB)]

The way <radio> and <radiogroup> currently are speced, the <radio> has to be a
children of <radiogroup>. This is not practical in some advanced layouts, where
I may not way to use the standard sequential layout, but arrange the radios in
some other way, but for them to still have the same logic.

For example, I needed to have a grid with each row being a radiogroup, but some
of the cells each being a radio. (not several radios in the same cell.) (After
several hours of unsuccessful attempts, Neil showed me a totally unobvious
workaround using style to make radiogroups a <row>.)

This is just one example, there are lots of other thinkable layouts where the
current assumption in the spec is not true.

So, give me a way to explicitly assign a radio to an arbitary radiogroup using
an attribute. E.g.

<radiogroup id="a" />
... lots of other XUL elements ...
<radio group="a" value="1" label="a1" />
... lots of other XUL elements ...
<radio group="a" value="2" label="a2" />

Should not be very hard to implement. I tried to create a new pair of elements,
inheriting from the existing ones, it should have worked, but it didn't, most
likely due to my lackign XBL knowledge. FWIW, the file in question is
xpfe/global/resources/content/binding/radio.xml.

Filing as bug, because I think the current way is not workable.

Comment 1

[Ben Bucksch (:BenB)]

seems like blake *deliberately* broke this, costing me almost a day or work
(more, if Neil didn't help me). It seems like there used to be *exactly* the
attribute I proposed, but blake decided that it's "unnecessary or nonsensical"
<http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/global/resources/content/bindings/radio.xml>,
rev 1.7.

Comment 2

[Ben Bucksch (:BenB)]

(Sorry for being so agressive, I was just pissed about the unecessarily lost time.)

Comment 3

[[e-mail]]

document.getElementsByAttribute("group", this.id); might be a perf issue.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, with the current impl of getElementsByAttribute.  If it were redone to be
like other content lists (live, and cached), it wouldn't be nearly as bad.

Comment 5

[Alex Vincent [:WeirdAl]]

This bug is four years old now, with no patches.  Anyone planning on working on this?

Comment 6

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Like this.  (I also tightened up the radio bindings so that they'd check namespace URI's as well.)

Comment 7

[Alex Vincent [:WeirdAl]]

If you want a mochikit test, I can provide that too.  I have tested this on my own product, and it works.

Comment 8

[Neil Deakin]

Comment on attachment 268611 [details] [diff] [review]
patch, v1

>Index: toolkit/content/widgets/radio.xml
>-        </setter>        
>+        </setter>

Don't make the extra whitespace changes in the patch.

>+          // We don't have child nodes.
>+          radioChildren = [];

The array is already cleared earlier, so no need for this line.

>+          }
>+          this._radioChildren = radioChildren;
>+          return radioChildren;

Could use the same pattern as the earlier return statement.

>       <property name="control" readonly="true">
>+          var group = this.getAttribute("group");
>+          parent = this.ownerDocument.getElementById(group);

Add a null check for group here so an exception won't occur.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: revised patch, answering review nits

Comment 10

[Shawn Wilsher :sdwilsh]

Checking in toolkit/content/widgets/radio.xml;
new revision: 1.27; previous revision: 1.26

Comment 11

[[e-mail]]

(In reply to comment #8)
>>+          this._radioChildren = radioChildren;
>>+          return radioChildren;
>Could use the same pattern as the earlier return statement.
Or even use an else so that you share the return between the two versions.

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> If you want a mochikit test, I can provide that too.

Yes, it would be nice to have one.

Comment 13

[Eric Shepherd [:sheppy]]

Documentation has been updated.