127118 - Pressing access key for radio button doesn't focus radio group or show focus on radio button

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[:deanis74]

Steps:

1. Open the test case from bug 989 (attachment 59811 [details])
2. Press Alt+B
3. Watch radio1 select and get the focus indicator around it
4. Press Alt+C

Expected Results: radio2 is selected and gets the focus indicator

Actual Results: radio2 is selected but no focus indicator.  If I press Alt+C
again, the focus indicator is drawn.

Comment 1

[[e-mail]]

I've just noticed that radio buttons never get focussed since at least 1.4
(although to be precise it's the radio group that should get focussed).

Comment 2

[Aaron Leventhal]

Attachment: For mouse down or focus event on a radio button, we need focus the radio group, then select the button

Comment 3

[[e-mail]]

Comment on attachment 150347 [details] [diff] [review]
For mouse down or focus event on a  radio button, we need focus the radio group, then select the button

>       <handler event="mousedown" button="0">
>         <![CDATA[
>-          if (!this.disabled)
>+          if (!this.disabled) {
>+            this.radioGroup.focus();
>             this.radioGroup.focusedItem = this;
>+          }
>+         ]]>
>+      </handler>
I don't see why you need this change; clicking a radio button with the mouse
already focuses the group.

>+      <handler event="focus">
>+        <![CDATA[
>+          if (!this.disabled) {
>+            this.radioGroup.focus();
>+            this.radioGroup.focusedItem = this;
>+          }
>          ]]>
>       </handler>
>     </handlers>
I'm not sure why you need to set the focused item but if you do you should set
it before focussing the group.

Comment 4

[Aaron Leventhal]

Why do we make it possible to have a different focusedItem and selectedItem? We
should ensure that they're the always same. That's just how radio buttons work
-- you focus a radio button and it gets selected. Likewise, you select a radio
button and if the group has focus, the radio button also gets focus. It stays in
sync.



(In reply to comment #3)
> (From update of attachment 150347 [details] [diff] [review])
> >       <handler event="mousedown" button="0">
> >         <![CDATA[
> >-          if (!this.disabled)
> >+          if (!this.disabled) {
> >+            this.radioGroup.focus();
> >             this.radioGroup.focusedItem = this;
> >+          }
> >+         ]]>
> >+      </handler>
> I don't see why you need this change; clicking a radio button with the mouse
> already focuses the group.
> 
> >+      <handler event="focus">
> >+        <![CDATA[
> >+          if (!this.disabled) {
> >+            this.radioGroup.focus();
> >+            this.radioGroup.focusedItem = this;
> >+          }
> >          ]]>
> >       </handler>
> >     </handlers>
> I'm not sure why you need to set the focused item but if you do you should set
> it before focussing the group.
> 

Comment 5

[[e-mail]]

(In reply to comment #4)
>Why do we make it possible to have a different focusedItem and selectedItem?
Because focus and selection happen on different events, for example,
Checkbox: mousedown becomes focused; mouseup becomes toggled.
Radio: mousedown becomes focused; mouseup becomes selected.

Comment 6

[Aaron Leventhal]

Attachment: Add focus handler. This fixes accesskey problems with xul:radio

Actually in my mind focusedIndex and selectedIndex should be merged into one
variable.

Right now the esm code does this when an accesskey is fired on a xul:radio - 
     element->Focus();
     element->Click();

In reality doing one or the other should also work. So, perhaps this patch
should be changed to either 1) set both selectedIndex and focusedIndex, and
then focus the group or 2) merge them into 1 variable.

Comment 7

[Aaron Leventhal]

Attachment: New patch setting focusedItem not selectedItem

Comment 8

[[e-mail]]

Comment on attachment 150359 [details] [diff] [review]
New patch setting focusedItem not selectedItem

Hmm... this file is nicely inconsistent as to whether to leave blank lines
between handlers...

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 150359 [details] [diff] [review]
New patch setting focusedItem not selectedItem

sr=bryner

Comment 10

[Aaron Leventhal]

Checking in xpfe/global/resources/content/bindings/radio.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/radio.xml,v  <--  radio.xml
new revision: 1.33; previous revision: 1.32
done
Checking in toolkit/content/widgets/radio.xml;
/cvsroot/mozilla/toolkit/content/widgets/radio.xml,v  <--  radio.xml
new revision: 1.7; previous revision: 1.6
done

Comment 11

[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