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

Focus trap doesn't work in React 18 #720

Closed
moroshko opened this issue Jun 18, 2022 · 9 comments · Fixed by #721
Closed

Focus trap doesn't work in React 18 #720

moroshko opened this issue Jun 18, 2022 · 9 comments · Fixed by #721

Comments

@moroshko
Copy link

I'm using focus-trap-react 9.0.1 and the focus trap doesn't seem to work.
If I focus on the first <input> and press Tab twice, I land on the second <input> that sits outside the trap.
What am I missing?

<div className="App">
  <FocusTrap>
    <div>
      <input type="text" />
      <button>Submit</button>
    </div>
  </FocusTrap>
  <input type="text" placeholder="I am outside the trap" />
</div>

Codesandbox

@stefcameron
Copy link
Member

That is strange. What you have there is all it should take.

I appreciate the repro. I can't repro this locally -- but I did notice that you're running this code in strict mode, and when not in strict mode, it works as expected.

So, what is happening in strict mode that's preventing the trap from working. Likely some violation is happening somewhere that's silently blowing up or preventing the trap from finding something, and causing it to render, but not activate. 🤔

@stefcameron
Copy link
Member

Tracing this out, seems nothing is blowing up in strict mode. The trap gets created, everything looks good. And yet it silently just doesn't work.

@stefcameron
Copy link
Member

I see. It's because in strict mode, React unmounts and remounts the <FocusTrap> after mounting it the first time. When the component is unmounted, the trap is deactivated, but not destroyed. When it remounts, the trap already exists, so nothing happens. And you're left with a deactivated trap.

React doesn't re-render on re-mount the second time, though, and doesn't re-execute the callback ref, so I will assume here that React is remounting onto the same DOM state as well, which means the trap, on remount, if exists, should check if it should be active and isn't and just reactivate and assume what it thinks the container elements are still exist in the DOM.

If React doesn't re-execute the callback refs, that won't trigger getting new DOM nodes for the container elements (when you don't specify the containerElements prop like you're doing in your sandbox) and so I'm not sure there's anything more that can be done. We have to assume the DOM hasn't changed. There's no approved way to "kick" the render can to trigger checking the DOM with the callback refs (that I know of, anyway). 🤷‍♂️

@stefcameron
Copy link
Member

For the record, I am not a fan of this strict mode. For one thing, it violates correct assumptions about componentWillUnmount

Once a component instance is unmounted, it will never be mounted again.

(emphasis mine)

Strict mode violates this assumption. I get that React is looking forward to this day where they will decide which parts of the tree to unmount when the user goes away somewhere, but strict mode violates normal assumptions, and then they expect the entire community to not operate under those assumptions??

stefcameron added a commit that referenced this issue Jun 18, 2022
In React strict mode, the trap gets immediately unmounted and remounted after
first mount. The trap deactivates automatically on unmount, so on remount,
we try to restore the trap to its previous active/paused state based on
the component's existing state.

React does not re-render the component, nor does it call callback refs
again, so we have no choice but to assume the DOM hasn't changed and
the existing trap's container elements are still in the DOM...

Way to go React for strict mode violating the assumption of
`componentWillUnmount()`, "Once a component is unmounted, it will
never be mounted again." :(
stefcameron added a commit that referenced this issue Jun 18, 2022
Fixes #720

In React strict mode, the trap gets immediately unmounted and remounted after
first mount. The trap deactivates automatically on unmount, so on remount,
we try to restore the trap to its previous active/paused state based on
the component's existing state.

React does not re-render the component, nor does it call callback refs
again, so we have no choice but to assume the DOM hasn't changed and
the existing trap's container elements are still in the DOM...

Way to go React for strict mode violating the assumption of
`componentWillUnmount()`, "Once a component is unmounted, it will
never be mounted again." Not a fan.
stefcameron added a commit that referenced this issue Jun 18, 2022
Fixes #720

In React strict mode, the trap gets immediately unmounted and remounted after
first mount. The trap deactivates automatically on unmount, so on remount,
we try to restore the trap to its previous active/paused state based on
the component's existing state.

React does not re-render the component, nor does it call callback refs
again, so we have no choice but to assume the DOM hasn't changed and
the existing trap's container elements are still in the DOM...

Way to go React for strict mode violating the assumption of
`componentWillUnmount()`, "Once a component is unmounted, it will
never be mounted again." Not a fan.

Also bumped react-dom to 18.2.0 since react was already there, and
set NODE_ENV for different builds.
stefcameron added a commit that referenced this issue Jun 18, 2022
Fixes #720

In React strict mode, the trap gets immediately unmounted and remounted after
first mount. The trap deactivates automatically on unmount, so on remount,
we try to restore the trap to its previous active/paused state based on
the component's existing state.

React does not re-render the component, nor does it call callback refs
again, so we have no choice but to assume the DOM hasn't changed and
the existing trap's container elements are still in the DOM...

Way to go React for strict mode violating the assumption of
`componentWillUnmount()`, "Once a component is unmounted, it will
never be mounted again." Not a fan.

Also bumped react-dom to 18.2.0 since react was already there, and
set NODE_ENV for different builds.
@stefcameron
Copy link
Member

@all-contributors add @moroshko for bug

...Although I wouldn't technically consider this a bug, but nonetheless, whatever I may think, many people seem to run in strict mode, so I definitely appreciate the heads-up here. Thank you, @moroshko !

@allcontributors
Copy link
Contributor

@stefcameron

I've put up a pull request to add @moroshko! 🎉

@stefcameron
Copy link
Member

Will be published in 9.0.2

@moroshko
Copy link
Author

Thank you @stefcameron for such a quick fix!

@stefcameron
Copy link
Member

My pleasure!

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 a pull request may close this issue.

2 participants