-
Notifications
You must be signed in to change notification settings - Fork 808
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
'You tried to return focus to but it is not in the DOM anymore' warning. #577
Comments
Hi @Jacob-Allen, before the modal is open, it will put in a queue the |
Hey @diasbruno, |
There is an option we added recently - |
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. |
I'll keep this open, because this feature is not documented. |
@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). |
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 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 Thanks! |
I'm getting this warning, too. Tracing into it, the following code in 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, In my test case, I haven't actually displayed a modal yet, and still get the warning (i.e. my |
Hi @royrwood , @diasbruno mentioned that there is an undocumented prop to bypass the focusmanager. By adding |
This is actually documented on the https://reactcommunity.org/react-modal/. (sorry) |
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 This will warn on any test that unmounts—including Enzyme unmounts—and will even show the warning with the default CRA setup. |
@indiesquidge I opened this issue because I think the part of
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. |
I would love to help improve the sophistication of the focus manager to better work with unmounted components. Potential Solution 1: using
|
@indiesquidge Checking for the length of Using the mounted flag on @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. |
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. |
@Jacob-Allen I miss this part. :) |
@Jacob-Allen @indiesquidge @royrwood Here is a place to dig in Modal.js#L144. It's possible that on |
@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.
Could you explain why you think One could say that |
@diasbruno, I think I am confused by your recommendation here. |
I'm referring to a possible path that can trigger |
So the way I am understanding it, the way that elements are added to the queue is via the Reconciling this with when 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 componentWillUnmount() {
if (this.props.isOpen) {
this.afterClose();
}
clearTimeout(this.closeTimer);
} Proposed SolutionSo 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 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 Now to address your concern mentioned above
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 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 |
@indiesquidge looks good! |
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:
Expected behavior:
Additional notes:
The modals are behaving normally, I am just unsure why the focus manager is giving me this warning.
The text was updated successfully, but these errors were encountered: