@react-aria/focus - Failed to execute 'createTreeWalker' on 'Document'

[szymonwawak]

🐛 Bug Report

It is a bug similar to https://github.com/adobe/react-spectrum/issues/4407 but there are some differences, which is why I decided to create a separate report.

When executing both component and e2e Cypress tests, sometimes after the modal is closed, an error Failed to execute 'createTreeWalker' on 'Document': parameter 1 is not of type 'Node'. pops up. I checked in on older versions of the library and the issue was already there. Also, I wrote Cypress tests using react-aria-components and the result was the same.

🤔 Expected Behavior

No error should be thrown.

😯 Current Behavior

The error is thrown when any element inside the modal FocusScope is focused, and the modal is closed by clicking on the button closing the modal. This part of the code executes when FocusScope is already unmounted and an error is thrown.

💁 Possible Solution

I saw this commit that could possibly solve the issue. Don't know why it was removed though.

🔦 Context

I'm testing the behavior of the modal implemented with the usage of react-aria. ~10% of test runs fail because of this issue on Chrome, on Firefox it is around 40%.

💻 Code Sample

I can't provide an exact sample but a really simple example behaves exactly the same. autoFocus isn't necessary - focusing input in the test also works, it just makes the test simpler.

const Example: VFC = () => {
  const modalState = useOverlayTriggerState({});

  return (
    <>
      <button onClick={modalState.open} data-qa="open">Open modal</button>
      {modalState.isOpen && (
        <div>
          <FocusScope restoreFocus contain autoFocus>
            <input type="text"/>
            <button onClick={modalState.close}/>
          </FocusScope>
        </div>  
      )}
    </>
  );
};

Test:

describe("Error example", () => {
  Cypress._.times(100, () => {
    it("", () => {
      mount(<Example/>);
      cy.get("#open").should("be.visible").realClick();
      cy.get("#close").should("be.visible").realClick();
    });
  });
});

🌍 Your Environment

Library: "@react-aria/focus": "3.12.0". React: 17.0.2 Cypress: 12.8.1 OS: Windows 10

[snowystinger]

That raf shouldn't be able to fire after unmount. It's cleaned up here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/focus/src/FocusScope.tsx#L406

That commit you noted was removed because it wasn't related to the root cause of the issue that PR was solving and we haven't had an example like this one.

I have a theory that it's not that raf, but rather a second one further down with cypress appearing to our code as a virtual cursor. Would you mind using patch-package or something to add a console log to https://github.com/adobe/react-spectrum/blob/main/packages/@react-aria/focus/src/focusSafely.ts#L31 ?

The fix may still be the same, but I'd like to verify what is actually happening, it may help us create a reproduction allowing us to test the fix.

[amrmetwalyy]

@snowystinger I also saw this issue when I upgraded my @react-aria/focus to ^3.12.0

[bsramming]

Hey @snowystinger, I'm also seeing the same crash though via a slightly different path. Below is the traceback:

Uncaught TypeError TypeError: Failed to execute 'createTreeWalker' on 'Document': parameter 1 is not of type 'Node'.
    at $9bf71ea28793e738$export$2d6ec8fc375ceafa (c:\Dev\web-ui\node_modules\@react-aria\focus\dist\packages\@react-aria\focus\src\FocusScope.tsx:674:1)
    at $9bf71ea28793e738$var$focusFirstInScope (c:\Dev\web-ui\node_modules\@react-aria\focus\dist\packages\@react-aria\focus\src\FocusScope.tsx:455:1)
    at <anonymous> (c:\Dev\web-ui\node_modules\@react-aria\focus\dist\packages\@react-aria\focus\src\FocusScope.tsx:656:1)
    --- requestAnimationFrame ---
    at <anonymous> (c:\Dev\web-ui\node_modules\@react-aria\focus\dist\packages\@react-aria\focus\src\FocusScope.tsx:638:1)

I ran a test and added the following check to the if statement on line 655:

getScopeRoot(treeNode.scopeRef.current) !== null

Doing that stopped the crashes. It turns out, in our case, that the parent element of treeNode.scopeRef.current was null.

[LFDanLu]

@bsramming Interesting, mind sharing what situation/configuration caused that crash? Was it that treeNode.scopeRef.current was an empty array?

