-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(toast): improve a11y support for $mdToast.simple(). improve docs #11424
Conversation
src/components/toast/toast.js
Outdated
* <td>`.actionKey(string)`</td> | ||
* <td> | ||
* Adds a hot key for the action button. <br/> | ||
* If the `actionKey` and Control are pressed, the toast's action will be triggered.<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always having this be based on control
seems a little too specific. The user might want to do ctrl + shift
or even just a single key. Gmail, for example, uses just z
for undo. For MatSnackbar
, we just document that the user should add their own keyboard shortcut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. However this only applies to the "simple" toasts.
If developers need more flexible options, then the custom toast demo was updated to demonstrate how to setup custom keyboard shortcuts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also happy to extend this API to accept values for the metaKey
, ctrlKey
, shiftKey
, and altKey
should the request arise.
Do you think that we should just go ahead and support those now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel super strongly either way.
src/components/toast/toast.js
Outdated
options: /* @ngInject */ function($mdToast, $mdTheming) { | ||
return { | ||
template: | ||
'<md-toast md-theme="{{ toast.theme }}" ng-class="{\'md-capsule\': toast.capsule}">' + | ||
' <div class="md-toast-content">' + | ||
' <span class="md-toast-text" role="alert" aria-relevant="all" aria-atomic="true">' + | ||
' <div class="md-toast-content" role="alert" aria-relevant="all">' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using role="alert"
isn't really the best pattern for snackbars. alert
is a more specific type of dialog, which are inherently interuptive, while snackbars are meant to be non-interuptive.
The recommend approach is to use an aria-live
region (polite) for the text of the notification (which is what the MatSnackbar does)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into using $mdLiveAnnouncer
which does enable the use of role="status"
live regions with aria-live="polite"
but the current implementation of it does not work well with interim elements like toasts.
I'm not 100% sure, but I believe for the current implementation to work properly, the $mdLiveAnnouncer
needs to be injected in a component that is longer lived. This might work fine if other components are using $mdLiveAnnouncer
and you add a toast, but it doesn't seem to work if you only add the toast w/o any other components to trigger instantiation earlier.
The description of the alert
role does seem to align well with the behavior of snackbars and toasts.
The alertdialog
role does not, as it better fits a modal dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right, I mixed up alert
and alertdialog
. In that case you should be able to swap role="alert"
with aria-live="polite"
and get the same treatment. Not going through the live announcer isn't ideal, but AFAIK it should work most of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I swap to role="status"
then it should default to aria-live="polite"
. However, it seems like a snackbar with an interactive button / hotkey would better fit the alert
role versus the status
role.
The other downside here is that role="status"
is broken in Chromevox until Chrome M70.
It looks like role="alert" aria-live="polite"
together may be the best fit. I'll do some more testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, adding both isn't a good idea as it causes double reading of the messages.
However, polite
in most cases (with our current defaults and demos) causes the toast to be read after the toast times out and disappears. This means that by the time the user heard the instructions for interacting, the toast would be gone.
To mitigate this, I've added instructions to the hideDelay
docs to recommend setting it to 0
when the toast includes an action. I would like to do this automatically for simple toasts, but I will avoid it for now since it might cause some g3 tests to fail. I've also updated the demos to use hideDelay(0)
when they have an action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't believe role="status"
is widely supported.
The Material Design team also says that snackbars shouldn't auto-dismiss when they have an action, so that would indeed be the right behavior.
move the role="alert" up a level - makes action button visible to screen readers add support for defining an actionKey to assign a hot key to an action - this enables Control-actionKey to activate the action add support for defining a dismissHint for screen readers add support for defining an actionHint for screen readers align custom toast demo with Material Design guidelines enhance custom toast demo to demonstrate an accessible custom toast Fixes #349
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
' <span class="md-visually-hidden">{{ toast.dismissHint }}</span>' + | ||
' <span class="md-visually-hidden" ng-if="toast.action">' + | ||
' {{ toast.actionHint }}' + | ||
' </span>' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new dismissHint
and actionHint
span
elements are returned by tests that do .find('span').text()
like expect(parent.find('span').text().trim()).toBe('Do something');
. So the tests will now get the all of the contents (even the contents of the two new md-visually-hidden
span
s). This broke two AngularJS Material tests, but it appears to be breaking some tests within Google as well.
Can anyone think of any way to work around this that doesn't mean changing all of the tests?
This is in an Aria Live Region, so I don't think that something like aria-describedby
would be a valid workaround.
I wonder if wrapping the new elements in a div
or changing them to div
s would work. It seems like it may break screen readers but I will test it.
- Test wrapping
dismissHint
andactionHint
span
elements indivs
- Test changing
dismissHint
andactionHint
span
elements todivs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jeremy indicated that it should actually be easy to fix the 2 tests within Google that are broken by this.
…ngular#11424) move the role="alert" up a level - makes action button visible to screen readers add support for defining an actionKey to assign a hot key to an action - this enables Control-actionKey to activate the action add support for defining a dismissHint for screen readers add support for defining an actionHint for screen readers align custom toast demo with Material Design guidelines enhance custom toast demo to demonstrate an accessible custom toast Fixes angular#349
…ngular#11424) move the role="alert" up a level - makes action button visible to screen readers add support for defining an actionKey to assign a hot key to an action - this enables Control-actionKey to activate the action add support for defining a dismissHint for screen readers add support for defining an actionHint for screen readers align custom toast demo with Material Design guidelines enhance custom toast demo to demonstrate an accessible custom toast Fixes angular#349
PR Checklist
Please check that your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Actions within toasts are not announced at all for screen readers.
It's almost impossible to activate an action in a toast from a screen reader.
There are no instructions for screen reader users on how to dismiss a toast.
Issue Number:
Fixes #349
What is the new behavior?
move the role="alert" up a level
add support for defining an actionKey to assign a hot key to an action
add support for defining a dismissHint for screen readers
add support for defining an actionHint for screen readers
align custom toast demo with Material Design guidelines
enhance custom toast demo to demonstrate an accessible custom toast
Does this PR introduce a breaking change?
Other information