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

[FocusScope] Fix bug where focus is moved to container when it shouldn't be #3115

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BenLorantfy
Copy link

@BenLorantfy BenLorantfy commented Sep 11, 2024

Description

Fixes bug described in #2436 where focus was moved to the container unnecessarily. The bug occurs when an element is removed via an onBlur callback.

Before

before.mov

After

after.mov

More technical details

Let's say we have some error text "This field is required" that is removed when the field is filled in and blurred. The current code checks if any node is removed in the mutation observer (not just if the focused element is removed), and if so focuses the container. The text getting removed triggers this condition:

if (mutation.removedNodes.length > 0) focus(container);

Additionally, the early return is not triggered here:

if (focusedElement !== document.body) return;

Because during any blur, the activeElement is document.body. This means checking document.activeElement === document.body is not a robust way of checking if the focus was lost.

image

The fix was to prevent focusing the container if the last focused element is still in the container:

// If the last focused element is still in the container, then it wasn't
// removed and we don't need to rescue focus
const lastFocusedElementStillInContainer = container?.contains(
lastFocusedElementRef.current
);
if (lastFocusedElementStillInContainer) return;
focus(container);

I left the previous checks in place because I was a bit scared to remove them, but it may not be required. The vue fork fixed this by replacing the existing checks: https://github.com/radix-vue/radix-vue/pull/519/files

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.

1 participant