focus-trap-react: Conditionally rendering FocusTrap causes immediate deactivation with Strict Mode

I’m honestly convinced now that how it currently behaves is the correct behavior, and that relying on using onDeactivate for side-effecting code is the user’s problem. All the documentation really needs is a large bold warning about it, maybe some suggestions for how to deal with it, and definitely the rationale behind it so that it doesn’t take people by surprise.

If my guess about the unexpected behavior I mentioned above is in fact due to the Strict Mode-specific mitigations already in place, then truthfully trying to muck with things more than they already have will do more harm than good.

Hmm, I don’t think we’re talking about the same thing. I linked a fork of your sandbox where if (props.isOpen) is now if (props.isOpen || true) and it still works in strict mode. Note that props.isOpen remains false until the button is clicked, though, which means the trap remains inactive because it’s controlled by active={props.isOpen}, hence it doesn’t activate, and doesn’t get re-activated after the unmount/remount caused by strict mode, and so the handler isn’t called.

Ah, that’s my mistake, I had forgotten about that. Makes sense. To be clear, I wasn’t saying before that the modal didn’t work, just that the onDeactivate wasn’t being called when the app loads, which is now explained by this.

I wonder if, in your particular case, you would be better served using the function version of the clickOutsideDeactivates option. It will only be called if the click was indeed outside the trap’s containers, so that’s a clear signal that the user clicked outside, at which point you can call handleClose() (ultimately triggering the conditional rendering to not render the portal and destroy the modal) and return true, knowing the trap will be deactivated. And this won’t be affected by unmounting/remounting.

This was the use case I had originally tried for, but it requires me to have onDeactivate causing the side-effecting problems, since there isn’t specifically an onClickOutside or onEscapeKey, and realistically there shouldn’t be for sake of scope creep. My solution was ultimately to just use my own click-outside and escape key listeners that weren’t tied to FocusTrap.

@scatterfish

In the case where Strict Mode is used but conditional rendering is not, everything still works as (naively) expected because the simulated double mounting only happens when the component first mounts, and so the (naively) unexpected onDeactivate would only be called initially when the modal is still closed, resulting in no observable change.

…However, this actually isn’t the case. We would expect to see a message in the console saying that the handleClose function in the demo was called when the modal initializes but this doesn’t happen, and only happens when deactivated normally. Could this be the result of previous mitigation efforts for Strict Mode compatibility?

If I understand correctly, you’re saying that when strict mode is enabled, and conditional rendering (meaning if (props.isOpen) is changed to if (true) in your sandbox), that the dialog does not appear. But it does for me, unless I mucked something else up without realizing it.

BTW, thank you for that detailed sandbox with the helpful comments. Much appreciated!

---

If my guess about the unexpected behavior I mentioned above is in fact due to the Strict Mode-specific mitigations already in place, then truthfully trying to muck with things more than they already have will do more harm than good.

This is the strict mode mitigation – though I wouldn’t call this a mitigation, but rather support for strict mode. The fact that this.focusTrap is defined after unmounting and remounting can only mean that React kept the component instance somewhere after unmounting, and then restored it (i.e. restored the component’s state) upon remounting it. And when the trap is remounted, we’re checking state to see if the trap should be active, and then we’re checking to see if we already created the trap (which can only mean we’re in strict mode at this point), and then if what the props say doesn’t match what the trap thinks, and if so, we’re definitely in strict mode (or in a future day when React has implemented this remove sections of the UI while preserving state thing) and we need to bring the component’s state back to what it was on remount.

That seems to abide by what React is wanting components to support in the future, or to be “resilient” to.

---

@Slapbox

I appreciate the time you’re spending trying to think of a solution.

Still, I wonder if that makes that approach any more amenable in your view @stefcameron? 4 is a lot better than the 50 I originally mentioned. This also works as intended even if I set the browser to 6x slowdown.

Unfortunately, it doesn’t. I’m going to stick to requiring a solution that meets these two principals because I believe this will be what benefits consumers the most in the long run, and avoids imposing a burden of catch-up maintenance/support in this library:

