92358 - Nested XUL radio button groups don't work.

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Steve Meredith (gone)]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.2+)
Gecko/20010717
BuildID:    20010725

If you have a group of radio buttons inside another group of radio buttons,
clicking on the buttons in the nested group unselect the buttons in the outer group.

I am going to attach a patch with a simple change to
/xpfe/browser/samples/widgets.xul to demonstrate the problem.

Reproducible: Always
Steps to Reproduce:
Create a set of nested radio buttons in XUL, to looks something like this:

( ) Radio 1
(*) Radio 2
    (*) SubRadio A
    ( ) SubRadio B
( ) Radio 3

with the 2 sets of radio buttons in different groups. 

Select Radio 2
Click on SubRadio A


Actual Results:  As soon as you click on SubRadio A, Radio 2 gets unselected. 

Expected Results:  Clicking on SubRadio A should not unselect anything in the
outer group.

I need this to work to implement bug 90989.

Comment 1

[Steve Meredith (gone)]

Attachment: Patch to xpfe/browser/samples to show this defect.

Comment 2

[John Morrison]

Hmm, so the radiogroup binding does not anticipate a nested radiogroup, so when
the click bubbles up from the inner group to the outer group, the outer group
assumes that the radio was a member of its collection. Seems, that it would be 
possible to extend the filter on the click (and mousedown and other) handler
to check that the group attribute of the event target does actually match the 
id of the radiogroup that is catching this event. 

Comment 3

[John Morrison]

Attachment: functioning example of nested radiogroups

Comment 4

[John Morrison]

Attachment: use this one; "correct" but stupid vnd mimetype

Comment 5

[Mike Pinkerton (not reading bugmail)]

*pink spins the bottle* --> jag!

Comment 6

[Steve Meredith (gone)]

I am attaching a patch that fixes this. It works, but there might be a better 
way. 

Comment 7

[Steve Meredith (gone)]

Attachment: Proposed patch to stop the event from bubbling.

Comment 8

[John Morrison]

Hmm. I don't think you want to unilaterally kill event bubbling out of this 
widget. I think the right fix is what I noted above ("check that the group 
attribute of the event target does actually match the id of the radiogroup 
that is catching this event.") jag/blake: does that seem correct?

Comment 9

[Steve Meredith (gone)]

Attachment: A better fix.

Comment 10

[Steve Meredith (gone)]

Oh, you mean like this? (See attached patch.) Why didn't you say so. ;-)

Comment 11

[John Morrison]

Yeah, that's the ticket :-]

I might be inclined to do it this way, since if it's not a radio element,
then it doesn't matter if the 'group' attribute matches the radiogroup's id.
[Excuse the bugzilla line wrapping ...]

Index: radio.xml
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/radio.xml,v
retrieving revision 1.3
diff -u -r1.3 radio.xml
--- radio.xml   2001/07/25 06:14:25     1.3
+++ radio.xml   2001/08/04 02:04:11
@@ -129,7 +129,9 @@
     <handlers>
       <handler event="click" button="0">
         <![CDATA[
-            if (event.target.localName == "radio" && !event.target.disabled)
+            if (event.target.localName == "radio" &&
+                event.target.getAttribute("group") == this.id && // nested 
radiogroups?
+                !event.target.disabled)
               this.selectedItem = event.target;

          ]]>

blake/jag, comments, r=/sr=, etc.?

Comment 12

[Blake Ross]

seems okay to me, sr=blake

Comment 13

[jag (Peter Annema)]

r=jag on john's version of the patch. Please make sure this works in both modern
and classic.

Comment 14

[Steve Meredith (gone)]

Checked in the patch.

Comment 15

[[e-mail]]

Just out of interest, why does the radiogroup handle the click? Why not handle
the click on the radio element?

Comment 16

[[e-mail]]

Should the mousedown event be updated? It may not be necessary.

Index: radio.xml
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/bindings/radio.xml,v
retrieving revision 1.5
diff -u -r1.5 radio.xml
--- radio.xml   2001/08/16 03:14:18     1.5
+++ radio.xml   2001/08/17 08:08:41
@@ -140,7 +140,10 @@

       <handler event="mousedown" button="0">
         <![CDATA[
-            if (event.target.localName == "radio" && !event.target.disabled)
+
+            if (event.target.localName == "radio" &&
+                event.target.group == this.id &&
+                !event.target.disabled)
               this.focusedItem = event.target;

          ]]>