Closed Bug 1136563 Opened 5 years ago Closed 5 years ago

ARIA 1.1: Add support for 'switch' role

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed
relnote-firefox --- 39+

People

(Reporter: MarcoZ, Assigned: MarcoZ)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

ARIA 1.1 gets a new role 'switch' that needs to be supported:
http://rawgit.com/w3c/aria/master/aria/aria.html#switch

Joanie just landed support for it in Webkit:
http://trac.webkit.org/changeset/180600
Attached patch Patch (obsolete) — Splinter Review
Mapped switch to toggle_button role, made it react to aria-checked without the "mixed" part, and check for actions and events as well as roles and expected states.

I realize that there are differences to the traditional toggle_button implementation that you get when you add aria-pressed to a html:button element, for example, and these are intentional. For one, aria-pressed is not mentioned in the specification, but aria-checked is. Second, mixed state is not supported, which is logical. And third, I decided to go with the checked/checkable stuff to be consistent with aria-checked. It felt totally wrong to map aria-checked to pressed states in this scenario.
Assignee: nobody → mzehe
Status: NEW → ASSIGNED
Attachment #8569108 - Flags: review?(surkov.alexander)
I suggest to add SWITCH role to internal layer that is mapped to TOGGLE_BUTTON on ATK and IA2 and to AXCheckBox on OS X. Also OS X it should have AXSwitch subrole [1] per [2].

Otherwise it's good. (I don't see aria-readonly in the spec which should be considered as a bug I think)

[1] https://www.w3.org/WAI/PF/Group/track/actions/1563
[2] http://mxr.mozilla.org/mozilla-central/source/accessible/mac/mozAccessible.mm#428
Comment on attachment 8569108 [details] [diff] [review]
Patch

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

::: dom/base/nsGkAtomList.h
@@ +2296,5 @@
>  GK_ATOM(setsize, "setsize")
>  GK_ATOM(spelling, "spelling")
>  GK_ATOM(spinbutton, "spinbutton")
>  GK_ATOM(status, "status")
> +GK_ATOM(switchrole, "switch")

I think it should be "_switch" similar to "_false" for "_for". I can see "svgSwitch" but that's rather bad naming (if nothing has been changed)
Attached patch Patch v2Splinter Review
Added new role and mappings for it and the subrole (for Mac), adjusted the constant in NSGKAtoms.
Attachment #8569108 - Attachment is obsolete: true
Attachment #8569108 - Flags: review?(surkov.alexander)
Attachment #8569338 - Flags: review?(surkov.alexander)
Comment on attachment 8569338 [details] [diff] [review]
Patch v2

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

awesome! thanks, Marco! r=me

::: accessible/base/RoleMap.h
@@ +1059,5 @@
> +ROLE(SWITCH,
> +     "switch",
> +     ATK_ROLE_TOGGLE_BUTTON,
> +     NSAccessibilityCheckBoxRole,
> +     ROLE_SYSTEM_PUSHBUTTON,

not big deal but probably ROLE_SYSTEM_CHECKBUTTON would be more reasonable than pushbutton for MSAA

@@ +1061,5 @@
> +     ATK_ROLE_TOGGLE_BUTTON,
> +     NSAccessibilityCheckBoxRole,
> +     ROLE_SYSTEM_PUSHBUTTON,
> +     IA2_ROLE_TOGGLE_BUTTON,
> +     eNameFromSubtreeRule)

I see we do that for checkbox as well but it's a bit weird since they don't really have name valueable subtrees.

::: accessible/tests/mochitest/role/test_aria.html
@@ +181,5 @@
>    </a>
> +  <a target="_blank"
> +     href="https://bugzilla.mozilla.org/show_bug.cgi?id=1136563"
> +     title="Support ARIA 1.1 switch role"
> +    Mozilla Bug 1136563

nit: no 'Mozilla' please

::: accessible/tests/mochitest/states/test_aria.html
@@ +92,5 @@
>        // aria-checked
>        testStates("aria_checked_checkbox", STATE_CHECKED);
>        testStates("aria_mixed_checkbox", STATE_MIXED);
> +      testStates("aria_checked_switch", STATE_CHECKED);
> +      testStates("aria_mixed_switch", 0, 0, STATE_MIXED); // unsupported

side comment: I think it is mapped to STATE_CHECKED, that doesn't go with spec
Attachment #8569338 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #6)
> not big deal but probably ROLE_SYSTEM_CHECKBUTTON would be more reasonable
> than pushbutton for MSAA

Done.

> @@ +1061,5 @@
> > +     ATK_ROLE_TOGGLE_BUTTON,
> > +     NSAccessibilityCheckBoxRole,
> > +     ROLE_SYSTEM_PUSHBUTTON,
> > +     IA2_ROLE_TOGGLE_BUTTON,
> > +     eNameFromSubtreeRule)
> 
> I see we do that for checkbox as well but it's a bit weird since they don't
> really have name valueable subtrees.

Filed bug 1137146 for this since it's a separate bug with its own scope.

> ::: accessible/tests/mochitest/states/test_aria.html
> @@ +92,5 @@
> >        // aria-checked
> >        testStates("aria_checked_checkbox", STATE_CHECKED);
> >        testStates("aria_mixed_checkbox", STATE_MIXED);
> > +      testStates("aria_checked_switch", STATE_CHECKED);
> > +      testStates("aria_mixed_switch", 0, 0, STATE_MIXED); // unsupported
> 
> side comment: I think it is mapped to STATE_CHECKED, that doesn't go with
> spec

Confirmed. I thought the eARIACheckableBool state type took care of this? I mean why else would we have an item for "mixed" as well? Do radio and menuitemradio roles suffer from the same problem? How do we solve it?
http://mxr.mozilla.org/mozilla-central/source/accessible/base/ARIAStateMap.cpp#118
Flags: needinfo?(surkov.alexander)
(In reply to Marco Zehe (:MarcoZ) from comment #7)

> Confirmed. I thought the eARIACheckableBool state type took care of this? I
> mean why else would we have an item for "mixed" as well? Do radio and
> menuitemradio roles suffer from the same problem? How do we solve it?
> http://mxr.mozilla.org/mozilla-central/source/accessible/base/ARIAStateMap.
> cpp#118

same problem, we don't follow the spec here. I filed an ARIA thread [1] btw, so that's a separate issue I think.

[1] https://lists.w3.org/Archives/Public/public-pfwg/2015Feb/0117.html
Flags: needinfo?(surkov.alexander)
OK I'll just land it with the nits/MSAA role fixed, then.
https://hg.mozilla.org/mozilla-central/rev/fef52bcc6093
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Release Note Request (optional, but appreciated)
[Why is this notable]: Useful support for screen readers
[Suggested wording]: Support for 'switch' role in ARIA 1.1
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA

Marco, if you have suggestions for better wording for this release note, need-info me. Thanks!
Flags: needinfo?(mzehe)
Not really.
Flags: needinfo?(mzehe)
I've added a brief page about switch here:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_switch_role

but in the future we'll have to look at restructuring the ARIA docs and improving things.
You need to log in before you can comment on or make changes to this bug.