theKashey / react-focus-on

🎯 Solution for WAI ARIA compatible modal dialogs or full-screen tasks, you were looking for
MIT License
333 stars 14 forks source link

Fix react-focus-trap resetting focus when preventScrollOnFocus is used #74

Closed tkajtoch closed 1 year ago

tkajtoch commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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:

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 commented 1 year ago

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