250817 - nsWidgetStateManager assumes first element returned by getElementsByAttribute is its child

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Ian Neal]

If you have nested radio groups like below:
<radiogroup id="groupone">
  <radio value="0" label="Option A for Group 1"/>
  <radiogroup id="grouptwo" style="margin-left: 2em">
    <radio value="0" label="Option A for Group 2"/>
    <radio value="1" label="Option B for Group 2"/>
  </radiogroup>
  <radio value="1" label="Option B for Group 1"/>
</radiogroup>

where you use the same set of values for both radiogroups.

When nsWidgetStateManager.js, in the set_Radiogroup function, tries to set the
"Option B for Group 1" radio button it sets the "Option B for Group 2" button
instead because that is the first element it finds with the value of 1.

Comment 1

[Ian Neal]

Attachment: First pass on a patch to fix this issue

Checks id of parentNode to make sure it's a child and not a grandchild, etc

Comment 2

[[e-mail]]

To fix this properly you'd probably want to iterate over the radio children as
computed by radio.xml - perhaps by enhancing the "value" setter?

Comment 3

[Ian Neal]

Attachment: Patch v0.2

Borrows from rest of setters in radio.xml, not too sure what best to do with
invalid values - whether to throw an exception or just report an error and
continue with other radiogroups.

Comment 4

[[e-mail]]

Comment on attachment 153226 [details] [diff] [review]
Patch v0.2

>+            throw new Error("Invalid value for radiogroup - has to be the value of one of its children");
I don't think this is a good idea. For instance, menulists simply set the
attribute as the given value even if a new selection cannot be made. This would
then simplify your shortcut code above as you could just see if the attribute
is the same and return early if it is.

Comment 5

[Ian Neal]

Attachment: Patch v0.3

Takes on board Neil's comments
Value attribute can sometimes already have the correct value without the
corresponding item being selected so no easy way to return early.

Comment 6

[[e-mail]]

Comment on attachment 153457 [details] [diff] [review]
Patch v0.3

>+            this.setAttribute('value', val);
Hmm... is local style to use single or double quotes inside CDATA?

>+            var children = this._getRadioChildren();
>+            for (var i = 0; i < children.length; i++) {
>+              if (children[i].value == val) {
>+                this.selectedItem = children[i];
>+                return val;
break perhaps? I need to think about this during the day ;-)

>+              }
>+            }
>+            return val;

Comment 7

[Ian Neal]

Both styles are used, there may be slightly more use of " than ' but not
decisively so.

Would using break mean a tidier exit from the setter?

Comment 8

[[e-mail]]

Comment on attachment 153457 [details] [diff] [review]
Patch v0.3

I've trawled the rest of the bindings and the style does seems to lean towards
double quoted strings in CDATA and break over early return.

Comment 9

[Ian Neal]

Attachment: Patch v0.3a - Tweaked version of patch v0.3

Changed ' to " and used break instead of returning early as per Neil's comments

Comment 10

[Ian Neal]

Comment on attachment 153533 [details] [diff] [review]
Patch v0.3a - Tweaked version of patch v0.3

Carrying forward r+ and requesting sr

Comment 11

[Ian Neal]

Attachment: Firefox version of patch 0.3a

To keep things in sync if it is wanted

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 153534 [details] [diff] [review]
Firefox version of patch 0.3a

looks good.  I like when Neil reviews this stuff first, makes my job easier.

Comment 13

[Ian Neal]

toolkit version of patch (bug250817patch0_3af) checked into trunk
Checking in widgets/radio.xml;
/cvsroot/mozilla/toolkit/content/widgets/radio.xml,v  <--  radio.xml
new revision: 1.9; previous revision: 1.8
done
Checking in nsWidgetStateManager.js;
/cvsroot/mozilla/toolkit/content/nsWidgetStateManager.js,v  <-- 
nsWidgetStateManager.js
new revision: 1.8; previous revision: 1.7
done

Comment 14

[jag (Peter Annema)]

Comment on attachment 153533 [details] [diff] [review]
Patch v0.3a - Tweaked version of patch v0.3

sr=jag

Comment 15

[Ian Neal]

xpfe version of patch (bug250817patch0_3a) checked into trunk
Checking in bindings/radio.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/radio.xml,v  <--  radio.xml
new revision: 1.34; previous revision: 1.33
done
Checking in nsWidgetStateManager.js;
/cvsroot/mozilla/xpfe/global/resources/content/nsWidgetStateManager.js,v  <-- 
nsWidgetStateManager.js
new revision: 1.29; previous revision: 1.28
done

Leaving open for aviary checkin if needed.

Comment 16

[Ian Neal]

Attachment: Aviary version of patch - includes fix to bug 260357

Revised patch that includes fixes to this bug and bug 260357

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 159429 [details] [diff] [review]
Aviary version of patch - includes fix to bug 260357

let's get review before considering for approval.

Comment 18

[Ian Neal]

Comment on attachment 159429 [details] [diff] [review]
Aviary version of patch - includes fix to bug 260357

Carrying forward r's from this and bug 260357

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 159429 [details] [diff] [review]
Aviary version of patch - includes fix to bug 260357

don't need this on aviary branch, especially at this stage.

Comment 20

[Ian Neal]

Resolving as fixed as it's not required for aviary branch.

Comment 21

[Ian Neal]

Needs to be relanded once aviary branch has landed on trunk (possibly)

Comment 22

[Ian Neal]

Patch re-checked in
Checking in widgets/radio.xml;
/cvsroot/mozilla/toolkit/content/widgets/radio.xml,v  <--  radio.xml
new revision: 1.13; previous revision: 1.12
done
Checking in nsWidgetStateManager.js;
/cvsroot/mozilla/toolkit/content/nsWidgetStateManager.js,v  <-- 
nsWidgetStateManager.js
new revision: 1.10; previous revision: 1.9
done

Comment 23

[Ian Neal]

Fixed