-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
refactor(v2): remove focus outline from mouse users #3626
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
body:not(.navigation-with-keyboard) *:focus { | ||
outline: none; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import {useEffect} from 'react'; | ||
|
||
import ExecutionEnvironment from '@docusaurus/ExecutionEnvironment'; | ||
|
||
import './styles.css'; | ||
|
||
// This hook detect keyboard focus indicator to not show outline for mouse users | ||
// Inspired by https://hackernoon.com/removing-that-ugly-focus-ring-and-keeping-it-too-6c8727fefcd2 | ||
function useKeyboardNavigation(): void { | ||
useEffect(() => { | ||
if (!ExecutionEnvironment.canUseDOM) { | ||
return undefined; | ||
} | ||
|
||
const keyboardFocusedClassName = 'navigation-with-keyboard'; | ||
|
||
function handleFirstTab(e: KeyboardEvent) { | ||
if (e.key === 'Tab') { | ||
document.body.classList.add(keyboardFocusedClassName); | ||
|
||
document.removeEventListener('keydown', handleFirstTab); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
document.addEventListener('mousedown', handleMouseDown); | ||
} | ||
} | ||
|
||
function handleMouseDown() { | ||
document.body.classList.remove(keyboardFocusedClassName); | ||
|
||
document.removeEventListener('mousedown', handleMouseDown); | ||
document.addEventListener('keydown', handleFirstTab); | ||
} | ||
|
||
document.addEventListener('keydown', handleFirstTab); | ||
|
||
return () => { | ||
document.body.classList.remove(keyboardFocusedClassName); | ||
|
||
document.removeEventListener('keydown', handleFirstTab); | ||
document.removeEventListener('mousedown', handleMouseDown); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
}; | ||
}, []); | ||
} | ||
|
||
export default useKeyboardNavigation; |
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.
not needed, useEffect never runs in node