Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

JeffersonBledsoe
Copy link
Member

@JeffersonBledsoe JeffersonBledsoe commented Apr 15, 2024

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.

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 568ea19
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66313ed50e79a90008df2096

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 568ea19
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/66313ed59304f10008c7f97f

@ichim-david
Copy link
Sponsor Member

@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 :)

@sneridagh
Copy link
Member

@ichim-david if you have a moment, please take a look at this one, please!

@ichim-david
Copy link
Sponsor Member

@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)

@ichim-david
Copy link
Sponsor Member

ichim-david commented May 1, 2024

@JeffersonBledsoe @sneridagh these changes do improve the navigation but more so for the VoiceOver on Mac.
I suggest Jeff you add tabIndex={-1} also on skiplinks otherwise it won't interact properly with it.
See these 2 videos where I tab and use voiceOver and you will see it doesn't select it after using the landmark option.

skiplinks-without-tabindex.mp4

And this is with tabIndex

skiplinks-with-tabindex.mp4

There are still several things I don't like about our current skiplink story but this is another issue to add.

These issues are:

  • Go to main content is #view and that is not found on /contents or /edit .. I would add #main to the main tag and modify the skiplink to go there
  • The skiplink should be moved outside of the content area before the toolbar so that it is the first thing you focus on when you hit tab otherwise this isn't a proper skiplink
  • There is a problem with main section that I can trigger even on apple.com where the focus jumps from the main area to the area before it and can be seen even in my video demos

Copy link
Sponsor Member

@ichim-david ichim-david left a 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

@JeffersonBledsoe
Copy link
Member Author

@ichim-david You mean add the tabindex to the nav element containing our skip links right?
I agree I think a few things needs a bit of a rethink long-term

@ichim-david
Copy link
Sponsor Member

@ichim-david You mean add the tabindex to the nav element containing our skip links right? I agree I think a few things needs a bit of a rethink long-term

@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

@sneridagh
Copy link
Member

@JeffersonBledsoe @ichim-david status?

@ichim-david
Copy link
Sponsor Member

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants