Closed
Bug 1514687
Opened 5 years ago
Closed 5 years ago
Focusing radio buttons causes them to automatically be checked (changed)
Categories
(Core :: Disability Access APIs, defect, P1)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox64 | --- | unaffected |
firefox65 | --- | disabled |
firefox66 | --- | fixed |
People
(Reporter: MarcoZ, Assigned: Jamie)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
2.03 KB,
patch
|
Details | Diff | Splinter Review |
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 years 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
Comment 2•5 years 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 years 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 years 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 years 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 years 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 years 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.
Assignee | ||
Comment 8•5 years 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 years 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.
Comment 10•5 years ago
|
||
Updated•5 years ago
|
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Comment 11•5 years 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 years 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•5 years 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•5 years ago
|
||
This patch (on top of the other patch) fixes the issue described in comment 13.
Assignee: nobody → jteh
Assignee | ||
Comment 15•5 years 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•5 years ago
|
Assignee: jteh → nobody
Assignee | ||
Comment 16•5 years 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•5 years 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•5 years ago
|
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.
Comment 18•5 years ago
|
||
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)
Updated•5 years ago
|
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•5 years 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•5 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Updated•5 years ago
|
Assignee: nobody → jteh
Comment 21•5 years ago
|
||
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.
Description
•