[bsramming]

Hi @LFDanLu , We have a button that triggers an AlertDialog pop-up. The crash occurs after clicking the primary action button (Delete in this example). I can click the cancel button as much as I want, and the crash doesn't happen. Functionality wise, things are still working after clicking the primary button: the on-delete logic is successful, DB is updated, and the pop-up dialog goes away. But right after the Dialog goes away is when the crash happens. And though it happens frequently, there are times when we won't see the crash after clicking the Dialog's primary action button. Yay for intermittent bugs!

The treeNode.scopeRef.current is an array with 1 HTMLDivElement component in it. I added some debug to the code to print this out:

0: div.spectrum_b37d53.spectrum_2a241c.custom-dark_spectrum--dark__Ckpzp.spectrum--medium_4b172c.custom_spectrum__epJpE.spectrum--medium_9e130c.spectrum--large_9e130c.custom_spectrum--darkest__jA7ZS.custom_spectrum--dark__Mp-Da.custom_spectrum--light__MKZSj.custom_spectrum--lightest__qflBa
accessKey: ""
align: ""
ariaAtomic: null
ariaAutoComplete: null
ariaBrailleLabel: null
ariaBrailleRoleDescription: null
ariaBusy: null
ariaChecked: null
ariaColCount: null
ariaColIndex: null
ariaColSpan: null
ariaCurrent: null
ariaDescription: null
ariaDisabled: null
ariaExpanded: null
ariaHasPopup: null
ariaHidden: null
ariaInvalid: null
ariaKeyShortcuts: null
ariaLabel: null
ariaLevel: null
ariaLive: null
ariaModal: null
ariaMultiLine: null
ariaMultiSelectable: null
ariaOrientation: null
ariaPlaceholder: null
ariaPosInSet: null
ariaPressed: null
ariaReadOnly: null
ariaRelevant: null
ariaRequired: null
ariaRoleDescription: null
ariaRowCount: null
ariaRowIndex: null
ariaRowSpan: null
ariaSelected: null
ariaSetSize: null
ariaSort: null
ariaValueMax: null
ariaValueMin: null
ariaValueNow: null
ariaValueText: null
assignedSlot: null
attributeStyleMap: StylePropertyMap {size: 12}
attributes: NamedNodeMap {0: class, 1: lang, 2: dir, 3: style, class: class, lang: lang, dir: dir, style: style, length: 4}
autocapitalize: ""
autofocus: false
baseURI: "http://localhost:3000/"
childElementCount: 1
childNodes: NodeList [div]
children: HTMLCollection [div]
classList: DOMTokenList(11) ['spectrum_b37d53', 'spectrum_2a241c', 'custom-dark_spectrum--dark__Ckpzp', 'spectrum--medium_4b172c', 'custom_spectrum__epJpE', 'spectrum--medium_9e130c', 'spectrum--large_9e130c', 'custom_spectrum--darkest__jA7ZS', 'custom_spectrum--dark__Mp-Da', 'custom_spectrum--light__MKZSj', 'custom_spectrum--lightest__qflBa', value: 'spectrum_b37d53 spectrum_2a241c custom-dark_spectr…um--light__MKZSj custom_spectrum--lightest__qflBa']
className: "spectrum_b37d53 spectrum_2a241c custom-dark_spectrum--dark__Ckpzp spectrum--medium_4b172c custom_spectrum__epJpE spectrum--medium_9e130c spectrum--large_9e130c custom_spectrum--darkest__jA7ZS custom_spectrum--dark__Mp-Da custom_spectrum--light__MKZSj custom_spectrum--lightest__qflBa"
clientHeight: 0
clientLeft: 0
clientTop: 0
clientWidth: 0
contentEditable: "inherit"
dataset: DOMStringMap {}
dir: "ltr"
draggable: false
elementTiming: ""
enterKeyHint: ""
firstChild: div
firstElementChild: div
hidden: false
id: ""
inert: false
innerHTML: "<div>...</div>"
innerText: "Delete ProjectThis cannot be undone.CancelDelete"
inputMode: ""
isConnected: false
isContentEditable: false
lang: "en-US"
lastChild: div
lastElementChild: div
localName: "div"
namespaceURI: "http://www.w3.org/1999/xhtml"
nextElementSibling: null
nextSibling: null
nodeName: "DIV"
nodeType: 1
nodeValue: null
nonce: ""
offsetHeight: 0
offsetLeft: 0
offsetParent: null
offsetTop: 0
offsetWidth: 0
onabort: null
onanimationend: null
onanimationiteration: null
onanimationstart: null
onauxclick: null
onbeforecopy: null
onbeforecut: null
onbeforeinput: null
onbeforematch: null
onbeforepaste: null
onbeforetoggle: null
onbeforexrselect: null
onblur: null
oncancel: null
oncanplay: null
oncanplaythrough: null
onchange: null
onclick: null
onclose: null
oncontentvisibilityautostatechange: null
oncontextlost: null
oncontextmenu: null
oncontextrestored: null
oncopy: null
oncuechange: null
oncut: null
ondblclick: null
ondrag: null
ondragend: null
ondragenter: null
ondragleave: null
ondragover: null
ondragstart: null
ondrop: null
ondurationchange: null
onemptied: null
onended: null
onerror: null
onfocus: null
onformdata: null
onfullscreenchange: null
onfullscreenerror: null
ongotpointercapture: null
oninput: null
oninvalid: null
onkeydown: null
onkeypress: null
onkeyup: null
onload: null
onloadeddata: null
onloadedmetadata: null
onloadstart: null
onlostpointercapture: null
onmousedown: null
onmouseenter: null
onmouseleave: null
onmousemove: null
onmouseout: null
onmouseover: null
onmouseup: null
onmousewheel: null
onpaste: null
onpause: null
onplay: null
onplaying: null
onpointercancel: null
onpointerdown: null
onpointerenter: null
onpointerleave: null
onpointermove: null
onpointerout: null
onpointerover: null
onpointerrawupdate: null
onpointerup: null
onprogress: null
onratechange: null
onreset: null
onresize: null
onscroll: null
onscrollend: null
onsearch: null
onsecuritypolicyviolation: null
onseeked: null
onseeking: null
onselect: null
onselectionchange: null
onselectstart: null
onslotchange: null
onstalled: null
onsubmit: null
onsuspend: null
ontimeupdate: null
ontoggle: null
ontransitioncancel: null
ontransitionend: null
ontransitionrun: null
ontransitionstart: null
onvolumechange: null
onwaiting: null
onwebkitanimationend: null
onwebkitanimationiteration: null
onwebkitanimationstart: null
onwebkitfullscreenchange: null
onwebkitfullscreenerror: null
onwebkittransitionend: null
onwheel: null
outerHTML: "<div>...</div>"
outerText: "Delete ProjectThis cannot be undone.CancelDelete"
ownerDocument: document
parentElement: null
parentNode: null
part: DOMTokenList [value: '']
popover: null
prefix: null
previousElementSibling: null
previousSibling: null
role: null
scrollHeight: 0
scrollLeft: 0
scrollTop: 0
scrollWidth: 0
shadowRoot: null
slot: ""
spellcheck: true
style: CSSStyleDeclaration {0: 'isolation', 1: 'background-image', 2: 'background-position-x', 3: 'background-position-y', 4: 'background-size', 5: 'background-repeat-x', 6: 'background-repeat-y', 7: 'background-attachment', 8: 'background-origin', 9: 'background-clip', 10: 'background-color', 11: 'color-scheme', accentColor: '', additiveSymbols: '', alignContent: '', alignItems: '', alignSelf: '', …}
tabIndex: -1
tagName: "DIV"
textContent: "Delete ProjectThis cannot be undone.CancelDelete"
title: ""
translate: true
virtualKeyboardPolicy: ""
[[Prototype]]: HTMLDivElement

