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

fix(material/snack-bar): flaky screen reader announcements for NVDA/JAWS #20487

Merged
merged 1 commit into from Sep 23, 2020

Conversation

haywoodsloan
Copy link
Contributor

@haywoodsloan haywoodsloan commented Sep 2, 2020

Fixes a bug where NVDA won't announce polite snack bars and JAWS won't announce any.
This is because the live region (snack-bar-container) was added to the DOM, marked as a live
region, and content was added to it in the same operation. Some screen readers require the live
region to be added to the DOM, some time to pass, then content can be added. Now the snack bar
content is added to an aria-hidden div then, 150ms later, moved to a div with aria-live defined.
This won't cause any visual changes and keeps the snack bar content available immediatly after
opening. Also, no longer using the alert or status roles. Instead just using aria-live as testing
showed that NVDA will double announce with the alert role and JAWS won't announce any button text.

BREAKING CHANGE: matSnackBarHarness.getRole() replaced with .getAriaLive() due to using aria-live
rather than the alert and status roles.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 2, 2020
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

My understanding is that this will only be a problem for snack bars that don't have an announcementMessage. What if, instead of setting a role on the container, we announced the textContent of the container with the LiveAnnouncer, when an announcementMessage isn't specified? The advantage would be that we won't have to add the extra timeout and we'll avoid having to move DOM nodes around.

@haywoodsloan
Copy link
Contributor Author

I had previously considered an approach like you describe, using textContent and LiveAnnouncer. There were a couple of concerns that dissuaded me though:

  1. Screen readers will announce more than just the text content of a live region, this includes things like aria-label, aria-describedby, aria-disabled, aria-expanded, etc. Each screen reader has a slightly different language it uses to describe these attributes so matching what a users typically expects to hear from their screen reader isn't possible.
  2. Updates to the user's snack bar content after the initial announcement wouldn't automatically be announced. Could be a scenario where they open a snack bar from a custom component which changes content based on some external event.
  3. If the real content of snack bar appears late enough after the entrance animation ends, could capture the wrong text content and announce nothing. E.g. open a snack bar from a custom component that defer loads it's content.

Maybe a MutationObserver could address 2+3, but not 1. Given that and the extra complexity to filter mutation events to only announce changes a screen reader would for a normal live region, I think the DOM manipulation is cleaner solution.

@haywoodsloan haywoodsloan requested a review from a team as a code owner September 11, 2020 21:12
@jelbourn jelbourn added Accessibility This issue is related to accessibility (a11y) G This is is related to a Google internal issue labels Sep 15, 2020
// If an element in the snack bar content is focused before being moved
// track it and restore focus after moving to the live region.
let previouslyFocusedEle: HTMLElement | null = null;
if (this._platform.isBrowser &&
Copy link
Member

Choose a reason for hiding this comment

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

Are the isBrowser check and the instanceof HTMLElement strictly necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The instanceof HTMLElement check is because document.activeElement is type Element which doesn't implement focus. So I check to make sure the active element is of type HTMLElement and therefore focusable.

I found without including the isBrowser check I would get some view_engine test failures related to document not existing. For mdc-snack-bar with the switch back to using setTimeout the call to _screenReaderAnnounce is already nested in a isBrowser check, so I've removed the redundant one. For snack-bar I've kept the check because it isn't done elsewhere in the component.

Here's an example test failure: https://app.circleci.com/pipelines/github/angular/components/13171/workflows/d5670bcf-b85d-4721-8372-86b96b5f872f/jobs/187667/parallel-runs/0/steps/0-107

src/material/snack-bar/snack-bar-ref.ts Outdated Show resolved Hide resolved
@haywoodsloan haywoodsloan force-pushed the snack-bar-a11y branch 2 times, most recently from f70b92b to f3cdd40 Compare September 17, 2020 21:03
@haywoodsloan haywoodsloan requested review from jelbourn and removed request for a team September 17, 2020 22:42
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one last comment about the extra change detection cycle from setTimeout

@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Sep 17, 2020
@haywoodsloan haywoodsloan force-pushed the snack-bar-a11y branch 3 times, most recently from b12cd20 to 0628e6d Compare September 21, 2020 21:55
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Caretaker note: let me know if this results in a lot of failures

@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Sep 21, 2020
Fixes a bug where NVDA won't announce polite snack bars and JAWS won't announce any.
This is because the live region (snack-bar-container) was added to the DOM, marked as a live
region, and content was added to it in the same operation. Some screen readers require the live
region to be added to the DOM, some time to pass, then content can be added. Now the snack bar
content is added to an aria-hidden div then, 150ms later, moved to a div with aria-live defined.
This won't cause any visual changes and keeps the snack bar content available immediatly after
opening. Also, no longer using the alert or status roles. Instead just using aria-live as testing
showed that NVDA will double announce with the alert role and JAWS won't announce any button text.

BREAKING CHANGE: matSnackBarHarness.getRole() replaced with .getAriaLive() due to using aria-live
rather than the alert and status roles.
@annieyw annieyw merged commit cf8e8ee into angular:master Sep 23, 2020
@haywoodsloan haywoodsloan deleted the snack-bar-a11y branch September 23, 2020 21:53
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Sep 24, 2020
…AWS (angular#20487)

Fixes a bug where NVDA won't announce polite snack bars and JAWS won't announce any.
This is because the live region (snack-bar-container) was added to the DOM, marked as a live
region, and content was added to it in the same operation. Some screen readers require the live
region to be added to the DOM, some time to pass, then content can be added. Now the snack bar
content is added to an aria-hidden div then, 150ms later, moved to a div with aria-live defined.
This won't cause any visual changes and keeps the snack bar content available immediatly after
opening. Also, no longer using the alert or status roles. Instead just using aria-live as testing
showed that NVDA will double announce with the alert role and JAWS won't announce any button text.

BREAKING CHANGE: matSnackBarHarness.getRole() replaced with .getAriaLive() due to using aria-live
rather than the alert and status roles.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants