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

FearlessSlug commented 5 months ago

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 🔁:

Open Windows 10 narrator by clicking ctrl+Start+Enter
Click a button that opens a toast
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:

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
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
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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

@mmalomo Is correct, please use pnpm

tricinel commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

emilkowalski commented 3 months ago

Thank you @tricinel

tricinel commented 3 months ago

This is the idea (sound on):

https://github.com/emilkowalski/sonner/assets/216008/baace819-1ae9-4cbc-8fae-1913945af29d

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?

ep-isaac commented 3 weeks ago

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 commented 3 weeks ago

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

emilkowalski / sonner