Null value from parent selector causes TypeError: Cannot read properties of null (reading 'removeChild')

[ADTC]

Summary:

Using a ref as a parent selector will cause a crash if the parent changed. Basically, if the parent selector gives a null this would cause the crash.

Steps to reproduce:

1. Use a ref's current as the parent element: parentSelector={parentRef && (() => parentRef.current as HTMLElement)}
2. Do some operation which will change the parent.
3. Observe the crash the next time the modal is invoked.

Expected behavior:

Ignore that the previous parent is now null, because it was already removed by React unmounting the referenced element.

Additional notes:

Suspected line of code:

react-modal/src/components/Modal.js

Line 161 in a275399

prevParent.removeChild(this.node);

This should check if (prevParent) before calling removeChild, or use conditional call: prevParent?.removeChild.

[ADTC]

I found #983 but I think parentSelector should allow null and internally just do null checks. In TypeScript, even the example from the documentation will not be allowed because querySelector returns HTMLElement | null.

<Modal
  // Type 'null' is not assignable to type 'HTMLElement'.
  // Type '() => HTMLElement | null' is not assignable to type '() => HTMLElement'
  // To fix, add `as HTMLElement` after the querySelector call:
  parentSelector={() => document.querySelector('#root')}>
  <p>Modal Content.</p>
</Modal>

The chance of this causing real-world problems may be low because we assume the parent is always present in the DOM, but it's not entirely impossible.

[ADTC]

EDIT: Don't do this! See my comment below.

Original comment: Currently, my very strange workaround, inspired by #769 (comment), is to return a dummy object if it's null:

(This is the TypeScript version:)

// EDIT: Don't do this! See my comment below.
<Modal
  parentSelector={
    parentRef &&
    (() =>
      parentRef.current ||
      ({
        contains: () => {},
        appendChild: () => {},
        removeChild: () => {},
      } as unknown as HTMLElement))
  }
/>
// EDIT: Don't do this!

See my comment below.

[diasbruno]

react-modal needs to know which element it will attach its component.
What should be the behavior if parentSelector is null?

[ADTC]

@diasbruno

What should be the behavior if parentSelector is null?

What does it do when this prop is omitted (undefined)? Same thing.

[diasbruno]

https://github.com/reactjs/react-modal/blob/master/src/components/Modal.js#L101

[diasbruno]

It will always default to document.body.

[ADTC]

This is good info. So at first glance it would seem I could actually do this: parentRef.current || document.body

The problem is, removeChild will throw an error if the child doesn't exist. (See MDN)

I don't suppose you have any error handling in place for this.

[diasbruno]

This is the requirement "the user must guarantee that a valid node is available for the lifetime and placement of any modal".

[ADTC]

How to guarantee it when using a ref?

The exact problem I'm trying to solve is stacking of multiple modals. If I show a new modal when one is already shown, the new modal can get drawn under the existing one. I'm finding the solution to this problem is to set the parent of the new modal to be an element inside the existing modal.

But because this is a ref, closing the existing modal would have React unmount it from the DOM. Since it no longer exists, the ref.current returns null. Is there the harm in just ignoring when it's null? If React unmount the parent element it would mean the modal itself is already gone. So there's no need to try to remove it.

(If there's a more proper or easier way of stacking modals correctly, please let me know.)

[diasbruno]

I always recommend to avoid document.body and elements in the main tree.

<div>
  <div id="app"></div>
  <div id="modals"></div>
</div>

This will make sure the parent will always exists, and, don't pollute the main tree.

There is also an example of how nesting modal (but I highly discourage doing this - bad UI/UX).

[ADTC]

With some testing, I determined that this is the correct way of stacking modals (not nesting). You need to have a separate parent root for each stacking layer: (These IDs are examples. You can name the ID anything you like.)

<div>
  <div id="app"></div>
  <div id="modals-layer-1"></div>
  <div id="modals-layer-2"></div>
</div>

Modals should primarily go into the first layer, so that's the default. But whenever you have to stack a second modal over an existing modal, you need to use the ID of the second layer. You can imagine it being something like this:

<Modal
  parentSelector={() => document.getElementById(`modals-layer-${isLayer2 ? 2 : 1}`) as HTMLElement}>
  <p>Modal Content.</p>
</Modal>

(Or you can pass in the layer Id as a prop: document.getElementById(layerId || 'modals-layer-1'). How you do this depends on how you're using the library. You can also use querySelector of course, just be sure to add the # as the CSS ID selector.)

Note: as HTMLElement is necessary in TypeScript as the result of the document property method can be null. But the onus is on you to ensure that the parent ALWAYS exists and will never be null.

[diasbruno]

Great! Grande lavoro! 🎉

There are a lot of ways to make this work, but I'm glad you've found one that suites your use case.

I'll close this for now. Let me know if you need anything.