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

Upgrade success Alerts to role="alert" so they're read consistently #573

Closed
wants to merge 1 commit into from

Conversation

echappen
Copy link
Contributor

@echappen echappen commented Oct 22, 2024

Changes proposed in this pull request:

  • Upgrade success Alerts to role="alert" so they're read consistently
  • Make aria-live region for users list visible only to screen readers
  • Include a prototype page (/prototype/alert) that demonstrates how Alert components are read when they're added dynamically to a page.

See comments on #531 for further explanation.

Related issues

Part of research for #531

Submitter checklist

  • Added logging is not capturing sensitive data and is set to an appropriate level (DEBUG vs INFO etc)
  • Updated relevant documentation (README, ADRs, explainers, diagrams)

Security considerations

None - UI only

@echappen echappen marked this pull request as ready for review October 22, 2024 16:22
@echappen echappen requested a review from a team as a code owner October 22, 2024 16:22
@echappen echappen changed the title WIP: Upgrade success Alerts to role="alert" so they're read consistently Upgrade success Alerts to role="alert" so they're read consistently Oct 22, 2024
role = 'status';
}
if (type === 'error' || type === 'emergency') {
if (type === 'error' || type === 'emergency' || type === 'success') {
role = 'alert';
Copy link
Contributor

Choose a reason for hiding this comment

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

Eleni! Thank you very much for pulling all this together. (And for all the research in #531!)

I’m curious if this change might potentially be trading one set of usability issues for another? At least, I was under the impression that role="alert"/aria-live="assertive" were to be reserved for urgent messages that need a timely response, as they’ll interrupt any other message currently being read to the user by their AT. I’m worried that could be differently disorienting than what we’ve already got.

Dunno; what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point and I agree that this seems odd. In our use cases, success messages were not being read when they were given a role of status, which to me seemed even more disorienting for AT users. But they were read consistently when upgraded to role of alert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm up for fiddling with the /prototype/alert page more to see if we can still get status alerts to work more consistently. Adding and removing the status alert from the DOM gave no SR announcements. I tried keeping a status alert in the DOM while updating the text inside of it, but the SR would only read it the first time it changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Eleni! I was finally able to access /prototype/alert (#575), and can confirm messages aren’t being read consistently in Safari/VO.

Out of curiosity, would it be possible to have an empty ARIA live region in the document by default, and inject our messages into that container? I wonder if that’d fix the timing issue. (I think we might have discussed this briefly on a call, and specifically that this wasn’t possible; apologies if I’m asking a pre-answered question.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that, but ultimately concluded that it would be a workaround for alerts not acting properly, which they should be. I'd like for future devs to be able to use the Alert component wherever and have it work as expected. I'm happy to talk through it more (maybe at the next a11y jam?).

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great, let’s do it!

@echappen echappen mentioned this pull request Oct 29, 2024
2 tasks
@echappen
Copy link
Contributor Author

Closing in favor of #581

@echappen echappen closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants