plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
427 stars 575 forks source link

Add tabindex to each of the skiplink elements to better handle focus #5959

Open JeffersonBledsoe opened 2 weeks ago

JeffersonBledsoe commented 2 weeks ago

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.

netlify[bot] commented 2 weeks ago

Deploy Preview for plone-components canceled.

Name Link
Latest commit 568ea19cfcb250a4cbc6bdd45c76a7a5dea6176e
Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66313ed50e79a90008df2096
netlify[bot] commented 2 weeks ago

Deploy Preview for volto canceled.

Name Link
Latest commit 568ea19cfcb250a4cbc6bdd45c76a7a5dea6176e
Latest deploy log https://app.netlify.com/sites/volto/deploys/66313ed59304f10008c7f97f
ichim-david commented 2 weeks ago

@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 commented 1 day ago

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

ichim-david commented 1 day ago

@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 https://github.com/plone/volto/pull/5929#pullrequestreview-2032225984

ichim-david commented 17 hours ago

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

https://github.com/plone/volto/assets/152852/9620e6c0-7b22-4f5d-a2b5-3cfc956839cd

And this is with tabIndex

https://github.com/plone/volto/assets/152852/bca13644-428d-4b69-ba8b-370e44434857

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

These issues are:

JeffersonBledsoe commented 16 hours ago

@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 commented 16 hours ago

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

https://github.com/plone/volto/assets/152852/efabaa7b-ee01-480e-9758-c71398f7c6b2