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

♿Add alert role to toast for screen reader support #24825

Merged
merged 3 commits into from Oct 1, 2019

Conversation

jackbsteinberg
Copy link
Contributor

@jackbsteinberg jackbsteinberg commented Sep 30, 2019

Adds the alert role to toasts so screen readers assertively announce the content when the toast is displayed.
Resolves #24805

@amp-owners-bot
Copy link

Hey @newmuis, these files were changed:

  • extensions/amp-story/1.0/toast.js

@jackbsteinberg
Copy link
Contributor Author

I've got a few questions I want to answer before I'd be satisfied calling this ready to merge.

First, how should I test this? I ran the test-amp-story test suite and nothing seemed to break but I'm not sure if there's an accessibility testing pipeline I should be looking at.

Second, I chose status because it announces the content to screen readers politely (i.e. waits for whatever content is currently being read to finish before announcing its content), since I assume the toasts are low priority for the user to get to in this context. If that's not the case let me know and I should change the role to alert.

Context: Toast roles, status role, alert role

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2019

This pull request introduces 1 alert when merging 94c6694 into 8370cee - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@amp-owners-bot
Copy link

Hey @newmuis, these files were changed:

  • extensions/amp-story/1.0/toast.js

@newmuis
Copy link
Contributor

newmuis commented Sep 30, 2019

I am by no means an expert, but I think we might want to consider alert here? It's quite possible that there's a lot of content in the bookend (the optional last screen of the story used for sharing and related articles), so if all you want is the link, it might be inconvenient to wait for that all to be read out.

The code change LGTM, just wondering about the role itself.

@jackbsteinberg
Copy link
Contributor Author

I am by no means an expert, but I think we might want to consider alert here? It's quite possible that there's a lot of content in the bookend (the optional last screen of the story used for sharing and related articles), so if all you want is the link, it might be inconvenient to wait for that all to be read out.

The code change LGTM, just wondering about the role itself.

The guidelines for this pretty much allow you to choose which makes more sense in the context. If the toast content is the content we want to call attention to immediately, then alert likely makes more sense.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Oct 1, 2019

Hey @newmuis, these files were changed:

  • extensions/amp-story/1.0/toast.js

@jackbsteinberg jackbsteinberg changed the title ♿Add status role to toast for screen reader support ♿Add alert role to toast for screen reader support Oct 1, 2019
@jackbsteinberg
Copy link
Contributor Author

Update: I tried this with a the macOS VoiceOver screen reader and it seems to be working well

@gmajoulet gmajoulet merged commit 805360f into ampproject:master Oct 1, 2019
@jackbsteinberg jackbsteinberg deleted the toast-role branch October 2, 2019 14:25
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.

Toast is not read out through screen readers
4 participants