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

Fix focus loosing for last page element #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix focus loosing for last page element #20

wants to merge 1 commit into from

Conversation

azat-io
Copy link

@azat-io azat-io commented May 6, 2021

There is a bug.

If the focus is on the last element of the page, by pressing Tab button, focus moves to the address bar of the browser. After then focus move to first focusable element of the page. Your hook tracks focus movement inside focus layer component. For this reason, focus trap stops working.

Also this example currently not works too.

@ai
Copy link
Contributor

ai commented May 6, 2021

@faultyserver I tried to update my project too, but found this regression in 6.1.

It was the reason why I was not able to use my focus-layers fix from #10

@ai
Copy link
Contributor

ai commented May 13, 2021

@faultyserver can we accept this small fix. I see warning from my #10 on every yarn upgrade-interactive and it bother me.

@faultyserver
Copy link
Collaborator

faultyserver commented Jun 18, 2021

Hello! Sorry for such a delayed response.

I'm not sure this is the approach that we'd want to take here. The forceFirst option of wrapFocus is really a utility for when focus first moves into a new container, where there is no directionality. The use case is in these lines, where as the comment says, we need to automatically move focus if it hasn't already moved:

// Move focus into the container if it is not already, or if an element
// inside of the container will automatically receive focus, it won't be moved.
const container = containerRef.current;
if (
container != null &&
document.activeElement != null &&
!container.contains(document.activeElement) &&
container.querySelector("[autofocus]") == null
) {
wrapFocus(container, document.activeElement, true);
}

An example of where this wouldn't be wanted is if the user was using shift+tab to move backwards through the document. When they go past the first element on the page and back into the browser's controls, this would just stop focus on that element instead of wrapping to the end as expected.

I think the brokenness that you're experiencing here is from not including FocusGuards around your app which create a fake tab stop to prevent exactly this issue of focus moving out of the document from the first or last elements on the page. The first example in the README needs to be updated to include these, but later on in the docs this is mentioned under "Edge Guards".

Additionally, changing attachTo to window for the event listeners limits the flexibility of the hooks, because it wouldn't be (easily) possible to have a component higher in the tree intercept focus events and pre-handle them. This isn't a normal use case, but is something we want to preserve as an option.

Hopefully that fixes the behavior you're seeing! If not, please let us know.

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.

3 participants