fix(a11y): remove useKeyboardNavigation hook#11713
Conversation
|
Hi @nmggithub! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
✅ [V2]
To edit notification comments on pull requests, go to your Netlify project configuration. |
slorber
left a comment
slorber
left a comment
There was a problem hiding this comment.
I don't believe there is anything we need to replace it with as I believe the default behavior of browsers is now good enough.
Can you explain how the browser behavior has changed over time? I'm surprised that we don't need at least to use something like :focus-within somewhere.
From what I understand, the :focus / :focus-visible behavior didn't change, but it's likely the user agent stylesheet that was updated to use :focus-visible instead of :focus. However, how can we verify browser support is good? When did browsers do this change exactly?
Can you create a dogfood test page with all focusable html elements so that we can easily review the behavior in a single place?
This probably needs:
<a href=""> links
<button>
<input> (text, checkbox, radio, etc.)
<select>
<textarea>
<div tabindex="0">| body:not(.navigation-with-keyboard) *:not(input):focus { | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
Shouldn't we add some CSS to prevent the focus ring from displaying on some elements?
I don't know exactly which elements, but this code was probably added for a reason (I don't know the reason, unfortunately)
slorber
left a comment
slorber
left a comment
There was a problem hiding this comment.
Hmmmm, seems convincing enough to remove safely 👍
Apparently all 2022+ browsers migrated their User Agent stylesheets to outline on :focus-visible
2 participants
cc: @slorber
Pre-flight checklist
Motivation
The
useKeyboardNavigationhook was designed in a time where browser behavior with focus rings caused "ugly" visuals. The hook was written to resolve such behavior. I don't believe such behavior exists anymore, and that can motivate us to remove the hook in its entirety. This provides more benefits in that, with the code removed, it can no longer conflict with downstream styles (electron/website#1002) nor will it break some screen readers (#11314).I don't believe there is anything we need to replace it with as I believe the default behavior of browsers is now good enough. One potential edge case is this: if an element is selected via keyboard navigation and then that same element is then subsequently clicked, the focus ring will not disappear. However, I personally don't think that case is worth writing CSS code to prevent. If maintainers feel differently, I am willing to add such CSS to this PR.
Test Plan
N/A
Test links
N/A
Deploy preview: https://deploy-preview-11713--docusaurus-2.netlify.app/
Related issues/PRs
Should hopefully resolve #11314.