-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[Snackbar] Change default role from 'alertdialog' to 'alert' #17897
[Snackbar] Change default role from 'alertdialog' to 'alert' #17897
Conversation
…tent to resolve potential accessibility issues and added unit test for the prop
Details of bundle changes.Comparing: 5d564f9...25b66c7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for fixing an a11y issue. I left some comments at lines that need some work.
Should we document it the role aspect with an accessibility section at the bottom of the page (as with the other components)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document it the role aspect with an accessibility section at the bottom of the page (as with the other components)?
Added! I also added the new section to the non-English language docs (but it's still in English so they will need to be translated).
…kbar examples to use the new prop, and change default props usage to assigned default in props spread instead
…emilyuhde/material-ui into issue-17560-snackbarcontentrole
…tent to resolve potential accessibility issues and added unit test for the prop
…kbar examples to use the new prop, and change default props usage to assigned default in props spread instead
@oliviertassinari I made the default My understanding of the WAI ARIA specifications is that if the toast/snackbar/alert requires focus, then it should get https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_alert_role |
@emilyuhde Thanks for the feedback, I have added a note about a potential breaking change for v5 (so we don't forget it) |
…emilyuhde/material-ui into issue-17560-snackbarcontentrole
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the concern for potential breaking changes.
Though the default dialog is not an alertdialog. It doesn't require an action (even resolves itself after a certain amount of time has passed) and doesn't wrap focus. I'd rather default this to alert
and make it obvious in the PR title that we changed the default role (because it will appear that way in the changelog).
If somebody enhanced their snackbar to implement alertdialog properly then I'm ok with breaking their code. Maybe then they will open an issue so we can ask if they want to contribute their bug fix back 😉
PS: NVDA either seems to fixup current issues with alertdialog
or doesn't distinguish between alertdialog or alert anyway.
@eps1lon so we don't wait v5 for making the change? No objection 👍 |
@emilyuhde What do you think? Do you want to update the default value in this pull request? Alternatively, we can handle it, later on. Your work is already a significant step forward. |
That's fine. I can update the PR to set the default to 'alert' and I'll update the examples accordingly when I get a chance in a few hours. |
… and docs accordingly
…log" Co-Authored-By: Sebastian Silbermann <silbermann.sebastian@gmail.com>
… close because of the timeout
@emilyuhde Perfect, thanks for sticking with it 👍 |
Closes #17560