'You tried to return focus to but it is not in the DOM anymore' warning.

[Jacob-Allen]

I have recently upgraded react-modal from 1.7.1 to 3.1.6. I am now getting a warning in the console whenever react-modal unmounts.

Steps to reproduce:

1. Mount a component that is using React Modal as a child.
2. Dismount the component has has React Modal as a child. (this happens for me when I navigate between routes)

Expected behavior:

1. I'm not sure if this warning is due to incorrect implementation on my part or if this is expected behavior.

Additional notes:

The modals are behaving normally, I am just unsure why the focus manager is giving me this warning.

[diasbruno]

Hi @Jacob-Allen, before the modal is open, it will put in a queue the document.activeElement, so it can try to return the focus after the modal is closed. So, if this element doesn't exist in the next route this warn will be presented.

[Jacob-Allen]

Hey @diasbruno, It is often that the modal is never opened on certain pages. Is there anything I can do to drop it from the queue before I dismount the component so that I can circumvent this warning?

[diasbruno]

There is an option we added recently - shouldReturnFocusAfterClose={bool}. If false, it will be no-op when pop out of the queue.

[diasbruno]

It should work fine in case where you can jump to other pages from the modal, but if it's not the case, it may cause some accessibility issues.

[diasbruno]

I'll keep this open, because this feature is not documented.

[indiesquidge]

@diasbruno, I am also getting this warning more than a dozen times. It does not happen when I run my app in the browser, but when I run my tests. I see that the logic for displaying this warning has existed for some time now, but I only started seeing it when I upgrade to v3.1.7.

I don't fully understand why this warning is being shown to me. It occurs on components that have modals in them, but they are closed by default, so I'm curious why would they ever be attempting to "return focus" when they shouldn't have been muddling with the focus in the first place (since they are all closed).

[indiesquidge]

before the modal is open, it will put in a queue the document.activeElement, so it can try to return the focus after the modal is closed.

Doh, I missed this my first read through! So the modals are adding to the queue before they are open, i.e. when they are rendered.

I can remove the error by not manually unmounting my component (removing ReactDOM.unmountComponentAtNode(node) from my smoke test), which come to think of it seems fine.

I suppose I hadn't considered this when I first started writing my tests, but there is usually no reason to manually unmount (only caveat is when testing things like <ReactModal> open/close directly, since exiting a test with a modal open will pollute document.body.classList, which could affect other tests). I think it would be safe to remove the manual unmount call for my smoke tests, which also resolves my issue of seeing this warning so many times when running tests.

Thanks!

[royrwood]

I'm getting this warning, too. Tracing into it, the following code in focusManager.js is the issue:

function returnFocus() {
  var toFocus = null;
  try {
    toFocus = focusLaterElements.pop();
    toFocus.focus();
    return;
  } catch (e) {
    console.warn(["You tried to return focus to", toFocus, "but it is not in the DOM anymore"].join(" "));
  }
}

For whatever reason, toFocus is undefined when the line toFocus.focus() is called. Why not include a test to ensure that toFocus has a valid value before executing that?

In my test case, I haven't actually displayed a modal yet, and still get the warning (i.e. my render( ) method does include a <Modal> element, but isOpen=false so it is not actually visible).

[Jacob-Allen]

Hi @royrwood ,

@diasbruno mentioned that there is an undocumented prop to bypass the focusmanager. By adding shouldReturnFocusAfterClose={false} to the Modal it will opt out of the focus handler. I have the parent component set this value to false in the componentWillUnmount and it has caused the warning to go away.

[diasbruno]

This is actually documented on the https://reactcommunity.org/react-modal/. (sorry)

[indiesquidge]

The problem with this is that I don't want to opt out of the focus manager. I just want to be able to render components in my test without every component that has a modal attached to it throwing this warning.

This check in returnFocus as it stands now is making a huge assumption in that it never expects a component to be manually unmounted, which is unrealistic.

This will warn on any test that unmounts—including Enzyme unmounts—and will even show the warning with the default CRA setup.

[Jacob-Allen]

@indiesquidge I definitely agree that this should not be the default behavior. I agree that this opt out should not be required.

I opened this issue because I think the part of

making a huge assumption

is in fact an undesirable feature in the current implementation of this package.

I have only referenced this workaround as a way to silence the warning, not as justification of a permanent intended fix.

I know the warning could simply be removed, but ideally the contributors will expand on this logic and auto opt out of the focus manager before a dismount.

[indiesquidge]

I would love to help improve the sophistication of the focus manager to better work with unmounted components.

Potential Solution 1: using isMounted flag

Without a deep dive into this codebase, the simplest solution I can think of is simply to use a flag that tracks whether or not the component is mounted, could be as simple as

// in <ModalPortal>

componentDidMount () {
  this._isMounted = true
}

// later …

componentWillUnmount () {
  this._isMounted = false
}

Then when we call returnFocus on the afterClose instance method, we could check this flag

afterClose = () => {
  if (this.props.shouldReturnFocusAfterClose && this._isMounted) {
    // where returnFocus is called; only runs if the component is still mounted
  }
}

I tested this out in a small CRA build and it works as expected (i.e. warnings are gone, all tests still pass, returnFocus still works in browser).

Potential Solution 2: check for empty focusLaterElements array

Another option would be to simple check if there are any elements in the focusLaterElements array in returnFocus and if there aren't simply return

// in focusManager.js

returnFocus () {
  if (focusLaterElements.length === 0) {
    return
  }
  // …
}

