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

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

Merged
merged 4 commits into from
Dec 9, 2020

Conversation

maddy-jo
Copy link
Contributor

@maddy-jo maddy-jo commented Dec 8, 2020

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 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.

Mike Lumetta added 2 commits December 8, 2020 12:33
…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.
@rafgraph
Copy link
Owner

rafgraph commented Dec 8, 2020

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
Copy link
Contributor Author

maddy-jo commented Dec 8, 2020

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
Copy link
Owner

rafgraph commented Dec 8, 2020

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
Copy link
Contributor Author

maddy-jo commented Dec 9, 2020

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
Copy link
Owner

rafgraph commented Dec 9, 2020

Looks good, thanks!

@rafgraph rafgraph merged commit ba413a5 into rafgraph:main Dec 9, 2020
@rafgraph
Copy link
Owner

rafgraph commented Dec 9, 2020

v2.3 released with is change.

@rafgraph rafgraph mentioned this pull request Dec 9, 2020
@maddy-jo
Copy link
Contributor Author

maddy-jo commented Dec 9, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants