search

rafgraph / react-router-hash-link

Hash link scroll functionality for React Router
https://react-router-hash-link.rafgraph.dev
MIT License
732 stars 62 forks source link

Allow focus to remain on the targets of hash links when the target is focusable #73

Closed maddy-jo closed 3 years ago

maddy-jo commented 3 years ago

This PR makes the element.blur() call after the focus handling conditional depending on whether or not the element itself is focusable, either natively or by the presence of a tabindex.

  1. In the first case - if the element is a natively focusable element - leaving focus on the element brings the behavior closer to the native behavior for hash links. Linking directly to an interactive control is probably less common than linking to, say, headings, but it seems like a benefit to handle this case.
  2. In the second, consumers of this module can choose to leave focus on non-interactive elements by setting a tabindex on them. Therefore, it will work as before by default, but developers looking to add focus styles can opt in by setting that tabindex.

I'm most interested in the second case for the site I'm working in. We're leaning toward displaying focus visually for in-page navigation events as discussed in https://github.com/w3c/wcag/issues/1001, or at least exploring that option. This change allows us to remove our workaround click handlers that manage focus while still getting a visual focus indicator on target headings by leaving a tabIndex={-1} on those headings, so we can take advantage of the focus handling from https://github.com/rafgraph/react-router-hash-link/commit/d57be4898b4d2e20afca029db7d218bb7001411b while still keeping that flexibility.

Any feedback is welcome, and thank you for your work on this project.

rafgraph commented 3 years ago

Thanks for the PR. I'm on board with recreating the browser's native behavior without adding additional opinions, and it seems like this PR does that.

One question, what about interactive <area> elements, what's the browser's native scroll and focus behavior for them?

maddy-jo commented 3 years ago

One question, what about interactive <area> elements, what's the browser's native scroll and focus behavior for them?

That's a good call-out, thanks. Based on this part of the HTML standard and this StackOverflow answer, it looks like it should be focusable. I'll add that to the list of interactive elements.

rafgraph commented 3 years ago

Thanks, makes sense. Also I think <a> and <area> are only focusable if they also have an href attribute (omitting an href makes them disabled as they don’t support a disabled attribute). So it’d be good to update the interactive element check to include that. Thanks.

maddy-jo commented 3 years ago

That's also a good point, thanks. I've added that to that function. If you have any particular stylistic preferences for how to write that condition for readability, let me know and we can always rework it, but it should capture that case now.

rafgraph commented 3 years ago

Looks good, thanks!

rafgraph commented 3 years ago

v2.3 released with is change.

maddy-jo commented 3 years ago

Thanks so much @rafgraph! Appreciate your feedback and quick responses.