Focusing radio buttons causes them to automatically be checked (changed)

RESOLVED FIXED in Firefox 66

Status

()

defect
P1
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: MarcoZ, Assigned: Jamie)

Tracking

(Blocks 1 bug)

Trunk
mozilla66
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox64 unaffected, firefox65 disabled, firefox66 fixed)

Details

Attachments

(2 attachments)

This is after the fix for bug 1429940. Steps to reproduce, with the NVDA screen reader:

1. Open Settings. For my example, I used the tracking protection radio buttons.
2. Switch to NVDA's browse mode (virtual cursor) by pressing Escape and a deep beep tone is heard.
3. Arrow down through the options.

Expected: NVDA should read the different radio buttons for Standard or Strict, but the selected radio button should be unchanged when the browser focuses the second radio button. The status should only change if the user presses Enter or Space when the virtual cursor is on that second radio button.

Actual: As soon as the virtual cursor is on that second radio button, NVDA causes Firefox to set focus to it. This automatically sets this radio button to checked, changes the check mark. This is not standard behavior, as on normal web sites, simply focusing a different radio button does not change its state. This only happens if in focus mode and changing the radio buttons with the arrow keys. The Settings dialogs should behave in the same way.

Comment 1

5 months ago
I'd like to debug this but I can't open nightly preferences while nvda is installed, it crashes Firefox: https://crash-stats.mozilla.org/report/index/f575a5a0-4947-488c-bfc6-2614e0181217

I expect this is a bug in the a11y code that deals with XUL radio buttons.
Component: Preferences → Disability Access APIs
Product: Firefox → Core

Updated

5 months ago
Depends on: 1513332

Comment 2

