refactor(v2): remove focus outline from mouse users #3626
Conversation
lex111
added
the
pr: polish
| if (e.key === 'Tab') { | ||
| document.body.classList.add(keyboardFocusedClassName); | ||
|
|
||
| document.removeEventListener('keydown', handleFirstTab); |
There was a problem hiding this comment.
For best performance, I add/remove event handlers depending on whether the focus indicator is enabled. Is it worth it? Or we'd better add event handlers once when the Layout component is mounted (as it was done before in Tabs component), and accordingly remove them only when the component is unmounted (from useEffect return).
Something like this:
useEffect(() => {
if (!ExecutionEnvironment.canUseDOM) {
return undefined;
}
const keyboardFocusedClassName = 'navigation-with-keyboard';
function handleTab(e: MouseEvent | KeyboardEvent) {
document.body.classList.toggle(keyboardFocusedClassName, e.key === 'Tab');
}
document.addEventListener('mousedown', handleTab);
document.addEventListener('keydown', handleTab);
return () => {
document.body.classList.remove(keyboardFocusedClassName);
document.removeEventListener('keydown', handleTab);
document.removeEventListener('mousedown', handleTab);
};
}, []);
There was a problem hiding this comment.
Unless we have a performance issue for tabs, which is unlikely, I'd rather keep the code simpler to read/understand.
But I'm not sure the classList.toggle would work great because it would remove the outline on non-tab key presses? A user might navigate to inputs with keyboard, and then type in the input, and I think the focus ring should remain in such case?
There was a problem hiding this comment.
No, the outline will be displayed, you can see this on the page https://deploy-preview-3626--docusaurus-2.netlify.app/classic/docs/styling-layout
|
Deploy preview for docusaurus-2 ready! Built with commit 4d5b422 |
| document.body.classList.remove(keyboardFocusedClassName); | ||
|
|
||
| document.removeEventListener('keydown', handleFirstTab); | ||
| document.removeEventListener('mousedown', handleMouseDown); |
There was a problem hiding this comment.
Can we use mousemove instead of mousedown event? Not sure about that.
There was a problem hiding this comment.
not sure either, maybe the user is tabbing and moving the mouse at the same time 🤪 probably a niche usecase not very important to consider
There was a problem hiding this comment.
That seems pretty good.
I started separating theme/style components from behavioral ones, because we want at some point to support multiple themes (classic/bootstrap/tailwind...), all having the same UX (just the style and css system would change).
This code is likely to be shared between the 3 themes, and not meant to be swizzled, so I'd rather move it to src/client, until we create a new package with all the design-agnostic theme code. (there's outline none, but I think this still make sense for all themes to adopt this behavior)
| document.body.classList.remove(keyboardFocusedClassName); | ||
|
|
||
| document.removeEventListener('keydown', handleFirstTab); | ||
| document.removeEventListener('mousedown', handleMouseDown); |
There was a problem hiding this comment.
not sure either, maybe the user is tabbing and moving the mouse at the same time 🤪 probably a niche usecase not very important to consider
| useEffect(() => { | ||
| if (!ExecutionEnvironment.canUseDOM) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
not needed, useEffect never runs in node
| if (e.key === 'Tab') { | ||
| document.body.classList.add(keyboardFocusedClassName); | ||
|
|
||
| document.removeEventListener('keydown', handleFirstTab); |
There was a problem hiding this comment.
Unless we have a performance issue for tabs, which is unlikely, I'd rather keep the code simpler to read/understand.
But I'm not sure the classList.toggle would work great because it would remove the outline on non-tab key presses? A user might navigate to inputs with keyboard, and then type in the input, and I think the focus ring should remain in such case?
This must be done in a separate PR. Moreover, I did not delete anything in the Bootstrap theme, so focus behavior will is the same as before. |
|
Hey @lex111, found this potential new solution that looks more future proof. Do you think we should adopt it instead? |
|
It's cool, but Safari and Opera don't support it. I think it's better to wait until global support is at least 50% https://caniuse.com/?search=focus-visible We also have tablets that are connected to keyboard (for example, iPad with Magic Keyboard). WDYT? |
There was a problem hiding this comment.
LGTM, feel free to self merge if ready on your side.
We can improve this later with :focus-visible when browser support will be better, and refactor to a separate package when we start working on the 3 themes
Was wondering if it's worth adding a touchStart listener or something for touch devices with keyboards that you mentioned? Not sure it's needed.
Motivation
We currently have outline displayed after clicking on the dropdown item, similar to tabs (which was decided in #3171).
Therefore, in order to avoid duplicate code, we are better off using more general solution for the whole application than applying this fix (remove outline when not tab) to proper components.
This a11y issue is detailed in this blog post https://hackernoon.com/removing-that-ugly-focus-ring-and-keeping-it-too-6c8727fefcd2 that I was inspired in this PR.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview.
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.)