And have a public function on the focus manager to reset focusLaterElements to an empty array when the component unmounts

// in focusManager.js

export function clearFocusLaterElements () {
  focusLaterElements.length = 0
}

// in <ModalPortal>

componentWillUnmount () {
  focusManager.clearFocusLaterElements()
}

---

Both of these solutions seem to solve the issue, but I'm willing to bet I'm not handling some edge case perhaps. @diasbruno, anything come to mind on an edge case I might be missing? Any preference for either solution? Would it be worth my time to get something like this up in a PR?

[diasbruno]

@indiesquidge Checking for the length of focusLaterElements looks nice, but this may hide a bug if the focus manager has a real bug (failed/miss to queue the element, for example). We can temporarily apply this patch though. :+1:

Using the mounted flag on afterClose looks a bit weird because the only way it can/should be called is if the ModalPortal is actually mounted. This is something we can investigate (afterClose been called when it shouldn't).

@royrwood pointed out that it's possible to get this warning even if the modal is not open. (Can you get a reproducible example for this?).

Since this is a warning, I think we can look for bugs on the focus manager and the paths that uses this API.

[Jacob-Allen]

@diasbruno

possible to get this warning even if the modal is not open

My implementation of react-modal is only rarely opened. It mounts closed and dismounts closed 99% of the time and I always get this warning without the opt out.

[diasbruno]

It is often that the modal is never opened on certain pages.

@Jacob-Allen I miss this part. :)

[diasbruno]

@Jacob-Allen @indiesquidge @royrwood

Here is a place to dig in Modal.js#L144. It's possible that on componentWillUnmount of the modal is scheduling the close action event when already closed.

[indiesquidge]

@diasbruno, I think I may need a bit more context to understand the scope of this issue if I am to help find an adequate solution.

Using the mounted flag on afterClose looks a bit weird because the only way it can/should be called is if the ModalPortal is actually mounted.

Could you explain why you think afterClose is being called when it shouldn't be? From what I understand (and from what other people are reporting here), ModalPortal is mounted when the component using the modal is mounted, not when the modal itself is opened. (This can be verified in the examples by putting console statements in componentWillUnmount and observing that the function is never called save on the React Router example.)

One could say that afterClose is perhaps poorly named (or at the very least does not make sense to be invoked in componentWillUnmount with all of its current behavior), but I don't see why it shouldn't be called.

[indiesquidge]

Here is a place to dig in Modal.js#L144. It's possible that on componentWillUnmount of the modal is scheduling the close action event when already closed.

@diasbruno, I think I am confused by your recommendation here. <Modal> does not seem to make any reference to the focusManager—don't we want to be looking for a fix in <ModalPortal>, or am I misunderstanding something?

[diasbruno]

I'm referring to a possible path that can trigger afterClose.

[indiesquidge]

Checking for the length of focusLaterElements looks nice, but this may hide a bug if the focus manager has a real bug (failed/miss to queue the element, for example).

So the way I am understanding it, the way that elements are added to the queue is via the focusManager.markForFocusLater() function, which is currently only ever used once in the open instance method of the <ModalPortal>. That open instance method is only ever called twice (in componentDidMount and componentWillReceiveProps), and both times it is called it is behind a predicate of this.props.isOpen.

Reconciling this with when focusManager.returnFocus() is called (the function that is the source of this warning issue), which is only ever used in the afterClose instance method of <ModalPortal>. That afterClose instance method is only ever called twice: once in componentWillUnmount, another invocation in the instance method closeWithoutTimeout as the callback to setState().

Now, running the tests with the codebase as it stands results in 12 focus warnings by my count. If we simply mimic the predicate on which open is invoked (i.e. this.props.isOpen must be true) for afterClose, we see the number of focus warnings in the test drop down 75% (to only 3 warnings).

componentWillUnmount() {
  if (this.props.isOpen) {
    this.afterClose();
  }
  clearTimeout(this.closeTimer);
}

Proposed Solution

So I've been playing around with the source code for a while now, and despite the above snippet removing 75% of the test case warnings, there is an overarching issue at hand here: we are often calling focusLaterElements.pop() on an empty array.

There is an even simpler solution that gets rid of 100% of the test warnings

let toFocus = null;
try {
+ if (focusLaterElements.length !== 0) {
    toFocus = focusLaterElements.pop();
    toFocus.focus();
+ }
  return;
} catch (e) {
  // …
}

That's it. Just don't try and call pop() on an empty array, otherwise we will always find ourselves in the catch block.

Now to address your concern mentioned above

Checking for the length of focusLaterElements looks nice, but this may hide a bug if the focus manager has a real bug (failed/miss to queue the element, for example).

I'm not entirely sure I see a way in which an element misses the queue. As I laid out at the top of this comment, an element can only ever be added if the open instance method is invoked, which is always behind a this.props.isOpen flag. Appending to the focusLaterElements array is not async in any way, so (AFAICT) there is not any case in which an element can miss the queue if the modal is opened.

The only theoretical situation I can come up with is if the element to return the focus to (i.e. whichever element opened the modal in the first place I would think), somehow asynchronously disappeared after the modal was already opened. I can't possibly see a use case for such an event, and even if there was it seems like it break WebAIM guidelines.

If there is some other situation in which the element to return the focus to will not exist after the modal is opened, please let me know.

As it stands though, I think that the simple predicate check that focusLaterElements is not an empty array solves 99% of these warning issues.

[diasbruno]

@indiesquidge looks good!