-
Notifications
You must be signed in to change notification settings - Fork 219
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 "Skip to main content" to content pages #450
Conversation
This way, when you click on the skip link, the screen reader starts reading the main content instead of the first link or button
Just noting: reviewer assignment based on the CODEOWNERS file didn't work properly 😢 I'll add this to the monorepo tasks: #384 |
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.
LGTM. Exciting to review my first frontend PR in the monorepo.
Co-authored-by: Zack Krida <[email protected]>
Full-stack documentation: Ready https://WordPress.github.io/openverse/_preview/450 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was updated 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes |
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.
Approving, with 1 question
@@ -1,5 +1,5 @@ | |||
<template> | |||
<Component :is="as" ref="containerNode"> | |||
<Component :is="as" id="main-content" ref="containerNode" tabindex="-1"> |
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.
I don't understand why tabindex
was set to -1 was set here. I could be wrong but I thought the default is to set it to 0.
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.
When the user clicks on the "Skip to main content" link, instead of focusing on the first tabbable element, we are now focusing on the element itself. It can be a main
or a div
, which is normally not focusable, or at least not focusable in older browsers.
I just realized that you weren't asking why there is a tabindex at all, but why it's set to -1
, @dhruvkb. It's because the element does not need to be focusable when you simply press Tab, it only needs to be focusable by Javascript.
This article suggests to use the -1
tabindex. It also shows that sometimes it makes sense to focus the h1
and not the main element. We might want to do that in the future, if the current implementation does not work well enough.
Fixes
Fixes #449 by @obulat
Description
This PR adds the "skip to main content" link to the content pages (About, Sources, etc).
It also updates the way
VSkipToContent
works. Instead of focusing on the first tabbable element inside the "main content", it focuses on the "main content" itself. This way, when you click "skip to content", the screenreader reads the main content instead of reading the first link.Testing Instructions
Go to
/about
, and pressTab
on your keyboard. You should see the "Skip to content" button appear at the top. When you click on it, if screen reader is on, you should hear the first heading or the text of the main content.Checklist
Update index.md
).main
) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin