-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Add tabindex to each of the skiplink elements to better handle focus #5959
Conversation
…and improve screen reader support
✅ Deploy Preview for plone-components canceled.
|
✅ Deploy Preview for volto canceled.
|
@JeffersonBledsoe I've been busy with work, I hope to look tomorrow morning at this fix, I know of the problem so I am hopeful to test your fix :) |
@ichim-david if you have a moment, please take a look at this one, please! |
@sneridagh tomorrow morning, I've been distracted tonight with giving my feedback on this other issue #5929 (review) |
@JeffersonBledsoe @sneridagh these changes do improve the navigation but more so for the VoiceOver on Mac. skiplinks-without-tabindex.mp4And this is with tabIndex skiplinks-with-tabindex.mp4There are still several things I don't like about our current skiplink story but this is another issue to add. These issues are:
|
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.
@JeffersonBledsoe if you add it also on skiplinks this is a good change to have and improves our screen reader navigation story
@ichim-david You mean add the tabindex to the nav element containing our skip links right? |
@JeffersonBledsoe I mean here https://github.com/plone/volto/blob/main/packages/volto/src/components/theme/SkipLinks/SkipLinks.jsx#L26 Also if we add a tab index = 0 on the first heading and on the breadcrumbs then it landmark navigation will function properly. Have a look at this video where I have them enabled and then toward the end I removed the tab index I've added on the devtools and you will see how it behaves. tabindex-breadcrumbs-heading.mp4 |
@JeffersonBledsoe @ichim-david status? |
@sneridagh I had some ideas of further improvements but they can come in a future pull request, it can be merged from my point of view. @JeffersonBledsoe unless you have something more to add I think we can merge it right? |
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.
@JeffersonBledsoe @ichim-david merging then!
Without the tabindex, the focus isn't correctly followed in most (all?) browsers and so the user may be put back to a different location from where they skipped to depending on how the link was used.