-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Navigation block: Add notice on reduced accessibility #52251
Navigation block: Add notice on reduced accessibility #52251
Conversation
Size Change: +176 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
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.
A couple code quality notes. I think we should only announce the notice after the initial toggle. Doing it more than that could get a little noisy.
Ok. I made the screen reader notice assertive and it doesn't trigger on the initial render. Let me know if that helps 🙂 |
Flaky tests detected in 5e15a923599b353eb8f4fbb5b6d2dedec148189b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5456756041
|
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.
@luisherranz Can you pass the entire notice content variable into speak()
? Screen readers should get the full context. Also, I think we can drop assertive off this notice. Through testing, I think it is okay that the screen reader announces the checkbox state and then the notice. Sometimes assertive is needed depending on how dynamic the notice is so we catch users attention before they navigate elsewhere canceling out the notice. Sorry, these things can be tricky to get right. Almost there.
Oh, absolutely. I did it because I saw that the color contrast spoken notice was a reduced version, so I thought spoken notices shouldn't be too long.
Sure, no problem 🙂 |
54399f0
to
97629b8
Compare
Generally, yes. However, users can always press control if they do not want to hear more. Where if they do want to hear more, they will have to navigate to the notice, what they already heard, and then the rest of the notice. It's a tricky balance at times. |
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.
👍
Cool. Thanks for the review, Alex! |
What?
Fixes #51226.
Why?
Because when both the
showSubmenuIcons
("Show arrows" in the UI) andopenOnClick
options are false, the accessibility is reduced, and after some discussion, the decision was to add a warning when both of those options are false.How?
Just use the component and approach as the one used in the Color panel for the low contrast warning.
I have one question: should the screen reader notice be triggered each time there is a full refresh if those options are selected, or only after the user has changed them?
I'm following what I saw for other notices, but it feels a bit weird to hear that notice each time you refresh, so I wonder what's the usual behavior here.
Testing Instructions
Screenshots or screencast