React 18 StrictMode

[lasseklovstad]

Hey! I'm having an issue opening my custom modal with FocusTrap in StrictMode after upgrading to React 18. When the modal state is opened onDeactive is called immediately (probably because of the double render in Strictmode), which closes the dialog.
I see you have recently released a fix for StrictMode but it doesn't fix this issue.

I made a demo where i just copied the example from: demo/js/demo-special-element.js

The change i made in the demo is to only render the FocusTrap element when it is active.

Code sandbox demo: https://codesandbox.io/s/0vhfsb

Is this a bug or should i implement it differently?

[stefcameron]

Thanks for bringing this up! This sounds a whole lot like #720 which was fixed in #721 but clearly doing it the other way around makes the fix ineffective. Thank you also for the repro.

<rant>I really don't like strict mode. It's a pain. And it violates React's very own rules!! But here we are. I never use Strict Mode for this very reason.</rant>

I will try to debug this later when I have some time, unless you can dig in and tell me what's happening. Quick check shows that the trap is immediately activated and then deactivated, probably because of the mount -> unmount -> mount cycle. But why doesn't it re-activate on the remount given this check? https://github.com/focus-trap/focus-trap-react/blob/master/src/focus-trap-react.js#L287 It's like either the component's state is now inactivate, or the trap's internal state is still active -- one of the two is the opposite of what the fix is expecting, and so we don't fall into the IF block and reactivate the trap.

[stefcameron]

@lasseklovstad I had a closer look. Turns out, AFAICT, the trap is doing what it's supposed to do, especially after #721, which is to handle being immediately unmounted and remounted after being initially mounted (behavior caused by Strict Mode).

This works for the use case in #721 when you don't have a component outside the <FocusTrap> that is controlling the trap's active state. A trap is active by default, so if you just plop one down and render, under Strict Mode, the fix in #721 is employed to restore the trap to an active state in the remount.

But in your case, because you have this activeTrap state variable, and you have this method tied to the trap's onPostDeactivate event:

  unmountTrap() {
    this.setState({ activeTrap: false });
  }

thing don't work so well because, under Strict Mode, the trap gets unmounted -- which causes this handler to get called, thus setting activeTrap to false. Then the trap is remounted, but then there's an immediate React state update (because the outer component's activeTrap state changed from true to false), and since you're conditionally rendering the trap when activeTrap is true, the trap gets unmounted again, the handler gets called again (has no effect this time since the state is already false), and things settle.

And you're left with no visible trap.

So this isn't a bug with the trap. Of course, not using Strict Mode, it works just right. The trap renders when activeTrap becomes true, and it's active.

I'm not sure what to suggest here. As you know, I'm not a fan of this behavior of Strict Mode. There are other things Strict Mode which I'm fine with. But this unmount/remount thing, no. IMO, React has an idea of what they want to do in the future (i.e. temporarily/dynamically disabling parts of the UI by unmounting them, and then reactivate them later by remounting them), but this behavior violates their API (as I explained in #720). And focus trap is behaving correctly according to the API. 🤷‍♂️ Not sure how to reconcile that, short of being able to somehow detect strict mode, and have the trap behave differently in that case (i.e. by expecting an initial unmount and setting a flag to remember it has happened, and not deactivating the trap, avoiding triggering the deactivation event handlers, that one time), something I'm also not a fan of.

[stefcameron]

I'm happy to keep chatting about what you might be able to do in your wrapping component, but I will close this issue since I don't believe it's a problem with focus trap and I'm not sure what more I can do beyond #721.

[Nantris]

But in your case, because you have this activeTrap state variable, and you have this method tied to the trap's onPostDeactivate event:

We're experiencing this issue but we're not managing the visibility in the way you described as far as I can tell, but we still have the issue. We do set like this:

active={!props.isStandalone}

but props.isStandalone is either true or false and doesn't change.

<FocusTrap
  active={!isStandalone}
  focusTrapOptions={{
    returnFocusOnDeactivate: false,
    clickOutsideDeactivates: true,
    onDeactivate: this.onDeactivate,
}}>

Unfortunately removing the line with the active prop makes no difference, nor does removing clickOutsideDeactivates. Any thoughts? We're on version 10.0.0.

I think the issue really for us is that onDeactivate fires. Is there any way it could be prevented in Strict Mode? We used onDeactivate to replace react-onClickOutside - which we previously used together with focus-trap-react, but 10.0.0 made that impossible (#719)

There are other things Strict Mode which I'm fine with. But this unmount/remount thing, no. IMO,

Couldn't agree more. StrictMode is a pain in the ass because of this behavior.

[Nantris]

@gaearon is there any advice you can give for how to make a library like focus-trap play nice with StrictMode? This seems the most daunting challenge I've come across so far in implementing it, to the point we're now considering ditching StrictMode - but I'd really prefer to stick to best practices.

[stefcameron]

@slapbox

I think the issue really for us is that onDeactivate fires. Is there any way it could be prevented in Strict Mode?

I don't know of any way to detect strict mode, and I'm guessing that would be beside the point of strict mode. I also don't think it's good to have a version of the component that works in strict mode one way, and another version that works in not strict mode another way... I really loathe strict mode for this.

We used onDeactivate to replace react-onClickOutside - which we previously used together with focus-trap-react, but 10.0.0 made that impossible (#719)

What in 10 made this impossible? The only thing that changed in 10 was the tabbable dependency, and there's a workaround to restore the old behavior. There were no code changes in 10 at all. Functionally, aside from the need for the workaround for old behavior (if it's even an issue), the library is the same as it was in 9.0.2

[Nantris]

Thanks very much for your reply as always @stefcameron!

Whoops that was my mistake! It was actually 9.x that I meant, not 10.x, and the issue was the removal of findDOMNode because react-onclickoutside makes you access like el.instanceRef to access the actual underlying component, and I guess that masked it - basically this comment: #719 (comment)

Perhaps I could re-cludge it back together using react-onclickoutside, but I think I'll just abandon StrictMode instead, it's simply not worth it for the trouble. If we can't use focus-trap-react then we can't use StrictMode, and you're right that it's not good to have to basically two versions of focus-trap-react code to try to keep in sync across DEV vs PROD.

---

But if it could be limited down to only modifying the onDeactivate logic, I feel like that might be acceptable? I don't know enough about the underlying logic to know if this would work, but what about debouncing onDeactivate in DEV and cancelling the pending function call when the component is unmounted? In strict mode it would be unmounted basically instantly, right?

If that's the only logic that needs to be patched, I feel like that's not entirely unreasonable - but it does open the door to trying to patch more oddities people may discover due to StrictMode as time goes on. Thoughts? Would it even work? Thanks again!

Update: I implemented a proof of concept in the case that was affecting our app and it does work! But it's incredibly unwieldy for users to implement for each time they need onDeactivate

In our actual use case there's a couple of components involved and some of this is passed as props, but I've simplified down the example here and hopefully it still works as intended:

In the constructor of our class-based component:

constructor(props, context) {
    this.onDeactivateFocusTrap = lodash.debounce(
      this.onDeactivateFocusTrap,
      IS_DEV_ENV ? 50 : 0
    );
}

and in its componentWillUnmount:

componentWillUnmount() {
  this.onDeactivateFocusTrap.cancel()`
}

I suspect this would cause no unintended side effects for virtually any possible use case as I can't see a case that calls for implementing FocusTrap where the focus enters and leaves more often than every 50ms. Thoughts?