-
Notifications
You must be signed in to change notification settings - Fork 61
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
Allow focus to remain on the targets of hash links when the target is focusable #73
Allow focus to remain on the targets of hash links when the target is focusable #73
Conversation
…avigated to should be focusable: 1. natively focusable elements (a, input, textarea, select, button) 2. elements where the target had a tabindex prior to the navigation event This change allows consumers of react-router-hash-link to apply focus styles to the target element.
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 |
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. |
…cus on navigation
Thanks, makes sense. Also I think |
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. |
Looks good, thanks! |
v2.3 released with is change. |
Thanks so much @rafgraph! Appreciate your feedback and quick responses. |
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.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 w3c/wcag#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 d57be48 while still keeping that flexibility.Any feedback is welcome, and thank you for your work on this project.