Add aria-readonly to switch, menuitemcheckbox, menuitemradio, and switch; remove aria-checked from menuitem

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jdiggs, Assigned: jdiggs)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
In ARIA 1.1, aria-readonly became a supported property of switch, menuitemcheckbox,
menuitemradio, and radiogroup.

In addition, aria-checked is not a supported property of menuitem (it is of menuitemcheckbox and menuitemradio, but not the generic superclass).
(Assignee)

Comment 1

2 years ago
Posted patch proposed patch (obsolete) — Splinter Review
Assignee: nobody → jdiggs
Status: NEW → ASSIGNED
Attachment #8879167 - Flags: review?(surkov.alexander)
Comment on attachment 8879167 [details] [diff] [review]
proposed patch

Review of attachment 8879167 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/base/ARIAMap.cpp
@@ +897,5 @@
>      eNoLiveAttr,
>      kGenericAccType,
>      kNoReqStates,
> +    eARIAOrientation,
> +    eARIAReadonly

thinking out loud: I'm surprised a bit to see aria-readonly on radiogroup, and not on radio

https://www.w3.org/TR/wai-aria-1.1/#radio

::: accessible/tests/mochitest/attributes/test_obj.html
@@ +30,5 @@
>        testAttrs("autocomplete", {"autocomplete" : "true"}, true);
>        testAttrs("checkbox", {"checkable" : "true"}, true);
>        testAttrs("checkedCheckbox", {"checkable" : "true"}, true);
> +      testAbsentAttrs("checkedMenuitem", {"checkable" : "true"}, true);
> +      testAttrs("checkedMenuitemCheckbox", {"checkable" : "true"}, true);

makes sense to have a test for 'menuitemradio' too

::: accessible/tests/mochitest/test_aria_token_attrs.html
@@ +356,5 @@
> +  <div id="switch_readonly_true" role="switch" aria-readonly="true">This switch has aria-readonly="true" and should get STATE_READONLY.</div>
> +  <div id="switch_readonly_false" role="switch" aria-readonly="false">This switch has aria-readonly="false" and should <emph>not</emph> get STATE_READONLY.</div>
> +  <div id="switch_readonly_empty" role="switch" aria-readonly="">This switch has aria-readonly="" and should <emph>not</emph> get STATE_READONLY.</div>
> +  <div id="switch_readonly_undefined" role="switch" aria-readonly="undefined">This switch has aria-readonly="undefined" and should <emph>not</emph> get STATE_READONLY.</div>
> +  <div id="switch_readonly_absent" role="switch">This switch has <emph>no</emph> aria-readonly attribute and should <emph>not</emph> get STATE_READONLY.</div>

one day we should something more automated for testing these
Attachment #8879167 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 3

2 years ago
(In reply to alexander :surkov from comment #2)
> Comment on attachment 8879167 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 8879167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/ARIAMap.cpp
> @@ +897,5 @@
> >      eNoLiveAttr,
> >      kGenericAccType,
> >      kNoReqStates,
> > +    eARIAOrientation,
> > +    eARIAReadonly
> 
> thinking out loud: I'm surprised a bit to see aria-readonly on radiogroup,
> and not on radio
> 
> https://www.w3.org/TR/wai-aria-1.1/#radio

There was Working Group discussion on this. I'd have to dig for all the notes, but if memory serves me the reasoning was that 1) The value being set is associated with the group; not an individual radio button. 2) If for some reason a particular item/radiobutton was not a valid choice (e.g. because that item was no longer available), that not-available option should be aria-disabled; not aria-readonly.
 
> ::: accessible/tests/mochitest/attributes/test_obj.html
> @@ +30,5 @@
> >        testAttrs("autocomplete", {"autocomplete" : "true"}, true);
> >        testAttrs("checkbox", {"checkable" : "true"}, true);
> >        testAttrs("checkedCheckbox", {"checkable" : "true"}, true);
> > +      testAbsentAttrs("checkedMenuitem", {"checkable" : "true"}, true);
> > +      testAttrs("checkedMenuitemCheckbox", {"checkable" : "true"}, true);
> 
> makes sense to have a test for 'menuitemradio' too

Done.

Thanks for the review!
Attachment #8879167 - Attachment is obsolete: true
Attachment #8879248 - Flags: review?(surkov.alexander)
Comment on attachment 8879248 [details] [diff] [review]
1374316-for-landing.patch

Review of attachment 8879248 [details] [diff] [review]:
-----------------------------------------------------------------

Actually not necessary to re-request review for minor tweaks. Just new patch + checkin-needed keyword if you are not going to land the patch yourself.
Attachment #8879248 - Flags: review?(surkov.alexander) → review+
Keywords: checkin-needed

Comment 5

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87825515d40b
Add aria-readonly to several roles as per ARIA 1.1. r=surkov
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87825515d40b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.