Not sure how useful that is.

[LFDanLu]

Thanks for the snippet. If I'm not mistaken that seems to be the outer Provider div that wraps the AlertDialog. That div will exist in the FocusScope's Tree but typically is removed before this portion of the code is triggered. Additionally, that portion of the code shouldn't get hit if there is a nodeToRestore still available in the DOM (aka usually the AlertDialog trigger button).

Two questions:

1. Does the AlertDialog trigger button you mentioned still exist in the DOM when closing the AlertDialog? Ideally it should be refocusing that button instead of trying to looking for the first element in the nearest ancestor scope.
2. Does this line not fire before this line?

[bsramming]

Thanks for the reply @LFDanLu.

You got me thinking about our structure. To provide a little context we have a ProjectList component that shows a list of Project items. Each project item has a Delete button associated with it so it can be removed from the list. That button will trigger the AlertDialog. This AlertDialog is contained in a DialogContainer component. Executing the primary action of the AlertDialog will remove the Project from the DB and then refresh the project list. This refresh will remove the Project item that just got deleted from the list component which means that the Delete button that triggered the AlertDialog has been removed. Maybe there is a race condition here? I tried pushing the project refresh logic up the stack and only trigger it via a state variable set when the DialogContainer's onDismiss function is called. This didn't have the results I was hoping for, but still experimenting.

For your second question, the function focusFirstInScope is getting called before focusScopeTree.removeTreeNode.

[LFDanLu]

I see, very strange that focusFirstInScope is getting called before focusScopeTree.removeTreeNode, they both are triggered as part of the various hook cleanups on unmount for the same scopeRef (aka the wrapping Provider div) but the focusFirstInScope is delayed by a requestAnimationFrame and thus usually run after removeTreeNode... If you create a basic DialogContainer + AlertDialog combo in your project without a unmounting trigger I assume the focusFirstInScope happens before this raf fires right?

The Delete button being removed from the list component before the Dialog unmounts isn't exactly a problem persay since the portion of FocusScope code that is breaking here should then handle moving focus to a node in an existing ancestor FocusScope and shouldn't blowup if it doesn't find one. Definitely would simplify things if the trigger still exists until the Dialog fully unmounts though.

As an aside I think ideally focus should land on a project adjacent to the one being deleted (similar to what happens in our useGridState) but that is something that FocusScope won't be able to handle here, needs to be done in the ProjectList probably. This is tangential to the issue at hand though haha.

[bsramming]

Hi @LFDanLu , thanks for the additional inputs. I did a little re-work on my code and moved the basic DialogContainer + AlertDialog combo to the ProjectList component. It follows the example here pretty closely. But the ActionButton component is in the ProjectListItem instead. The individual ProjectListItem components now simply have an ActionButton for delete. That delete button will call the SetState function passed into it's props from ProjectList to trigger showing the dialog (i.e. isOpen = true).

Unfortunately making this change I get the same crash. I set some breakpoints and did notice the following:

1. Click the Delete button on the ProjectListItem component.
2. Before the dialog comes up, the breakpoint on focusScopeTree.removeTreeNode was hit.
3. Click continue in the debugger, then the dialog comes up.
4. Click the Delete button on the dialog.
5. Breakpoint on focusFirstInScope is hit.
6. Continue, and the crash happens.
7. Breakpoint on focusScopeTree.removeTreeNode is hit.

So I still see that focusFirstInScope getting called before focusScopeTree.removeTreeNode. But I do see focusScopeTree.removeTreeNode getting called right before the dialog is opened, but maybe that's expected?

[LFDanLu]

Odd that you are getting focusScopeTree.removeTreeNode firing before the dialog comes up, are you running with React.StrictMode? I'm at a loss as to why step 5 happens before step 7 in your project, at this point we'll have to try and slowly peel away any custom changes that differ from the base experience or figure out what differences exist between the doc example and your implementation. Some questions to get started:

1. From your descriptions it sounds like you are using the React Spectrum components directly and not the react aria/stately hooks or React Aria Components. Is this accurate? Are there any custom implementations involved with the Dialog/DialogContainer here?
2. What version of @adobe/react-spectrum are you on? Any other packages from our library? React version?
3. Can you share a rough sandbox or code snippet showing how things are implemented? I see you said it follows the docs example pretty closely, but what are the differences exactly? If it follows the example basically 1:1 in implementation that makes it even odder that it works in docs + my local but not in your application.
4. In your debug breakpoints, can you confirm that they are running against the same scopeRef (aka the wrapping Provider div around the Dialog)?

Adding getScopeRoot(treeNode.scopeRef.current) !== null like you did to 655 or something to focusFirstInScope to guard against the case where the scope parent is undefined feels like something we could consider doing anyways, just trying to understand what might cause this issue in the first place.

[bsramming]