1. It must be deterministic (i.e. not time-based).
2. It must maintains focus-trap-react’s separation of concerns WRT to strict mode (i.e. focus-trap-react must not need to know that it’s in strict mode).

later it occurred to me that if React does implement that, then presumably by that point basically any code that could take advantage of it would have to work in StrictMode

Yes, every component out there will need to support this unmount and future remount with pre-existing state behavior, if React does indeed implement this future “remove sections of the UI while preserving state” thing.

But the point I’ve been trying to make about this whole issue with calling onDeactivate when unmounted is that usually, that really is what you want, because when a component is unmounted, it’s supposed to die. Forever. It’s not supposed to come back. And what happens on deactivate is critical: Document handlers are removed. If they aren’t, you’ll get memory leaks, and what won’t benefit anyone.

And I think that if React implements this new feature, they aren’t going to implement it just to unmount and immediately remount components. They’re going to add it in order to unmount a component for a “long” (though arbitrary) period of time in order to boost performance of the app in the area that is currently receiving the user’s attention.

But that brings me to another point: If that’s the case, then you could imagine the automatic optimization would be much like how Chrome stops updating tabs behind the scenes when you haven’t used them in a while, until you finally activate that tab again, and then it might reload it (if you had closed Chrome and reopened it without having looked at the tab since) or will at least paint it for the first time in a while.

And with that in mind, when would it ever make sense to trap the user’s focus inside a piece of UI that would become a candidate for this behavior? If it ever did, clearly, the user’s attention is no longer there, and focus should no longer be trapped.

---

I think @scatterfish has it right: It’s a bad practice to rely onDeactivate() to affect the state of your app, at least the React state. It was originally conceived to do things like set styles in onActivate() and then unset styles in onDeactivate(), and that in focus-trap, which is purely JS/DOM/HTML-based. Bringing that into React requires different considerations for what would be considered “state”.

Thanks for those thoughts!

The more I think about this, the more I’m convinced that the fact that onDeactivate() gets called as a result of unmounting is, well, the stated behavior of focus-trap-react. When you unmount a focus trap, it ought to deactivate. And since a component is not to know whether it’s operating under strict mode (since I think knowing would defeat the purpose of what React might want to do in the future), then the fact that strict mode causes the component to unmount, thereby causing the trap to deactivate, is basically not the problem of focus-trap-react.

If the consumer wants to use strict mode with focus-trap-react, then they should be ready to deal with the consequences of using strict mode, and that’s the fact that strict mode will unmount the trap, causing that handler to be called.

The consumer’s code should therefore be fortified to survive the unmounting and remounting – which I maintain, in the future, should React ever do anything with this reusable state concept, will happen at a much bigger time gap than immediately unmounting and remounting. Maybe it would be as short, or maybe longer, but if I introduce some behavior to presume that remounting “within some time frame” should in effect be considered having not unmounted in the first place, that will surely open a can of worms I don’t want to support.

And I will also say that I have already gone to what I feel are quite reasonable efforts to support strict mode as best as possible in the code (see #721), including a demo, in spite of not agreeing with the concept at all.

So unless there is yet another solution that is deterministic (i.e. not time-based) and which maintains focus-trap-react’s separation of concerns WRT to strict mode, thereby not introducing behavioral differences between strict vs non-strict, I don’t think there’s anything more for focus-trap-react to do here.

I understand that might seem frustrating to you and to other consumers, but adding some quirky behavior will not serve anyone well.

Documentation, however, we can most certainly do!! I’ve been gradually massaging the README for while now, but it definitely still has a lot of room for improvement.

And if you or @Slapbox want to come up with a sample solution/suggestion/pattern/best practice on how to deal with strict mode when using focus-trap-react, even if it’s specific to dealing with the onDeactivate() handler getting called unexpectedly (though it shouldn’t be unexpected, but I digress), then we can most definitely add that to a new “React Strict Mode” section in the README in order to help others.

We could make it FAQ-style with code solutions so that if other strict mode issues are discovered that continue to fall outside the realm of what can reasonably be addressed by this library, then we can add another topic on how to deal with that.