-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
fix: enhance accessibility for sidebar toggle button #2604
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
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@msftedad Could you please test it at https://docsify-preview-git-fork-sy-records-fix-2599-docsifyjs.vercel.app/preview/#/? |
@sy-records, As verified this issue in docsify. We observed that when the screen reader first focuses on the control, it announces 'Toggle Primary navigation button '. After pressing Enter or Space to hide or show the left navigation, the screen reader announces, 'Show primary navigation/Hide primary navigation, button, '. As discussion with the PWD team, they recommended that when the screen reader initially focuses on the control, it should announce the button's accessible label as 'Hide primary navigation', indicate its role as a button, and provide the shortcut key hint, such as 'Use shortcut key '. Please let us know if you need any further information. Thanks |
Thank you for your feedback. I've made the changes. |
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.
Thanks @sy-records @msftedad , testing on Mac OS and VoiceOver things look good and hopefully @msftedad can confirm the latest change.
Hi @paulhibbitts, To fix this, set them separately as: ![]() |
Shouldn't it be |
@sy-records, Yes, it should be. I noticed that there’s an issue when adding comments — the \ is automatically removed. Currently, the screen reader announces: "Hide primary navigation, Use shortcut key \ , button \ , " because the shortcut hint is included within the aria-label along with aria-keyshortcuts. To fix this, set them separately as: ![]() Please let us know if any further information is required. |
Okay, I've made the changes. Please retest. |
@sy-records, As verified this issue on docsify, and it appears to be resolved. Now, when the screen reader focus moves to the Left navigation pane show/hide button, it announces 'Hide primary navigation, button, Use shortcut key \ '. When Enter or Space is pressed, the screen reader announces, 'Show primary navigation, button, Use shortcut key \ '. ![]() Thank you. |
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.
Thanks very much @msftedad for confirming these changes, I am learning as we go through this very helpful process!
Summary
Related issue, if any:
Fix #2599
What kind of change does this PR introduce?
For any code change,
Does this PR introduce a breaking change?
Tested in the following browsers: