fix(v2): make more accessible skip link #4162
Conversation
lex111
force-pushed
the
lex111/iss3917
branch
from
4cc3725 to
4b7ef71
Compare
|
Size Change: +18 B (0%) Total Size: 156 kB ℹ️ View Unchanged
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4162--docusaurus-2.netlify.app/classic/ |
|
Thanks @lex111 ! @Mythra can you please tell us if the "skip to main content" link works better for you now? Testing on this URL: https://deploy-preview-4162--docusaurus-2.netlify.app/classic/
Not sure to understand 100% what you mean here, should we make that link (replaces the button) lead to the main content when space is pressed? I think this PR improves the issue you had, as we now use a link, and it actually gives the focus ring to the main content, so I'm going to merge, but feel free to re-open/comment if this does not work for you |
| el.setAttribute('tabindex', '-1'); | ||
| el.focus(); | ||
| setTimeout(() => el.removeAttribute('tabindex'), 1000); | ||
| } |
There was a problem hiding this comment.
didn't know this was possible, nice trick
is it safe in all cases to use? (seems to work for Chrome + Safari at least)
There was a problem hiding this comment.
I think yes, it's safe.
|
Hey, Thanks for doing the work here, was looking forward to doing it myself/reviewing it, but hey as long as it gets done. As for the tabindex trick it is useful for Safari, but is there a particular reason removing tabindex is set to a delay of 1 second, Rather than just using a setTimeout of 0 so it happens on the next visual tick? It might be weird for a user who is tabbing in that second to be on an element, and all of a sudden have that element just not be a core focus anymore. I hit it once accidentally before looking at the code, but I was more testing everything was working. Hard to say how many people would hit it in practice.
@slorber as long as it's a link you don't need to have the space key function. The problem I was outlining there was having a "button" element specifically, not reacting to space. As long as it's a link (which is more correct) we don't need the space key to work. It's just when me, and other screen-reader users here "button" we expect space to work as by default it does. And FWIW I'm happy to provide reviews on a11y issues, so we don't merge in broken examples like we did with this. Where it was done in good faith, just no one was sure how to test it. Always better to ask and wait a bit than ship a broken experience I think 😉 But thank you both, tons! Glad to see it get fixed! |
|
@Mythra thank you too for letting us know! We will be glad if you find any more issues related with a11y, we will try to fix them!
Indeed, this is not a very good experience, maybe it is worth turning off the |
|
I'd say let's not override the outline behavior that the browser , if the browser wants to outline it let's let it do that (even though I don't think it should with 0). I think setting to 0 would help people tabbing through fast, so definitely I'd be for that change. |
Motivation
Should fix #3917
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview https://deploy-preview-4162--docusaurus-2.netlify.app/classic/
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)