5 months ago
(In reply to Marco Zehe (:MarcoZ) [Gone until Jan 7, 2019] from comment #0)
> This is after the fix for bug 1429940.

If this worked before, it may be surfaced by the change to role="document".

Gijs, now that bug 1513332 is fixed, do you think you can debug this?

Comment 3

5 months ago
(In reply to :Paolo Amadini from comment #2)
> (In reply to Marco Zehe (:MarcoZ) [Gone until Jan 7, 2019] from comment #0)
> > This is after the fix for bug 1429940.
> 
> If this worked before, it may be surfaced by the change to role="document".

Which change is this? Because I'm pretty sure for Firefox windows we should be using role=application, if we're assigning any roles...
Flags: needinfo?(paolo.mozmail)

Comment 4

5 months ago
See bug 1429940 comment 36, and also bug 1429940 comment 39 specifically on the document role.
Flags: needinfo?(paolo.mozmail)
Assignee

Comment 5

5 months ago
(In reply to :Paolo Amadini from comment #2)
> If this worked before, it may be surfaced by the change to role="document".

To clarify, this only wasn't an issue before because the only way that users focused these controls was by tabbing. Now that we have role="document" (which provides many benefits such as heading navigation), a user might use screen reader navigation commands (rather than tabbing) to move through controls, which will cause them to be focused programmatically. It is here where the issue occurs. A radio button should accept focus without changing its state. This is certainly true for HTML <input type="radio">, but it seems that XUL <radio> misbehaves in this respect.

Looking at this briefly, I think this is actually a problem in the a11y code. The big difference between HTML and XUL radio buttons is that for XUL radio buttons, the container widget (<radiogroup>) gets DOM focus and the selected item is a separate property. In contrast, HTML radio buttons are themselves focusable. A11y deals with this by having TakeFocus code which checks if there is a "container widget", then setting the "current item" on that container widget:
https://searchfox.org/mozilla-central/source/accessible/generic/Accessible.cpp#705

I'm not sure if it's possible for a XUL radiogroup to "focus" a radio button which isn't checked. If it is, we should teach a11y how to do that. If not, the best option would be to have a11y refuse to focus radio buttons which aren't checked. That would technically be a regression, but IMO, a regression is better than non-standard behaviour, especially where that behaviour unexpectedly changes user values without the user being aware.
Assignee

Comment 6

5 months ago
Ug. It looks like XUL radiogroup does have a concept of "focusedItem", but that isn't exposed through an XPCOM interface. (nsIDOMXULSelectControlItemElement has selectedItem, but not focusedItem.)

Brian, I'd love your thoughts on this. I see three possible solutions here:
1. Expose focusedItem via XPCOM. That seems like a lot of work using what I understand to be considered legacy tech.
2. Make the XUL radiogroup custom element code set and observe changes to aria-activedescendant.
3. Make the a11y code manage the focused attribute on <radio> children directly.
I'd prefer option 2, but that's also not my area of the code. :)
Flags: needinfo?(bgrinstead)
Assignee

Comment 7

5 months ago
P1 because this could seriously hurt or confuse users using this new functionality. This will need an uplift to 65 because bug 1429940 landed in 65.
Priority: -- → P1
Assignee

Updated

5 months ago
Blocks: xula11y
Assignee

Comment 8

5 months ago
If we go with aria-activedescendant, we need to:
1. Add code to MozRadioGroup to ensure all children have ids, since aria-activedescendant needs an id. That might mean auto-generating ids for children that don't have them. This should probably be done in MozRadiogroup init.
2. Set aria-activedescendant in MozRadiogroup set selectedItem and MozRadiogroup set focusedItem.
3. Add an observedAttributes getter to MozRadiogroup which returns ["aria-activedescendant"].
4. Add an attributeChangedCallback to MozRadiogroup which sets .focusedItem based on the id set for aria-activedescendant. A11y will set this when a client asks a radio button to take focus.
5. Override XULRadioGroupAccessible::CurrentItem and  XULRadioGroupAccessible::SetCurrentItem to call the base Accessible::CurrentItem and  Accessible::SetCurrentItem methods respectively, as these do the aria-activedescendant stuff.
6. Verify that when a XUL <radio> is focused but not selected, pressing space to check the button causes an accessible state change event. If it doesn't, we might need some additional tweaks for event handling.

Comment 9

5 months ago
Thanks, that helps a lot! What you suggest in comment 8 seems to similar to what "tabbox" does, but in the short term it looks more complicated than option 1 from comment 6. In general, by looking at some of the code it seems to me that basically every XUL control that handles selection (menus, lists, radio buttons, tabs) uses a totally different implementation strategy.

I'd be interested in what Brian thinks, but probably in the long run we should unify all of these to something that resembles HTML more. It might be much easier to do this once everything has been converted to Custom Element rather than trying now with a mix of XBL and CE, so I wouldn't be too concerned with exactly which intermediate solution we come up with for radiogroup.

I have a tentative patch for what might be option 1, but I'm not sure how to test it and I don't know if it handles the space key for selecting the item. Jamie, can you help with testing? This seems closer to what nsIDOMXULMultiSelectControlElement already does when navigating lists, but without all the state handling for multiple selection.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED

Comment 11

5 months ago
I'm actually waiting on input from Jamie, who might be able to test this tentative patch and update it as necessary.
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jteh)

Comment 12

5 months ago
For Beta, we might consider reverting the role="document" change, effectively shipping this accessibility improvement on the Preferences page one release later, which according to comment 0 seems better than having this incorrect behavior.
Assignee

Comment 13

4 months ago
I tested this. Take focus no longer checks the radio button. However, the problem is that take focus on another button in the group doesn't fire an accessible focus event. This makes sense: a11y can query CurrentItem, but it doesn't know when it has changed in this case; there's no event for that. We could fix this by firing a DOMMenuItemActive event on a radio button when it becomes the "focused" item. We use this trick for richlistbox already:
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/richlistbox.xml#111

Also, I think this is going to need review from a dom/xul reviewer, since it touches code there.
Flags: needinfo?(jteh)
Assignee

Comment 14

4 months ago
This patch (on top of the other patch) fixes the issue described in comment 13.
Assignee: nobody → jteh
Assignee

Comment 15

4 months ago
(In reply to :Paolo Amadini from comment #11)
> I'm actually waiting on input from Jamie, who might be able to test this
> tentative patch and update it as necessary.

was it your intention that I "steal" this patch or do you want to incorporate the patch I posted in comment 14 into your patch yourself?
Flags: needinfo?(paolo.mozmail)
Assignee

Updated

4 months ago
Assignee: jteh → nobody
Assignee

Comment 16

4 months ago
(In reply to James Teh [:Jamie] (away from bugmail until 7 Jan) from comment #8)
> 6. Verify that when a XUL <radio> is focused but not selected, pressing
> space to check the button causes an accessible state change event. If it
> doesn't, we might need some additional tweaks for event handling.

Reading the code and my testing in comment 13 confirms that a state change event does indeed get fired in this case.

Comment 17

4 months ago
(In reply to James Teh [:Jamie] from comment #15)
> (In reply to :Paolo Amadini from comment #11)
> > I'm actually waiting on input from Jamie, who might be able to test this
> > tentative patch and update it as necessary.
> 
> was it your intention that I "steal" this patch or do you want to
> incorporate the patch I posted in comment 14 into your patch yourself?

If you can take it from here, that would be great!
Flags: needinfo?(paolo.mozmail)

Updated

4 months ago
Depends on: 1518113
Attachment #9033176 - Attachment description: Bug 1514687 - Allow accessibility code to focus radio buttons without selecting them. r=Jamie → Bug 1514687 - Allow accessibility code to focus radio buttons without selecting them.

Sorry for the delay - I trust your and Paolo's decision on the plan. Presumably, if we eventually made the radio children use <html:input type="radio"> with a shared [name] attribute we could either drop <xul:radiogroup> entirely, or at least remove a fair amount of its frontend / accessibility code. Does that sound right?

Flags: needinfo?(bgrinstead)
Attachment #9033176 - Attachment description: Bug 1514687 - Allow accessibility code to focus radio buttons without selecting them. → Bug 1514687 - Allow accessibility code to focus XUL radio buttons without selecting them.

Comment 19

4 months ago
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/346851347c90
Allow accessibility code to focus XUL radio buttons without selecting them. r=bgrins,smaug,MarcoZ,paolo

Comment 20

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee: nobody → jteh

IIUC, this bug doesn't require uplift because bug 1518113 reverted 65 back to the previous behavior.

Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.