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

BUG: Invalid WAI-ARIA setup preventing screen readers from announcing new toasts #306

Closed
FearlessSlug opened this issue Jan 19, 2024 · 13 comments · Fixed by #436
Closed

Comments

@FearlessSlug
Copy link

FearlessSlug commented Jan 19, 2024

Describe the feature / bug 📝:

New toasts are not announced when using some screen readers like window's narrator (ctrl+Start+Enter on Windows 10/11) as well as old versions of iOS's VoiceOver

Steps to reproduce the bug 🔁:

  1. Open Windows 10 narrator by clicking ctrl+Start+Enter
  2. Click a button that opens a toast
  3. See that the toast is not being announced

Why this is happening:

The problem occurs due to the ARIA setup of the toasts and how some screen readers handle ARIA live regions:

  • Each toast is wrapped with an li element with role="status"
  • That li element is only present in the HTML when there's a toast and is removed when there's no toast
  • This is the root cause of this bug because ARIA live regions should always be present on screen so that screen readers can subscribe to their changes in order to announce future changes, so by conditionally rendering the live region causes some ("dumb") screen readers to miss the changes of the live region therefore not announcing it

Solution:

  1. We can go the radix-ui route and have a separate element with role="status" that is always present in the HTML and the content of the latest toast will be injected to that element
  2. Or we can move the role="status" to the section (which is always present in the HTML) and explicitly add aria-relevant="additions text" to only announce additions and not removals and also explicitly add aria-atomic="false" to not re-read all the toasts when a new toast is added but rather only the changes
  3. Another suggestion is to allow consumers of the lib to provide their own ARIA props to the section element as well as to the ol and li elements in order to customize the looks / functionality and most importantly the ARIA settings (like changing the aria-label to the website's language, because right now it's hard coded to notifications alt+t which in case you use a language that is not English it will make your site violate WGAC a11y rules which are required by law in some regions, states and countries)

[I was trying to run this repo locally to test that my suggested changes work and create a pull request but I ran into too many errors and problems]

@emilkowalski
Copy link
Owner

Let's use the second solution. Whhat errors did you run into locally? You should be good with pnpm i and then pnpm dev:website

@FearlessSlug
Copy link
Author

Let's use the second solution. Whhat errors did you run into locally? You should be good with pnpm i and then pnpm dev:website

At first I got some errors after running npm run dev:website, I guess removing the directory and re-cloning it solved it
Right now I'm getting this printed out, I'm not sure what to do now, I tried going to localhost:3000 but got nothing

• Packages in scope: sonner, website
• Running dev in 2 packages
• Remote caching disabled
sonner:dev: cache bypass, force executing SOME-ID
website:dev: cache bypass, force executing SOME-ID

 Tasks:    0 successful, 2 total
Cached:    0 cached, 2 total
  Time:    535ms

@mmalomo
Copy link
Contributor

mmalomo commented Jan 24, 2024

Let's use the second solution. Whhat errors did you run into locally? You should be good with pnpm i and then pnpm dev:website

At first I got some errors after running npm run dev:website, I guess removing the directory and re-cloning it solved it Right now I'm getting this printed out, I'm not sure what to do now, I tried going to localhost:3000 but got nothing

• Packages in scope: sonner, website
• Running dev in 2 packages
• Remote caching disabled
sonner:dev: cache bypass, force executing SOME-ID
website:dev: cache bypass, force executing SOME-ID

 Tasks:    0 successful, 2 total
Cached:    0 cached, 2 total
  Time:    535ms

Same thing happened to me.

Im running npm i and then npm run dev:website

Edit:

@FearlessSlug just install pnpm and your are good to go. I had not seen the packageManager in the package.son.

@emilkowalski
Copy link
Owner

@mmalomo Is correct, please use pnpm

@tricinel
Copy link
Contributor

Looking at this as well. li elements are not allowed to have the status role either (see MDN). I can have a look if no one is already working on it.

@FearlessSlug
Copy link
Author

I can have a look if no one is already working on it.

That would be great, I've been too busy to dive deeper into this.

@tricinel
Copy link
Contributor

Alright, cool! Then I'll take it and update here early next week.

@emilkowalski
Copy link
Owner

Thank you @tricinel

@tricinel
Copy link
Contributor

This is the idea (sound on):

Screen.Recording.2024-03-20.at.09.24.41.mov

The thing is, now we can't do the different aria-live values of either polite or assertive using toast.important since we're putting the aria-live on the section (which may contain multiple toasts). We can make the entire section assertive if any of the underlying toasts are important - I'd argue we don't. Having it assertive might be a very disorienting experience if the screen reader stops mid-sentence to read the toast.

What do you think?

I could open a draft PR we can check. There are quite a few bells and whistles in there (like pointer event, swipping) which I must admit I haven't checked. And I don't have Windows 10 to check the original error. Maybe @FearlessSlug can check?

@isaacunderwood
Copy link

Is there any update on this? I have been experiencing this in Cypress with it reporting as a "serious violation" which is not ideal.

Thanks! 🙏🏻

@tricinel
Copy link
Contributor

tricinel commented Jun 7, 2024

I think I forgot about this. I'll open the PR.

tricinel added a commit to tricinel/sonner that referenced this issue Jun 7, 2024
@ooluyole-balogun
Copy link

Hey @tricinel is there any update on when your Fix would be merged? I have been experiencing this in storybook with it reporting "violation".

@tricinel
Copy link
Contributor

tricinel commented Sep 4, 2024

@ooluyole-balogun I couldn't say. I don't have merging permissions etc :) I started using my fork for my projects already, but happy to go back to the original and not miss on the great work already put into this.

emilkowalski added a commit that referenced this issue Nov 2, 2024
Co-authored-by: Emil Kowalski <36730035+emilkowalski@users.noreply.github.com>
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 a pull request may close this issue.

6 participants