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

Add focus context for managing focus on route changes #22

Merged
merged 1 commit into from
May 20, 2020

Conversation

cathryngriffiths
Copy link
Collaborator

There's a lot of research and opinions on how to handle focus on route changes in SPA. Unfortunately, there's no one-size-fits-all solution; where the focus goes on route change depends on the application and on its context (for example, the focus on a single page could be different depending on whether there is a focus on the page or not). It could even change over time as user testing reveals better solutions.

In order to support route change a11y out-of-the-box and keep it generic enough to be used by multiple different apps, I've added a focus context to the router. This context provides as focusRef and a focus method (which calls focus() on the focusRef). The focusRef is exposed to consumers via the useFocusRef hook. The focus method is not exposed the consumers, but is called on every route change. This allows the consumer app to decide what should receive focus on each route change, but the logic for when this is called is up to the router itself.

Comment on lines 12 to 20
useEffect(() => {
if (!firstRender.current) {
focus();
}
});

useEffect(() => {
firstRender.current = false;
}, []);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're keeping track of the first render here because on the first load of the page, we should not force the focus (we should let the browser do whatever native behaviour it wants). Once the user starts navigating through the SPA, only then should we start forcing focus somewhere.

Copy link
Owner

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

A few things I'd like to discuss, but I think this is the right level of integration for the library, since all the strategies I've read about can be based on it 👍

if (!firstRender.current) {
focus();
}
});
Copy link
Owner

Choose a reason for hiding this comment

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

Reach has a few optimizations we should probably take:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only focus when the URL changes:

Done ✅

Something something avoiding problems with autofocus

It looks like we should be okay with the current implementation here. Autofocus is only supposed to apply on initial page load/render (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#htmlattrdefautofocus, facebook/react#5534 (comment)). Since we're not forcing focus on the initial load, autofocus will still work with this implementation.

packages/react-router/src/context.ts Outdated Show resolved Hide resolved
packages/react-router/src/hooks/focus.ts Outdated Show resolved Hide resolved
@cathryngriffiths
Copy link
Collaborator Author

@lemonmade updated based on your comments, let me know what you think!

Copy link
Owner

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

Looking great once a few last nits are addressed. I guess the autofocus thing is kind of a waste in reach router then?

packages/react-router/src/context.ts Outdated Show resolved Hide resolved
@cathryngriffiths
Copy link
Collaborator Author

I guess the autofocus thing is kind of a waste in reach router then?

Yeah that seemed a bit strange to me 🤔 They also don't force the focus on the first load from what I could see, but maybe their implementation is doing something else that I didn't see that makes the autofocus logic necessary.

@cathryngriffiths cathryngriffiths merged commit 1a3436d into master May 20, 2020
@lemonmade lemonmade deleted the cathryn/a11y branch June 15, 2020 15:58
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