Marco Zehe (:MarcoZ) Assignee
	Description • 9 years ago

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

	Marco Zehe (:MarcoZ) Assignee
	Comment 1 • 9 years ago

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.

	Marco Zehe (:MarcoZ) Assignee
	Comment 2 • 9 years ago

Try run is at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ce558133b25

	alexander :surkov (:asurkov)
	Comment 3 • 9 years ago

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

	alexander :surkov (:asurkov)
	Comment 4 • 9 years ago

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)

	Marco Zehe (:MarcoZ) Assignee
	Comment 5 • 9 years ago

Added new role and mappings for it and the subrole (for Mac), adjusted the constant in NSGKAtoms.

	alexander :surkov (:asurkov)
	Comment 6 • 9 years ago

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

	Marco Zehe (:MarcoZ) Assignee
	Comment 7 • 9 years ago

(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

	alexander :surkov (:asurkov)
	Comment 8 • 9 years ago

(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

	Marco Zehe (:MarcoZ) Assignee
	Comment 9 • 9 years ago

OK I'll just land it with the nits/MSAA role fixed, then.

	Marco Zehe (:MarcoZ) Assignee
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fef52bcc6093

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/fef52bcc6093

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 12 • 9 years ago

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!

	Marco Zehe (:MarcoZ) Assignee
	Comment 13 • 9 years ago

Not really.

	Chris Mills [:cmills]
	Comment 14 • 9 years ago

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.