Skip to content
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

useFocusVisible: Only show focus ring on keyboard input #1073

Merged
merged 1 commit into from Jul 27, 2020

Conversation

christianvuerings
Copy link
Contributor

@christianvuerings christianvuerings commented Jul 23, 2020

Adds useFocusVisible a React hook we can use in all of our components to only show the blue outline (focus ring) when using the keyboard.

In this first PR we only apply it to <Button />

focus-outline-keyboard-only

Test Plan

Click on a <Button /> and don't see a focus ring:

useFocusVisible-mouse

Use the TAB key to get to a button and do see the focus ring:

useFocusVisible-keyboard

@netlify
Copy link

netlify bot commented Jul 23, 2020

Deploy preview for gestalt ready!

Built with commit 574551d

https://deploy-preview-1073--gestalt.netlify.app

@AlbertCarreras
Copy link
Contributor

@christianvuerings Interesting. is this a standard?

Copy link
Contributor

@themindfuldev themindfuldev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. The only consideration I had was about the DOM event listeners being global and then any DOM event will be fired off for all the handlers, in case this could cause unnecessary firing of these handlers. But since it's a global listener, it should not consume more memory and also will only fire at specific events. I'd just suggest keeping a look in the future to see if this can ever become a perf issue, if a page has too many components using useFocusVisible.

If that ever becomes an issue, an idea is to debounce the mousemove and pointermove events to reduce the number of handler callbacks.

@christianvuerings
Copy link
Contributor Author

@christianvuerings Interesting. is this a standard?

Yes, this is https://developer.mozilla.org/en-US/docs/Web/CSS/:focus-visible - however, since browser support is so low, we have to implement this in JavaScript right now.

https://caniuse.com/#search=focus%20visible

@christianvuerings christianvuerings marked this pull request as ready for review July 27, 2020 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release Minor release
Projects
None yet
3 participants