Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(snackbar): Accessibility: Announce label text to screen readers #4090

Merged
merged 7 commits into from
Nov 17, 2018

Conversation

acdvorak
Copy link
Contributor

Fixes #4063

@mdc-web-bot
Copy link
Collaborator

All 711 screenshot tests passed for commit b4b19b7 vs. feat/snackbar! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 711 screenshot tests passed for commit 6f34f18 vs. feat/snackbar! 💯🎉

<div class="mdc-snackbar__label">Can't send photo. Retry in 5 seconds.</div>
<div class="mdc-snackbar__label"
role="status"
aria-live="polite">Can't send photo. Retry in 5 seconds.</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're announcing it programmatically do we even need to mention it here? Seems like we're turning it off explicitly to avoid repetition.

announce() util could take, message and priority as arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question!

Yes, I think we want to keep it here in the markup so that clients can easily change it to assertive if necessary (some screen readers don't announce the text unless it's assertive).

I experimented with having announce() take message and priority args, but I wasn't able to make it work in practice. Screen readers are really picky about where you put the ARIA attributes and do the DOM manipulation, and sometimes that needs to happen on different elements.

I also tried doing something like goog.a11y.aria.Announcer#getLiveRegion_(), dynamically inserting an anonymous element into the <body> using JS. But unfortunately some screen reader/browser combinations refuse to announce the text when I do that. Others needed a much bigger timeout value between clearing and resetting the text.

I've arrived at the present solution after many hours of painful experimentation. It seems to work reliably in 6 of the most commonly-used browser/screen reader combinations: JAWS and NVDA in Chrome, IE 11 💩, and Firefox.

It doesn't seem to work with Safari + VoiceOver on macOS 10.13 High Sierra, unfortunately.
I'll document that limitation in the README (next PR).

Copy link
Contributor

@kfranqueiro kfranqueiro Nov 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing Safari+VO seemed to work with assertive rather than polite, but I can re-test when we get to the readme.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining! The announce adapter method seem to be coupled with what util.announce() method does which has its own business logic. It has to follow certain sequence of DOM operations to implement it.

For example, some wrappers have their own announcer utilities which makes it hard for them to reuse it or do they've to reimplement this util function?

Copy link
Contributor Author

@acdvorak acdvorak Nov 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect internal teams to use goog.a11y.aria.Announcer#say(), and all other wrappers to use util.announce().

I added unit tests for the util.announce().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, as long as if the screen reader doesn't end up start pronouncing it twice (or jank) because of aria-live on child element and from announcer service, I guess this works for everyone.

// however, `aria-live` is turned off, so this DOM update will be ignored by screen readers.
labelEl.setAttribute(ARIA_LIVE_LABEL_TEXT_ATTR, labelText);

setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little wary that we're not storing / returning the handle on this timeout, but maybe I'm being paranoid. Is there a chance of the snackbar being dismissed before this timeout even elapses and causing weird behavior? Or will it simply not announce because the snackbar will become display: none?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's display: none when the timeout fires, the browsers I tested (Chrome/IE11/Firefox in NVDA/JAWS) don't announce it.

The timeout is responsible for resetting the textContent back to its original value, so I'm not sure we'd want to clear it. But there could be a use case I'm not thinking of.

I made sure to test for non-empty textContent at the top of the function, so if you call it twice in a row, the second call is a noop.

From my testing, Windows screen readers queue the message and read it fully as long as the textContent was reset to its original value before the snackbar was set to display: none.

@@ -50,7 +50,6 @@
id="test-snackbar--wide"
role="status"
aria-live="polite"
aria-hidden="true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've been moving the role/aria-live attributes to the label but you didn't do that here?

(I'm actually curious why we want these attributes only on the label - does that mean the action/dismiss buttons won't be announced? How will a user relying on screen readers know that they exist?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed!

You are correct, the action/dismiss buttons won't be announced.

According to WCAG 2.0 — SC 2.2.3 — No Timing, UI controls should not be timing-dependent. We don't want to announce a button that won't be there by the time the user navigates to it.

The snackbar a11y spec also says that apps should provide a redundant means of achieving the same action elsewhere in the app.

So for AT users, this is simply a passive notification.

* @param {!HTMLElement} ariaEl
* @param {!HTMLElement=} labelEl
*/
export function announce(ariaEl, labelEl = ariaEl) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of labelEl being a separate parameter? We don't seem to be using it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time the DOM or CSS changes (e.g., adding display: none), it breaks in at least one AT/browser combination. I've gone back and forth between using separate elements or the same element, depending on the moods of the Screen Reader Gods.

For example, if we decided to announce the action button to screen reader users (which I advise against—see my comment above), we would need to move the ARIA attributes back to the root element and add aria-atomic="true". Then they would be two separate elements again.

I'm mainly trying to make future changes easier.

@mdc-web-bot
Copy link
Collaborator

All 711 screenshot tests passed for commit 90a52ff vs. feat/snackbar! 💯🎉

@acdvorak
Copy link
Contributor Author

Added unit tests for announce(). Snackbar has 100% code coverage.

@codecov-io
Copy link

codecov-io commented Nov 17, 2018

Codecov Report

Merging #4090 into feat/snackbar will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@                Coverage Diff                @@
##           feat/snackbar    #4090      +/-   ##
=================================================
+ Coverage          98.78%   98.78%   +<.01%     
=================================================
  Files                126      127       +1     
  Lines               5575     5600      +25     
  Branches             733      736       +3     
=================================================
+ Hits                5507     5532      +25     
  Misses                68       68
Impacted Files Coverage Δ
packages/mdc-snackbar/constants.js 100% <ø> (ø) ⬆️
packages/mdc-snackbar/foundation.js 100% <100%> (ø) ⬆️
packages/mdc-snackbar/index.js 100% <100%> (ø) ⬆️
packages/mdc-snackbar/util.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95c6482...276ac38. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 711 screenshot tests passed for commit b92a170 vs. feat/snackbar! 💯🎉

<div class="mdc-snackbar__label">Can't send photo. Retry in 5 seconds.</div>
<div class="mdc-snackbar__label"
role="status"
aria-live="polite">Can't send photo. Retry in 5 seconds.</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, as long as if the screen reader doesn't end up start pronouncing it twice (or jank) because of aria-live on child element and from announcer service, I guess this works for everyone.

test/unit/mdc-snackbar/mdc-snackbar.test.js Show resolved Hide resolved
@mdc-web-bot
Copy link
Collaborator

All 711 screenshot tests passed for commit 276ac38 vs. feat/snackbar! 💯🎉

@acdvorak acdvorak merged commit ed2b65a into feat/snackbar Nov 17, 2018
@acdvorak acdvorak deleted the feat/snackbar--a11y branch November 17, 2018 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants