@react-aria/focus - Failed to execute 'createTreeWalker' on 'Document' #3877
Comments
|
This could be difficult on our end. I suspect you are correct that it's an issue with unmounting and calling blur. Unfortunately, there is code here that we do want to run after unmount. In a browser, this won't matter because the page will be gone. In a test, however, the code keeps running. If you were using fakeTimers, I'd say run them out. But it looks like you are not. So you might try an async sleep to run out the request animation frame that is probably driving this. then just await it in the afterEach of your test suite to allow time for these things to finish. otherwise, we'd need some way of knowing that the entire react tree/app is unmounting, not just the focus scope. it's also possible that this is related to #3393. I don't think it is, but linking them just in case since that issue was a huge pain to figure out. |
|
What code does need to run even after unmount? Other possible solution is to explicitly handle this case calling whatever's needed but respecting that From my understanding changing this should be fine because the only change is that now instead of throwing we are skipping the part that focuses first in scope let onBlur = (e) => {
// Firefox doesn't shift focus back to the Dialog properly without this
raf.current = requestAnimationFrame(() => {
// Use document.activeElement instead of e.relatedTarget so we can tell if user clicked into iframe
if (shouldContainFocus(scopeRef) && !isElementInChildScope(document.activeElement, scopeRef)) {
activeScope = scopeRef;
if (document.body.contains(e.target)) {
focusedNode.current = e.target;
focusedNode.current.focus();
- } else if (activeScope) {
+ } else if (activeScope && getScopeRoot(activeScope.current) !== null) {
focusFirstInScope(activeScope.current);
}
}
});
}; |
|
In the meantime I found a nicer workaround. If I change in my +<div>
<FocusScope restoreFocus contain>
<div ref={popoverRef} {...mergeProps(dialogProps, otherProps, overlayProps)}>
{children}
</div>
<DismissButton onDismiss={onClose} />
</FocusScope>
+</div>It works because |
|
Maybe I didn't quite follow, I'll have a look again tomorrow. Thanks for all the extra info. |
|
@snowystinger just a reminder that you wanted to take a look at this |
|
Thanks for the ping, you're right, that's not failing in isolation https://codesandbox.io/s/fancy-moon-siqtm0?file=/src/App.test.js I think in the example you showed where you placed a div around the FocusScope, scope[0] should actually be the scope sentinel that FocusScope renders, so it won't be popoverRef. Not something that really matters, just pointing it out. The parent element of that is still the new div you've added. I'm not sure how you could have a FocusScope that doesn't have a parent if it's rendered into the DOM though. It looks like the blur raf you're looking at is also cleaned up during unmount and that is when it would lose the parent element. So I don't quite understand how we're getting into that section of code at all.
just to clarify, is this a typo? I think it's that the raf is called in onBlur, not the other way around. onBlur should be synchronous to whenever the element loses focus, though React has some bugs around onBlur handling of unmounted components facebook/react#12363 Can you provide more information about your test environment? Maybe you can use whatever your project settings are in the codesandbox I started above. React version may matter as well. this check you've proposed does seem safe enough, so I'm happy to open a PR with it, but I want to try to make sure we aren't covering something up |
Yes, I meant to say "race condition between unmounting the element and calling requestAnimationFrame in onBlur"
And usually it calls RAF before unmounting so it works, but when it doesn't for some reason the above bug occurs I will work on the reproduction this or next week to confirm that or find another thesis |
|
So this is where the bug I linked to earlier comes into play a bit. See this codesandbox in each of the browsers https://codesandbox.io/s/jovial-benji-e3rsml?file=/src/App.js
You'll notice that Safari and Firefox don't fire any blur events. Only Chrome does. Both React and the browsers have bugs logged against them for not firing blur on unmount of a focused element. So I don't think that the steps outlined in your comment are what is happening. That said, I still don't have an idea of what IS happening. Luckily, you can reproduce the issue though, so I'll be relying on you to determine what's happening. Could be I'm doing something wrong and your order is correct, but we'll need to prove it. |
|
I got it 🥳 will post a reproduction soon |
|
I played a bit with this and established that
Keep in mind that in order to see the error you need at least 2 tests because error from the first test is visible only in the second test |
|
I noticed there's some mechanizm to cancelAnimationFrame on unmount so added some console log to check execution order let onBlur = (e) => {
+ console.log('blur')
// Firefox doesn't shift focus back to the Dialog properly without this
raf.current = requestAnimationFrame(() => {
+ console.log('RAF')
// Use document.activeElement instead of e.relatedTarget so we can tell if user clicked into iframe
if (shouldContainFocus(scopeRef) && !isElementInChildScope(document.activeElement, scopeRef)) {
activeScope = scopeRef;
if (document.body.contains(e.target)) {
focusedNode.current = e.target;
focusedNode.current.focus();
} else if (activeScope) {
focusFirstInScope(activeScope.current);
}
}
});
};
document.addEventListener('keydown', onKeyDown, false);
document.addEventListener('focusin', onFocus, false);
scope.forEach(element => element.addEventListener('focusin', onFocus, false));
scope.forEach(element => element.addEventListener('focusout', onBlur, false));
return () => {
+ console.log('unmount - remove onBlur handler')
document.removeEventListener('keydown', onKeyDown, false);
document.removeEventListener('focusin', onFocus, false);
scope.forEach(element => element.removeEventListener('focusin', onFocus, false));
scope.forEach(element => element.removeEventListener('focusout', onBlur, false));
};
}, [scopeRef, contain]);
// eslint-disable-next-line arrow-body-style
useEffect(() => {
return () => {
+ console.log('unmount - cancel RAF')
if (raf.current) {
cancelAnimationFrame(raf.current);
}
};
}, [raf]);
}When the tests passed it resulted in When the test failed it resulted in There seems to be some race condition here |
|
And now I can also confirm that when the tests pass RAF is called with overlay open |
|
Thanks for all the great information and, most importantly, the reproduction. Here's a way to reproduce it every time, regardless of the machine you're on, no more race condition and only one test needed. While doing this, I was able to determine that the root cause is that we added the same onBlur event listener to multiple elements. When any of them receives a blur event, they create a request animation frame on the same ref object. We don't clear them out before overwriting them. This meant we were leaking a RAF unintentionally. I've adjusted the code to fix the underlying issue. I've updated my PR with a much more minimal test reproducing the behavior. |
|
Great job finding the root cause 👏 Glad I was able to help |
🐛 Bug Report
I was unable to reproduce it with a minimal code but this error is making some of the tests in my project flaky.
I suspect this is caused in my example by some kind of race condition between unmounting the element and calling onBlur in requestAnimationFrame
In a private repo it is flaky, for one set of tests it works fine if I add an artificial delay after each test (I assume during this time requestAnimationFrame has time to fire before test env is destroyed)
🤔 Expected Behavior
No error thrown
As far as I know it can only happen when scope[0] is no longer part of the DOM, it is equal to document or root of shadow DOM
😯 Current Behavior
FocusScope's onBlur throws because
scope[0].parentElementisnulla quick investigation simplifies the stack trace to
💁 Possible Solution
Early return in
onBlurorfocusFirstInScopeand possibly other functions that might suffer from the same bug🔦 Context
I'm trying to use react-aria and test the components I create with it without nasty workarounds
(at the moment I have overridden document.createTreeWalker in jest so it doesn't throw)
💻 Code Sample
This is what fails in the private repo, but doesn't fail in isolation
and at least two tests like
🌍 Your Environment
🧢 Your Company/Team
ChiliPiper
The text was updated successfully, but these errors were encountered: