Conditionally rendering FocusTrap causes immediate deactivation with Strict Mode

[scatterfish]

This doesn't appear to be quite the same issue as #720 or #738. Using v10.0.0.

I have a modal component that only renders itself when active, and as part of this modal component I use FocusTrap. When in Strict Mode, opening the modal causes onDeactivate and/or onPostDeactivate to get called immediately.

The focus trap works perfectly fine either without Strict Mode or without being conditionally rendered.

Here's a barebones CodeSandbox demo of the problem.

[Nantris]

@scatterfish correct me if I'm wrong, but you could probably fix this if you implement the debouncing solution I described here. Basically your issue is caused by onDeactivate managing the visibility of the component.

Really I should make a new issue because my comments on #738 went beyond the scope of the original issue, but the real issue in my view is that onDeactivate needs to be debounced and cancelled on unmount to make it work properly in StrictMode.

@stefcameron any thoughts on whether the change I suggested is surgical enough to be incorporated for StrictMode compliance? So far it seems like all the remaining issues people are running into are with onDeactivate specifically, and I think the workaround I suggested should automatically fix any issues downstream in onPostDeactivate as well. I haven't tested, but I suspect we could reduce the debounce time to lower than 50ms too.

[scatterfish]

I had seen that before, but (albeit without thoroughly studying the source for this component yet) didn't like debouncing as a solution here.

The "React Way" of persisting state across renders to ensure that things don't break is using refs. The debouncing solution also adds a layer of difference between dev and prod environments which really should be avoided if at all possible.

It feels like this is a problem internal to the component rather than something to be kludged around externally.

[Nantris]

The "React Way" of persisting state across renders to ensure that things don't break is using refs.

Yeah, but unfortunately StrictMode itself is the thing breaking React rules here, so we're really at their mercy.

The debouncing solution also adds a layer of difference between dev and prod environments which really should be avoided if at all possible.

Unfortunately it's not possible.

It feels like this is a problem internal to the component rather than something to be kludged around externally.

Yeah. While there's really no way around the other two issues, the only issue here is pretty much whether it's going to be the end user of focus-trap-react who is responsible for implementing the workaround I've shared, or if it's going to be integrated into focus-trap-react - which @stefcameron is rightly hesitant to do because yeah, DEV and PROD should really run the same code - but that's something we'd have to take up with the React team at Facebook. Or we can ditch StrictMode - which is what I would have done if I had to implement this workaround in more than one location myself.

If the workaround is internalized into focus-trap-react, that would be much safer in my view than each developer implementing their own workaround, as the same code would be used everywhere, which would lead to more reproducible issues (if any pop up,) and more battle tested code since more people are testing it.

[scatterfish]

Yeah, but unfortunately StrictMode itself is the thing breaking React rules here, so we're really at their mercy.

I agree that it's annoying but I don't think that just because what they say and what they do don't completely match that there's nothing to be done about it.

Unfortunately, I doubt that I personally have much to offer in regards to fixing it since my experience is mostly with functional components and hooks, let alone experience with the focus-trap library itself. Strict Mode is a feature that I do find helpful so I may need to make my own wrapper, which definitely isn't preferable since this already exists and is reasonably well supported.

Giving the source a look-through, it does stand out to me that the only refs used are for DOM elements rather than persistence, so I do (possibly naively) believe there is hope for a better solution. Wishful thinking, maybe, but is all really lost?

[stefcameron]

I appreciate the discussion, but here's my beef about Strict Mode: #720 (comment) And I firmly stand by it. React cannot just say one thing, and act like another. Plus, Strict Mode is not mandatory for using React. It's something you opt into. I've never used it myself in any of my work, and never plan to.

I also appreciate the suggestions for possible fixes, like debouncing onDeactivate, but that introduces differences in behavior between modes, which I don't like, and relies on timing, which I also don't like. Guaranteed whatever debounce time we this is reasonable, someone will need it to be a tad longer or shorter. And then will come the new configuration option to try to make this timeout configurable, and I'll have to support this...

All for the sake of Strict Mode, which violates a core tenant of React itself.

As for retaining state, I'm afraid refs won't help when the trap gets unmounted. When a component is unmounted, it's gone for good -- at least it's supposed to be -- and that includes any refs. Now React seems to indicate that maybe refs are restored, if they count that as "previous state", but I wouldn't call refs "state". Refs transcend what is state (ie. from useState() or this.state...). If refs are considered state, then would memoized values be restored too (from useMemo())?

In any case, the FocusTrap class creates an instance property, this.focusTrap, and somehow, that survives the unmount and remount. I also wonder if React, in Strict Mode, isn't actually unmounting, but rather just calling componentWillUnmount() and then componentDidMount() in succession to simulate the behavior (which would be even worse). So in effect, that kind of acts like a ref...

So far, the only thing I've been able to work with is the assumption that if the trap is remounted and this.focusTrap already exists (normally, on a fresh mount, it doesn't) and the component's active prop doesn't match the trap's active state (as in the this.focusTrap.active flag from the underlying instance of focus-trap that focus-trap-react creates), then we're probably in Strict Mode nonsense and we should just reactivate the trap to "restore state" and hope for the best.

I'm not sure what other assumptions I can make that wouldn't cause a difference in behavior between Dev and Prod, and wouldn't introduce a potential timing issue.

Finally, it kind of seems like we'd be pushing the burden of dealing with Strict Mode into focus-trap-react. This library is already doing everything I know to support Strict Mode without explicitly knowing about Strict Mode (which there's no way to know about, and we shouldn't anyway).

Shouldn't consumers also have to deal with Strict Mode behavior, like onDeactivate() being called? After all, it's supposed to be called on unmount, and the trap is really being unmounted...

Right now, Strict Mode simulates this unmounting/remounting, and it does it in quick succession, so it feels like a bug that onDeacticate() gets called. But if React does, some day, implement this "reusable state" feature, the remounting would happen much later, like minutes, hours, maybe even days (if a user keeps the app open in a browser for a long time) later. So deactivating the trap would really make sense then.

[scatterfish]

I appreciate the discussion, but here's my beef about Strict Mode: #720 (comment) And I firmly stand by it. React cannot just say one thing, and act like another.

I've already said my viewpoint on the "say one thing but do another" matter, but it's evident that React has plans for change, and these plans for change involve breaking existing components, which is why Strict Mode was introduced in the first place: to be prepared for it.

Strict Mode is a transitionary measure between the old React paradigm and the oncoming (and in many ways, currently present) modern/new React paradigm. If you plan to support future versions of React, then supporting Strict Mode in the meantime is the way to do it, at least according to the React dev team.

Plus, Strict Mode is not mandatory for using React. It's something you opt into.

For applications made with create-react-app, which the vast majority are, Strict Mode is now the default, meaning that it is now opt-out in every practical sense.

I also appreciate the suggestions for possible fixes, like debouncing onDeactivate, but that introduces differences in behavior between modes, which I don't like, and relies on timing, which I also don't like.

Fully in agreement here, which is why I'm trying to discuss possible better solutions.

As for retaining state, I'm afraid refs won't help when the trap gets unmounted. When a component is unmounted, it's gone for good -- at least it's supposed to be -- and that includes any refs.

Perhaps another "say one thing but do another" matter or perhaps it simply has already changed, but I have used refs for exactly this in my own components. It's possible that things work differently between functional components versus class components but regardless of what they're "supposed" to do, they have been a working solution for me at the very least.

Finally, it kind of seems like we'd be pushing the burden of dealing with Strict Mode into focus-trap-react.

That's the idea, yeah. You can tell users to work around it themselves but as @slapbox mentioned, the fragmentation of workarounds is really not a desirable outcome for anyone.

But if React does, some day, implement this "reusable state" feature, the remounting would happen much later, like minutes, hours, maybe even days (if a user keeps the app open in a browser for a long time) later. So deactivating the trap would really make sense then.

Unfortunately, until then we're stuck in the now, and it's going to continue being a problem if it's ignored, unless the documentation is updated to say that this component outright doesn't support Strict Mode or offers a solution to the problem, in whatever form it may take.

