react-dark-mode-toggle icon indicating copy to clipboard operation
react-dark-mode-toggle copied to clipboard

Outline on button :focus

Open todd-elvers opened this issue 4 years ago • 5 comments

Clicking on the toggle button produces an outline around the button in Chrome:

button-outline

Adding outline: "none" to the button element fixes the issue, however that will cause confusion to anyone who is strictly using the keyboard because when they tab to the element on the page that outline won't be present. Some articles online suggest listening for key presses and, if the element is focused and the last key pressed was tab, showing the outline. While that pleases both types of users, it complicates the component.

I'm not sure what makes the most sense for this library and I defer to you @cawfree

todd-elvers avatar Feb 14 '21 17:02 todd-elvers

Here's a quick proof-of-concept of how you could implement this logic using a custom hook:

function useLastKeyPress(targetKey) {
  const [keyPressed, setKeyPressed] = React.useState(false);
  const reset = () => setKeyPressed(false);
  const downHandler = ({ key }) => {
    if (key === targetKey) {
      setKeyPressed(true);
    }
  };

  React.useEffect(() => {
    window.addEventListener("keydown", downHandler);
    window.addEventListener("click", reset);
    return () => {
      window.removeEventListener("keydown", downHandler);
      window.removeEventListener("click", reset);
    };
  }, []); // Empty array ensures that effect is only run on mount and unmount

  return keyPressed;
}

Then in the component you can add:

const wasLastKeyPressedTab = useLastKeyPress("Tab");
const outlineStyles = wasLastKeyPressedTab ? {} : { outline: "none" };

And explode those outlineStyles into the button's style attribute.

While this works it feels too heavyweight for this component and can cause it to flash while re-rendering (though that may be addressable). If you have a better solution by all means, otherwise I'm good with just addingborder: "none" to the button.

todd-elvers avatar Feb 14 '21 17:02 todd-elvers

Hey @todd-elvers, thanks for raising this. Honestly, you are absolutely murdering this repo (in the best way possible)!

I really like your solution, although I agree that it does overcomplicate this component, but only because it strikes me that this functionality shouldn't be isolated here; it'd probably be quite useful for other people to have a generic way to handle Chrome's outline in a modular way.

I did a quick search on npm and couldn't find a hook to use in-place which achieves this kind of functionality, although there are some providers that appear to handle this for you:

react-outline-manager

It might be easier to recommend that for Chrome support, the user wrap their application in the manager.

If you decide to opt for the custom hook route and publish a package to npm, I'd be more than happy to have the toggle depend upon it. (It'd be interesting to see if it's compatible with the provider too).

cawfree avatar Feb 15 '21 02:02 cawfree

@cawfree You're right, I believe it falls upon the user of the library to customize the outlining behavior based on the browser(s) they are targeting. That being said I'll create a PR to update the readme and make this clearer. Once I have a PR I'll mention it here.

todd-elvers avatar Feb 18 '21 22:02 todd-elvers

I am using tailwindcss and I only defined it using className="focus:outline-none"

here's an example https://www.joshuagalit.ga/

acatzk avatar Apr 02 '21 06:04 acatzk

I've updated the readme to include some context and a link to this issue in PR #18

todd-elvers avatar May 29 '21 18:05 todd-elvers