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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Joanmarie Diggs]

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).

Comment 1

[Joanmarie Diggs]

Attachment: proposed patch

Comment 2

[alexander :surkov (:asurkov)]

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

Comment 3

[Joanmarie Diggs]

Attachment: 1374316-for-landing.patch

(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!

Comment 4

[alexander :surkov (:asurkov)]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87825515d40b
Add aria-readonly to several roles as per ARIA 1.1. r=surkov

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87825515d40b