Thanks @LFDanLu! Really appreciate the responses. I'm starting to peal back the different layers and reduce the code to the bare minimum and see what custom settings we have that would be impacting this. It's a bit tedious :) Will share any breakthroughs when I come across them. To answer your questions:

• We do have react StrictMode enabled. I did a quick test and turned it off. Still see the crash.
• We do have some custom implementations around Dialog/DialogContainter, but my recent tests and debug have been with those removed. There might still be more hidden ones that I'm looking into as I peel back the different components.
• Versions:
• React = 18.2
• react-spectrum = 3.21.0
• Lot of spectrum-css
• I hope to get the code into a rough sandbox state to be able to share. Might take some time unfortunately :(
• Yep confirmed in debugger that both breakpoints are referencing the same scopeRef

Totally understand wanting to get to the bottom of this and root cause the issue

[bsramming]

Hi @LFDanLu , after some delays I was able to determine what was happening in our code base. I looks like we had a race condition with deleting and refreshing our components. After clicking the primary button on a dialog to delete an item from our list, our components were not blocking while that delete was happening so they were re-rendering immediately. Once I added the check to wait for deletion to complete we never saw the crash again. These components are wrapped in a series of context's to share state across components which made it a little tricky to debug at first. At this point we are happy with our current solution. Thanks for all the suggestions and point me in the right direction.

[LFDanLu]

Thats great to hear, awesome that you were able to find the root cause!

[skrivle]

Hi All 👋 ,

We are also running into this issue quite a lot. So far it's does not seem to be causing any user facing bugs, but errors regularly show up in our Sentry logging. Also we're able to reproduce this in a browser (Chrome) manually, though it does seem to be intermittent.

I've tried to come up with a minimal reproduction, but did not succeed unfortunately. I'm also not able to share our code so I'm fully aware this does not make it easy to debug 😓 .

In our case it seem to be occurring quite a lot with our Modal component. In this specific case we have a list of <Item> components. When the user wants to delete an item a Confirmation Modal is shown. Every <Item> component renders it's own Modal component whenever the delete button is clicked. When the user confirms the delete the <Item> component is removed . We're using Apollo graphql to fire the mutation and refetch the data.

Our modal component looks a little bit like this:

const BaseModal = ({
    role,
    header,
    description,
    isOpen,
    hideCloseButton = false,
    contentClassNames,
    onRequestClose = () => {},
    children,
}: BaseModalProps) => {
    const dialogRef = useRef(null);

    const state = useOverlayTriggerState({
        isOpen,
        onOpenChange(open) {
            if (!open) {
                onRequestClose();
            }
        },
    });

    const { modalProps, underlayProps } = useModalOverlay(
        { isDismissable: true },
        state,
        dialogRef,
    );

    if (!state.isOpen) {
        return null;
    }

    return (
        <Overlay>
                <div
                    className="sc-c-overlayV2"
                    {...underlayProps}
                >
                        <Dialog
                            {...modalProps}
                            role={role}
                            header={header}
                            description={description}
                            dialogRef={dialogRef}
                            onRequestClose={onRequestClose}
                            contentClassNames={contentClassNames}
                            hideCloseButton={hideCloseButton}
                        >
                            {children}
                        </Dialog>
                </div>
        </Overlay>
    );
};

We're seeing this happening when the bug occurs:

=> focusFirstInScope is called via useLayoutEffect of useRestoreFocus. https://github.com/adobe/react-spectrum/blob/a399db1e314243a023f7b5c95e319eba4496604c/packages/%40react-aria/focus/src/FocusScope.tsx#L665

The treeNode.scopeRef.current seems to contain our modal underlay (sc-c-overlayV2)

=> With focusFirstinScope, getScopeRoot is called with the modal underlay https://github.com/adobe/react-spectrum/blob/a399db1e314243a023f7b5c95e319eba4496604c/packages/%40react-aria/focus/src/FocusScope.tsx#L455C39-L455C51

=> scopeRoot tries to get the parentElement of our underlay, but it’s already removed from the dom so it’s non-existent? https://github.com/adobe/react-spectrum/blob/a399db1e314243a023f7b5c95e319eba4496604c/packages/%40react-aria/focus/src/FocusScope.tsx#L278

then getFocusableTreeWalker throws because document.createTreeWalker is called with an undefined root variable.

It looks like in other places there's some protection against nodes being removed from the DOM, but in this specific scenario that doesn't seem to be case.

We are using react-aria@3.27.0

Hope this helps!

Thanks for looking into this 🙏 !

[snowystinger]

It sounds like the restoreFocus starts to work in the correct scope, and it has a nodeToRestore. That's good.

Can you show a bit of what the component in charge of the Items is doing? Could you try wrapping it in a FocusScope as well? Just like we do here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/listbox/src/ListBoxBase.tsx#L119

Otherwise, are there any animations or anything else at play here? Any delays in removing the items themselves? whether network or otherwise

I started a codesandbox based on what you've described using RSP components, though maybe it needs to be converted to RAC or hooks https://codesandbox.io/s/silent-sunset-n6vnc4?file=/src/App.js Either way, I'm not seeing the issue yet, though doesn't mean it won't happen with the right setup.

[skrivle]

Hi,

Thank you so much for looking into this 🙏 . I was quickly checking the repro that you put together, convinced that it would not show the issue as you said. Though, surprisingly I was able to reproduce 😅 . It does not seems to occur all the time but it does show up quite a lot.

It does seem that changing the onDelete function to use a function instead of updated state directly, makes the issue go away. The behaviour is also different. I'm not sure if it was set up like this on purpose?

let onDelete = (key) => {
    setItems((items) => items.filter((item) => item.key !== key));
  };

Tested on Chrome Version 116.0.5845.187

To answer your questions:

• The <Item> component seems to be pretty similar to what you have put together in the repro. Not much special going on in there. We are using React Aria hooks though... I'll try wrapping it with FocusScope and report back!
• Yes, there are delays at play in our case. Whenever the user presses the confirmation button in the modal we will fire a graphql mutation and wait for this the resolve. As a result of said mutation, the list of items will be refresh and the <Item> will no longer be there.

[snowystinger]

Oo, this is fantastic news then. I'll have my teammates try it and see if any of us can debug a little more

[snowystinger]

Ok, I took a look. There are a couple things going on.

First, good catch with this

let onDelete = (key) => {
  setItems((items) => items.filter((item) => item.key !== key));
};

As it was set up, it'd be acting on the old items because while the function would update, the Collections' caching would not rerender, so the remove button would still use the old version which would actually reintroduce the previously deleted row. Your change addresses that. You could also address that in a few other ways. Just in case you run into it again. Such as, you could store the onDelete function in each item's object in the collection.

However, when it brought back an element, I noticed there was an animation in play. So I tried this on a table with only one item. I've updated my codesandbox and the problem presents itself again. Thankfully this is a smaller example :)

When I placed this setup into a unit test, I didn't get the error, but I did get our warning about DialogTrigger being placed into something which unmounts it forcibly. I was actually surprised we didn't get it in the browser case. https://github.com/adobe/react-spectrum/blob/f29502a37a11c8b294850b6dc3783f43bec85cbd/packages/%40react-spectrum/dialog/src/DialogTrigger.tsx#L64 because doing this would probably also solve the problem you have. I recommend doing it regardless of what happens in this Issue. https://react-spectrum.adobe.com/react-spectrum/DialogContainer.html#dialogcontainer

I'll have to check with others to see if we can create a unit test for this. However, I think the correct thing to do is to add a check before we call focusFirst to check if the scope is actually still in the document, otherwise, continue our search up the tree for something to focus. I've put that together here: https://github.com/adobe/react-spectrum/pull/5131 I think this more correct than just failing to restore focus silently by checking if the element exists in getScopeRoot.

[skrivle]

Hi,

Thanks for looking into this again! Good to hear that you were able to come up with a more specific reproduction!

Regarding the DialogContainer: We are using React Aria's hooks, which to my knowledge, don't come with such a component or hook. But I guess the general idea is to make sure the Modal is not unmounted when it is still open? So for us that would probably mean moving our <ConfirmDialog> out of the <Item/> component and into our <List> component so that we can keep rendering it even when the corresponding <Item> is being removed...

[snowystinger]

Ah, yep, that's Spectrum specific. So yeah, you'll just want to handle it outside of the Item, as far up as makes sense