In the end, though, I'm not the one maintaining this component so who am I to complain? I get why you're angry, and I certainly don't envy the position you're in. In spite of that, what I don't think is that simply throwing in the towel is the answer here. Even if the solution is externalized to the end user, at least having a common pattern provided for it in documentation would (mostly) avoid fragmentation hell and offer some consistency.

[stefcameron]

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.

[scatterfish]

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.

This is a fair point, and I agree.

After sleeping on it, I gave the problem another look and I realize what the problem is here, but there still seems to be some strangeness to figure out.

In the case where conditional rendering is used but Strict Mode is not, everything works as (naively) expected since the focus trap is mounted once, and only deactivates once, so the modal can (wrongly) rely on the onDeactivate function to close.

In the case where both conditional rendering and Strict Mode are used, it doesn't work because when the modal gets unmounted and then re-mounted, the focus trap (rightly) calls the onDeactivate function, causing the modal to immediately close. Unfortunately, the only solution I can think of for this is to not rely on onDeactivate for anything with side-effects while in Strict Mode (and preferably in general).

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?

[Nantris]

I'm going to keep pondering if there's a reasonable approach to this that doesn't foist a bunch of extra responsibility onto the maintainers, but I wouldn't get my hopes up.

Most minimalist approach - not very useful probably:
The best minimal workaround I can think of right now is to provide an option onUnmount (which would then live outside of focusTrapOptions which would make it easy to put in the wrong place, so it's already getting messy) - and really all that does is removes the onus on the end user to wrap the FocusTrap in a HoC to handle the cancellation of what would be a user-implemented function debounce. It's certainly something, but is it enough to justify the effort? I don't know.

Debounce based approach with shortest possible timer:
Besides that, I tested the debounce method I described and it does work even if you set the debounce time to 0. But 0 really equates to around 4ms (assuming the backend of that is just a setTimeout call.) 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.

My only other thoughts are related to:

But if React does, some day, implement this "reusable state" feature, the remounting would happen much later, like minutes, hours, maybe even days (if a user keeps the app open in a browser for a long time) later. So deactivating the trap would really make sense then.

That's a very sensible approach, but 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 (I assume) - but that's pretty speculative and far into the future, so better to consider the here and now more, I agree.

I'll keep thinking about it. StrictMode doesn't do much good these days, but I assume in the future what is now StrictMode is going to be just plain old required patterns for React 20.x, or whatever version implements those features (if they ever do.)

[scatterfish]

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.

[stefcameron]

@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".

[stefcameron]

@scatterfish 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.

[scatterfish]

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.

No, what I mean is that what we would expect to happen would be:

• The app renders on first load
• All of the components are double mounted with Strict Mode
• Modal isn't being rendered conditionally, so it's present in the initial render and gets double mounted
• This means that FocusTrap gets double mounted on initial render

From this, we would expect to see that onDeactivate would be called when the app loads, resulting in a console message saying that the handleClose function was called. Instead, this doesn't happen, and my guess as to why would be from the previous compatibility efforts made for Strict Mode.

[scatterfish]

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.

[stefcameron]

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.

No, what I mean is that what we would expect to happen would be:

• The app renders on first load
• All of the components are double mounted with Strict Mode
• Modal isn't being rendered conditionally, so it's present in the initial render and gets double mounted
• This means that FocusTrap gets double mounted on initial render

From this, we would expect to see that onDeactivate would be called when the app loads, resulting in a console message saying that the handleClose function was called. Instead, this doesn't happen, and my guess as to why would be from the previous compatibility efforts made for Strict Mode.

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.

[scatterfish]

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.

[stefcameron]

FYI #804 will add a warning about React 18's Strict Mode behavior.

[scatterfish]

A bit rant-y for my taste but otherwise LGTM

[stefcameron]

😆 OK, I suppose you're right there. I'll try to tone it down a bit. It's hard!!

[stefcameron]

I think it's better now.

[scatterfish]

Yeah, that seems a lot better. 👍

[stefcameron]

Great! Thanks for having a look.