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(Notification): add roles for accessibility #724

Merged
merged 5 commits into from
Oct 18, 2023

Conversation

MuhammadM1998
Copy link
Contributor

πŸ”— Linked issue

#688

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Adds a role = "alert" attribute to the Notification component to be read aloud by screen readers on rendering a new toast.
Resolves #688

Before.mp4
After.mp4

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@vercel
Copy link

vercel bot commented Sep 22, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
ui βœ… Ready (Inspect) Visit Preview Oct 18, 2023 3:51pm

@HigherOrderLogic
Copy link
Contributor

Maybe it's better to make this optional? I don't think everybody will want to have the notifications read out loud for the viewers.

@Haythamasalama
Copy link
Contributor

You can pass role=alert normally for UNotification or useToast because of v-bind="attrs"

<template>
 <UNotification role="alert" />
</tempalte>

Or

<script setup>
const toast = useToast()
</script>

<template>
  <UButton label="Show toast" @click="toast.add({ title: 'Hello world!', role: 'alert'})" />
</tempalte>

@MuhammadM1998
Copy link
Contributor Author

@HigherOrderLogic @Haythamasalama Just to be clear. The notification is read aloud only when using a screen reader which is used by people visually impaired to know that there's something happened. For the normal user there's no change. If you open the deployment link and check the toast example you should not hear anything. Only if you used a screen reader like NVDA.

Copy link
Member

Are you sure role="alert" is the way to go here? Maybe it would be better to use the role="region" on the UNotifications container and role="status" on each notification?

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/status_role

@MuhammadM1998
Copy link
Contributor Author

After reading the docs, the alert role is a special type of the status role and is used to express important info.

From the alert role on MDN:

The alert role is for important, and usually time-sensitive, information. The alert is a type of status.

Toasts are not highly important nor time-sensitive info. So It may be better indeed to use status. I'll update the code

@MuhammadM1998
Copy link
Contributor Author

@benjamincanac Sorry for the ping there's no 'request review' button to notify you

@benjamincanac
Copy link
Member

@MuhammadM1998 No worries, you can use the draft system to do so. I'll be notified when marking a PR as Ready to review 😊

@benjamincanac benjamincanac changed the title fix: add alert role for accessibility chore(Notification): add roles for accessibility Sep 25, 2023
@benjamincanac benjamincanac changed the title chore(Notification): add roles for accessibility fix(Notification): add roles for accessibility Sep 25, 2023
@MuhammadM1998 MuhammadM1998 marked this pull request as draft September 25, 2023 09:15
@MuhammadM1998 MuhammadM1998 marked this pull request as ready for review October 18, 2023 15:47
@benjamincanac benjamincanac merged commit 40f3b16 into nuxt:dev Oct 18, 2023
1 check was pending
@benjamincanac
Copy link
Member

Thanks 😊

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.

Notifications are not read by screen readers
4 participants