277012 - RadioStateChange event doesn't fire for select-losing <radio>

Status: VERIFIED DUPLICATE of bug 320369

(Core :: Widget, defect)

Description

[Ben Bucksch (:BenB)]

The official ;-) reference doc
<http://www.xulplanet.com/references/elemref/ref_radiogroup.html#prop_selectedItem>
as well as the name implies that the RadioStateChange event should fire twice
during a change - once for the newly selected <radio> and once for the <radio>
which was previously selected. That would be needed anyways (because these
events fire after the change has been performed, you can check the state in the
handler via event.target.selected).

However, currently, only the newly selected radio gets the event. The radio
losing the select doesn't notice it.

Attaching a patch to fix it.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix

Comment 2

[Ben Bucksch (:BenB)]

> The official ;-) reference doc
>
<http://www.xulplanet.com/references/elemref/ref_radiogroup.html#prop_selectedItem>
> as well as the name implies

Actually, re-reading the xulplanet sentence, it says the *opposite*, but the
name of the event implies to me that it should fire for both, plus there are
good use cases for it.

Comment 3

[Aaron Leventhal]

Comment on attachment 170252 [details] [diff] [review]
Fix

Nit: I think it would more intuitive to fire the state change event for the old
radio button first.

Comment 4

[Aaron Leventhal]

In fact, if you do that, you don't need to cache oldSelected. You can instead
just fire the event where you are currently setting that.

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Fix, v2

> Nit: I think it would more intuitive to fire the state change event
> for the old radio button first.

Agreed (and I think it's more than a nit, it matters for app developers)

> In fact, if you do that, you don't need to cache oldSelected. You can instead

> just fire the event where you are currently setting that.

I need to set it up, too. Easier (smaller change) this way IMHO.

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Proposed Fix, v3

While working with it, I needed to remove that |if (focused)|, too, because
that prevented the event from firing when the selection got changed
programmatically in onload and co. It's something I could work around, but I
guess typical uses may want to see that.

Comment 7

[Serge Gautherie (:sgautherie)]

Ben, are still working on this ?

***

</xpfe/global/resources/content/bindings/radio.xml>
is obsolete.
</toolkit/content/widgets/radio.xml>
is the one to fix now.

Is a v3 patch like was is wanted ?

Comment 8

[Samuel Sidler (old account; do not CC)]

Not wanted for 1.9.0.x. We've lived with this for a long time. Try requesting blocking/wanted1.9.1.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 170267 [details] [diff] [review]
Proposed Fix, v3

This patch has been sitting around for years.  Neil, what do you think should happen here?

(Note that the XPFE version of this file no longer exists, but the patch looks like it applies (roughly, at least) to toolkit/content/widgets/radio.xml .)

Comment 10

[Neil Deakin]

This has already been fixed by bug 320369.

Comment 11

[Serge Gautherie (:sgautherie)]

V.Duplicate

Yet, I filed bug 456038 as a followup...