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 react-focus-trap resetting focus when preventScrollOnFocus is used #74

Merged

Conversation

tkajtoch
Copy link
Contributor

@tkajtoch tkajtoch commented Jul 5, 2023

We recently received a bug report describing an issue where react-focus-lock Trap resets focused element when previously focused element gets disabled programmatically. This is not something we expected to be happening, so I dug deep into debugger to figure out what was happening. I found that Trap is updating focus to the first element because its wrapper component (react-clientside-effect) detects any props update caused by react rerendering the tree and thus recreating the object passed to focusOptions:

focusOptions={preventScrollOnFocus ? { preventScroll: true } : undefined}

This PR uses useMemo to cache focusOptions value between renders and not cause react-clientside-effect to detect props change.

@tkajtoch
Copy link
Contributor Author

tkajtoch commented Jul 5, 2023

@theKashey I got straight to opening a PR with my fix but let me know if you'd prefer to open an issue describing what we've observed in more detail as well before we get this over the finish line.
Thank you for your amazing work!

@theKashey
Copy link
Owner

In this particular case it's better to extract { preventScroll: true } into a constant rather than using useMemo, and this is definitely not solving the root cause.

This change is 👍 in any case, consider it done. But I really need to understand what happened. What does mean "focused element gets disabled programmatically"?

It is:

  • a focused element inside Lock is being disable
  • it cannot be focused anymore and activeElements moves somewhere (body?)
  • focus reacts and puts focus back?

Well, there is a logic against it - if focus "jumps" too far it will be put back where it belongs, but that logic is definitely not ready for element disappearance.

Is my train of thought correct? Tell me your story.

@theKashey theKashey self-requested a review July 6, 2023 00:39
@theKashey theKashey merged commit bee7b55 into theKashey:master Jul 9, 2023
@theKashey
Copy link
Owner

theKashey commented Jul 9, 2023

Fix released as 3.9.1.
@tkajtoch - please open an issue for the root cause

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