bug: Conditionally rendering ionModal in react - Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

[corysmc]

Prerequisites

• I have read the Contributing Guidelines.
• I agree to follow the Code of Conduct.
• I have searched for existing issues that already report this problem, without success.

Ionic Framework Version

• v4.x
• v5.x
• v6.x
• Nightly

Current Behavior

Conditionally rendering an IonModal, breaks when dismissing when using ionic v6.

Observed behavior: white/blank screen.

Error in console:
react-dom.development.js:10301 Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

Expected Behavior

In an ideal scenario - the ionic modal could be conditionally rendered (without using the isOpen prop) this is a common pattern in react applications.

Steps to Reproduce

1. Conditionally render an IonModal

...
const [showIonModal, setShowIonModal] = useState(false);
...

return (
<div>
  <IonButton onClick={() => setShowIonModal(true)}>
    Show Modal
  </IonButton>
  {showIonModal && (
    <IonModal isOpen={true} onDidDismiss={() => setShowIonModal(false)}>
      <IonHeader>
        <IonToolbar>
          <IonTitle>Content</IonTitle>
        </IonToolbar>
      </IonHeader>
      <IonContent fullscreen>
        Ionic Modal Content here!
        <IonButton color="danger" onClick={() => setShowIonModal(false)}>
          Hide Modal (will break)
        </IonButton>
      </IonContent>
    </IonModal>
  )}
</div>
)
...

2. Dismiss the modal (so that IonModal is pulled from the dom)
3. Observe the error.

Code Reproduction URL

https://github.com/corysmc/react-overlay-bug

Ionic Info

Ionic:

Ionic CLI : 6.15.0
Ionic Framework : @ionic/react 6.1.13

Utility:

cordova-res : not installed
native-run : 1.6.0

System:

NodeJS : v14.16.0
npm : 6.14.11
OS : macOS Big Sur

Additional Information

Related:
#24887

[sean-perkins]

@corysmc thanks for opening this issue!

I tried to recreate a similar example in Angular to see what the behavior was there. While it doesn't error out, it does highlight a few problematic side effects of conditionally rendering modals in this way.

1. There is no dismiss animation for the modal.
2. None of the dismiss behavior fires (no dismiss events are emitted).

Since the framework is performing the remove operation, a lot of internal behavior is skipped. Modal is not currently architected to perform expected dismiss behavior when the node is removed outside of calling dismiss() or performing specific actions.

I'll need to compare with React and another modal library implementation to see how they handle this. I'll also need to explore useIonModal more and see if that's a more applicable pattern for achieving this behavior.

[JohnGeorgiadis]

I am having the same issue. I have a list of items with ids and when I click on an item I open a modal passing this id. If the id is null I unmount the modal.

I get the same error when onDismiss is being fired. MESSAGE Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

 {selectedViewGroupId && (
        <GroupInfoModal
          handleDismissModal={() => setSelectedViewGroupId(null)}
          id={selectedViewGroupId}
          isOpen={!!selectedViewGroupId}
        />
      )}

this code was working on v5.

[corysmc]

@sean-perkins I understand that useIonModal is the way around the issue, however - this is a very common and preferred pattern for react applications. When using the hook useIonModal - in a full scale application you end up having to wrap every modal hook with providers to ensure it works properly. When using a conditionally rendered modal - all the providers are there.

Also, take a look at the linked repo - the dismiss animation still works because it's called in the useLayoutEffect callback that is called before unmounting 😁

https://github.com/corysmc/react-overlay-bug/blob/development/src/components/CustomModal.tsx

[sean-perkins]

Another issue popped up that we believe to be related to the problematic behavior here: https://github.com/jameson-w-taylor/modal-state-error

Rendering new child nodes before an inline modal, during the modal dismiss, causes the same exception as originally reported.

Over simplified code:

{items.map(item => <IonCard />)}
<IonModal ref={modal} onWillDismiss={() => {
  setItems(...items, 'Another one');
}}>
  <IonButton onClick={() => modal.current.dismiss()}>Close</IonButton>
</IonModal>

FYI we do consider this a bug, but leaving in triage status just to keep it top-of-queue to try and get a better grasp of where the root problem exists.

[Fooweb]

Hello! Just wondering when an update will be made to correct this. Thanks!

[sean-perkins]

Ok I have been able to diagnose the root issue and have a better understanding of the work effort to resolve it.

The IonModal implementation in React does not have a framework delegate configured. This results in us manually removing the element from the DOM, instead of relying on React to re-render and remove both the React component and the element from the DOM.

Fortunately, my recent work with implementing the framework delegate into React for IonNav overlaps with this.

I'll classify this now to get it into our backlog. I'll create a dev build if my quick exploration is successful.

[JohnGeorgiadis]

@sean-perkins it works now as in v5. Overlay shadow is a bit weird for me, but I need to check that I did everything from the migration guide. Thanks!

[sean-perkins]

@JohnGeorgiadis can you provide an image or screencast of the before & after?

[Fooweb]

Hi Sean,

Thank you for the update. I tried to install the dev build but it's not working, I'm sure I'm doing something wrong.

Can you or someone help me with the command to update/install the dev build?

Thank you!

[Fooweb]

Thanks! I knew I was doing something wrong.

So the error is now gone and the Modal dismisses as expected without error however presenting the Modal has now changed and not being presented at the topmost layer but rather within the component.

I'm attaching two videos of before (IonModal Error.mov) and with the dev build (IonModal Dev Build.mov).

IonModal Error.mov
https://user-images.githubusercontent.com/88801453/180294020-2dece18e-1598-442d-bb81-e09b70abc8e8.mov

IonModal Dev Build.mov
https://user-images.githubusercontent.com/88801453/180294142-ee3a74f1-9ac1-4e2f-a6fe-6daf8bc51d39.mov

Thanks!

[Fooweb]

Something also seems broken with the router as the pages are reloading when switching from one page to another.

[sean-perkins]

@Fooweb can you share how your modal mark-up is structured so that I can create a test case to compare against? For example, here is the draft PR with the test cases for the two scenarios previously described: https://github.com/ionic-team/ionic-framework/pull/25663/files#diff-035351ce7a818e005cd493963c04ddc980772952f61d3d6ea2d591b23f80a401

Also, was your application previously on v6 prior to using the dev-build? I am unsure how the affected changes would impact routing, but I can test in a new application and see if I can reproduce.

[JohnGeorgiadis]

@sean-perkins so I tested it a bit more and I found some issues.

Routing: is not working on mobile (I was testing web desktop version before).
"react-router": "^5.2.0", "react-router-dom": "^5.2.0", .
I am being redirected to the default route every time I try to access any route. I also tried to install v6.3.0 but I guess it's not supported by ionic yet.

Modal on v5

Modal on v6

and I need to scroll down to see the actual modal

 <IonPage>
      <IonContent>

          {/* Refresher */}
          <IonRefresher slot="fixed" onIonRefresh={refreshPosts}>
            <IonRefresherContent />
          </IonRefresher>

          {isLoading ? (
            // Skeleton posts
            <div className="post-skeleton">
              <Loader />
            </div>
          ) : (
            // Loaded posts
            <div className="posts-container">
              {state.posts.posts.length ? (
                state.posts.posts.map(post => (
                  <>
                    <Post key={post.id} post={post} inModal={false} />
                    <div style={{ height: 8, backgroundColor: 'rgba(34, 108, 121, 0.1)' }} />
                  </>
                ))
              ) : (
                <NotFound
                  headerText={strings.notFoundFilterHeader}
                  contentText={strings.notFoundFilterContet}
                  onRedirectClick={() => window.location.replace('/posts')}
                />
              )}
            </div>
          )}
          <IonInfiniteScroll threshold="100px" onIonInfinite={loadMorePosts} disabled={state.posts.posts.length === 0}>
            <IonInfiniteScrollContent loadingText={strings.loadMore} />
          </IonInfiniteScroll>
      </IonContent>
    </IonPage>

[sean-perkins]

@JohnGeorgiadis can you try isolating the mock representation of your modal in a new starter app or help elaborate where the modal mark-up is used in your code snippet?

I am assuming that the Post component is responsible for presenting a modal? Or when setting inModal to true that causes some internal code to Post to present the view in a modal?

This bug is a focus of my upcoming sprint, so I am confident we can get to the bottom of whatever inconsistencies exist here. As part of this work effort, I am trying to move off the legacy class components and migrate to functional components (allows us to leverage JSX/TSX without manually calling React APIs). There is likely a small implementation detail in this migration that is inconsistent with the previous class component implementation.

[JohnGeorgiadis]

@sean-perkins I installed the latest version of the package and I still have the same issues regarding routing and modal's placement.

Post.tsx

<IonModal isOpen={true} onDidDismiss={handleDismissModal}>
      <ModalHeader modalTitle={strings.post} onBackButtonClicked={handleDismissModal} />
      <IonContent ref={contentRef}>
        <Post post={selectedPost} inModal={true} />

        {selectedPost?.allowComments && (
          <div ref={commentsRef}>
            <Comments
              postId={selectedPost?.id}
              comments={selectedPost?.comments}
              replyParentComment={replyParentComment}
              onReplyClick={setParentComment}
            />
          </div>
        )}
      </IonContent>

      {selectedPost?.allowComments && (
        <IonFooter>
          <IonToolbar>
            <NewCommentBar
              ref={newCommentBarRef}
              parentComment={replyParentComment}
              postId={selectedPost?.id}
              focusOnModalPresent={focusCommentInput}
              unsetParentComment={unsetParentComment}
              setShowMentionsUserList={setShowMentionsUserList}
              setMentionsSearchString={setMentionsSearchString}
              onSubmit={onSubmitComment}
            />
          </IonToolbar>
        </IonFooter>
      )}
    </IonModal>

I will try to create a new app and add my logic.

[lcorniglione]

Here is an updated dev-build to test with in the interim: 6.1.16-dev.11659112821.197cb2ec.

Accounts for some incorrect unmounting behavior from the migration from class components to functional components.

I can confirm that at least for me, the dev-build it's working correctly.

[Fooweb]

Hi Sean.

I'm still seeing the same behavior with the modal as with my previous post (illustrated by the videos). I will try and see if I can recreate my code in a new app and share it. Is there a preferred place to share the code? CodePen, CodeSandbox, StackBlitz...?

Thanks!

[sean-perkins]

@Fooweb a Github repository based off the Ionic starters would be great.

[Fooweb]

@sean-perkins - Thank you for helping me through this! After sliming down the app to just using the modal, I found a bug in my own code which was causing the issue. I was conditionally rendering a component and attempting to update another component that hadn't been created yet. This is what was causing my issue. Thank you again for helping me through this!

[ohmycode]

@Fooweb do you mean the fixes from the dev build are no longer necessary in your case?
I've upgraded my app from ionic-react 5 -> 6 and I'm getting exactly the same " Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node." error while live reloading during development when I update some code inside the Modal.

[sean-perkins]

Hello everyone, appreciate the collaboration here!

Here is an updated dev-build that I believe is our final draft: 6.2.3-dev.11660337759.18ea0f7e. Please let me know if you run into any issues.

The only difference in behavior you should be seeing in this build, is that inline overlays that are not conditionally rendered, will be rendered within a div.

e.g.:

<div>
  <IonModal>
    ...
  </IonModal>
</div>

This should not have any negative impact on your application, but helps solve for detached nodes in React.

[corysmc]

Once I try to use @ionic/[email protected] navigation is broken. Downgrading to 6.2.2 router works fine - but the issue outlined above is still present.

Tapping on a navigation link, changes the URL, then refreshes the app back to the root url

The same thing happened when I downloaded the react conference app - and upgraded to this version of ionic/react.

[corysmc]

@sean-perkins I had the same routing issue with this build as @Fooweb and @JohnGeorgiadis reported
but the issue I saw with routing was because I needed to install two dev versions to test:

@ionic/[email protected] and @ionic/[email protected]

This build is working much better - however there's still an edge case that I have a hack fix for. When navigating away from a route that renders a modal - the modal stays open. So this is what I've done to "fix" it.

import React, { ComponentProps, useLayoutEffect, useRef } from 'react';
import { IonModal } from '@ionic/react';

type TnModalProps = ComponentProps<typeof IonModal>;

const TnModal: React.FC<TnModalProps> = (props) => {
  const { children, ...ionModalProps } = props;
  const modalRef = useRef<HTMLIonModalElement>(null);

  useLayoutEffect(
    () => () => {
      modalRef.current?.dismiss();
    },
    []
  );

  return (
    <IonModal ref={modalRef} {...ionModalProps}>
      {children}
    </IonModal>
  );
};

export default TnModal;

[corysmc]

@sean-perkins
Here's a video of the simple demo I put together based off of the ionic-react-conference app
This pattern (route based IonModal) worked in v5 - and I've been told this is a common pattern in a react app.

The IonModal is rendered via a route with <IonModal isOpen>...

...
<IonContent>
  <IonButton href="/support/new">New Support Ticket</IonButton>
</IonContent>
<Switch>
  <Route path="/support/new" exact component={SupportModal} />
</Switch>
...

https://github.com/corysmc/ionic-react-conference-app

Video:

ionic-react-modal-route-demo-480p.mov

[sean-perkins]

@corysmc the behavior of the modal remaining presented when using back navigation appears to be an issue reproducible in main and is not isolated to the dev-build.

We will need to report that as a new issue, so it can be triaged and prioritized in our backlog.

If that is the only outstanding issue here, I believe the dev-build still resolves the original reported issue.

[corysmc]

Ah yep, you're right. I filed the original bug - and it does solve the filed issue - but still feels like an unpolished solution.

I'll file a new bug - thanks!

[JohnGeorgiadis]

@sean-perkins and @corysmc
Thanks for your time and effort. I can say that the last build for @ionic/react and @ionic/react-router solved my issue with rendering a modal conditionally. I will continue testing today and let you know if I see anything weird.
Cheers!

[JohnGeorgiadis]

Hello @sean-perkins,
I found another issue using

    "@ionic/react": "6.2.3-dev.11660337759.18ea0f7e",
    "@ionic/react-router": "6.2.3-dev.11660337759.18ea0f7e",

Let's say I have a component like that

const [showModal, setShowModal] = useState(false);
...
return (
  {showModal && <ShowModal onDismiss={() => setShowModal(false)}/>}
)

and inside the modal

 <IonModal isOpen={true} onDidDismiss={onDismiss}>
      <IconButton onClick={onDismiss} isFormDirty={isFormDirty} headerTitle={strings.newPost}  />
      MENU
 </IonModal>

When I click on the Icon I can see that the function is being fired, but the modal is still visible.
Using the Esc button when modal is visible, removes the modal first and then fires the onDidDismiss as expected.

[sean-perkins]

@JohnGeorgiadis can you try updating your component so that isOpen is driven by the state of the component and not always true?

For example:

const [isOpen, setIsOpen] = useState(true);

const onDismiss = () => {
  setIsOpen(false);
};

return (
  <IonModal isOpen={isOpen} onDidDismiss={onDismiss}>
  
  </IonModal>
);

The IonModal uses a React Portal to render at the root of the DOM. Likely when the dismiss state changes, the wrapper component is destroyed by React, but since the IonModal is contextually "open", it is still being rendered at the root of the DOM. Let me know if binding to state doesn't fix that issue!

There is still a few more remaining items here after discussing with the team & trying to solve for a change in v5->v6 where navigation events are not dismissing inline overlays. I will post a new dev-build when I figure that out 👍

[JohnGeorgiadis]

Hello @sean-perkins ,

I tried the fix with having const [isOpen, setIsOpen] = useState(true); inside the modal component. Unfortunately it does not dismiss the modal.

[sean-perkins]

@JohnGeorgiadis can you test with this updated build and let me know if the problem persists?

npm i @ionic/[email protected] @ionic/[email protected]

This build does some heavier lifting of supporting the conditional rendering of IonModal and the isOpen usage, while maintaining the previous behavior behind trigger-based overlays.

[JohnGeorgiadis]

@sean-perkins it looks like it works thanks for your hard work.
Can you please elaborate a bit further on why is this happening and what changed on ionic 6?
Also is there a recommended way of using modal in ionic 6 with react?

[JohnGeorgiadis]

@sean-perkins I know that you will love this, but I found a small bug with the new modal and IonDatetimeButton.
So using

 "@ionic/react": "6.2.4",
 "@ionic/react-router": "6.2.4",

I can see a <IonDatetime/> inside a modal.
Using your build version 6.2.5-dev.11661455187.182badba DatePicker modal is not visible.

How I use datepicker

          <IonItem>
            <IonLabel position="floating" style={{ color: '#226C79', marginTop: 0 }}>
              {strings.startDate}
            </IonLabel>

            <IonDatetimeButton datetime="startDate" className="updateUserForm__datetimeBtn" />

            <IonModal keepContentsMounted={true}>
              <IonDatetime
                id="startDate"
                preferWheel
                presentation="date"
                name="startDate"
                value={formik.values.startDate}
                onIonChange={formik.handleChange}
                showDefaultButtons
                doneText={strings.datePickerOkText}
                cancelText={strings.datePickerCancelText}
                color="primary"
                className="updateUserForm__datetime"
              />
            </IonModal>
          </IonItem>

Version 6.2.4

Version 6.2.5-dev.11661455187.182badba

you can see on the last image that the picker is selected and there is no modal visible.

Thanks!

[JohnGeorgiadis]

@sean-perkins after some investigation it seems that there is an issue with the order of the modals. When I click on the start date I can see that the <IonModal> which has the <IonDatetime> inside is indeed being mounted, but behind the edit profile modal. Probably there is an issue with zindex here?

[sean-perkins]

@JohnGeorgiadis appreciate your testing here! I'll need to do a little more investigation here. The current approach to solving conditionally rendering overlays is leading to a lot of branched behavior where it is only supported for a narrow configuration of overlays. I need to brainstorm with the team how we may alternatively support this.

Much of our overlay implementation is dependent on the overlay being rendered in the DOM. The usage of triggers requires the overlay to be in the DOM, to be able to emit a custom event to trigger the overlay to be presented. ion-datetime-button queries the closest modal or popover and there are other niche implementation patterns within our codebase. All of these do not play nicely with using React portals, which is the recommended architecture for solving the root problem.

[sean-perkins]

Hello everyone 👋 unsure what version we are up to now, but I have an updated dev-build!

npm install @ionic/[email protected] @ionic/[email protected]

This build has an opinionation that was required to make all the different ways we present overlays work correctly.

In this dev-build, when using triggers to present overlays, the overlay contents will always mount in the DOM. This means that it will internally set keepContentsMounted to true. You cannot use triggers without the contents being mounted. If you are using isOpen or the hooks to present overlays, you can defer mounting the React component until it is actually presented.

[JohnGeorgiadis]

Hello @sean-perkins ,

I tried your build but unfortunately I still have the issue with date-pickers inside modals described before.
Is this build addressing this issue?

Thanks :)

[sean-perkins]

@JohnGeorgiadis can you possibly isolate a small reproduction in a new repo?

I tried to replicate a scenario based on your code snippet above:

<IonDatetimeButton datetime="startDate" />
<IonModal keepContentsMounted={true}>
  <IonDatetime
    id="startDate"
    preferWheel
    presentation="date"
    name="startDate"
    showDefaultButtons
    color="primary"
  />
</IonModal>

& I see the modal correctly displayed:

[JohnGeorgiadis]

Hello @sean-perkins
https://github.com/JohnGeorgiadis/ion-datetime-button-issue here it is. I can still reproduce it.

I am loading a modal on tab1 with state isOpen.

Modal is opened with a datetimebutton

I cannot see the picker when I click on the modal

Maybe I am missing something obvious here.

[sean-perkins]

@JohnGeorgiadis perfect, thank you for the reproduction!

Here is an updated dev-build that I tested against your reproduction:

npm install @ionic/[email protected] @ionic/[email protected]

When presenting any overlay, we are now re-assigning the z-index value to make it the top overlay. Previously we didn't do this because inline overlays were being reconstructed when presented. We need to vet the impact of this change (appreciate any extra testing that overlays work correctly in your apps!), but if this change lands as-is, should be a minor performance improvement to inline overlays, since it reuses the DOM node and only does a style re-calc for the z-index change.

[JohnGeorgiadis]

@sean-perkins it works! I can see that the modals are loading in the right order now.
Thanks!

[puopg]

awesome!

[puopg]

Could I ask how long until this issue is resolved? Im waiting on this to address the other related issue (#25775) before I upgrade from v5 to v6. It's quite common we have a dialog rendered and then a component is unmounted.

[BAYiTUPAi]

Hoping for an update on this as well.

[sean-perkins]

The pull request to resolve this issue is open for review. Once the team has an opportunity to review the changes and any required changes are addressed, this will be available in an upcoming patch release.

In the interim, I greatly appreciate any additional testing and verification with the dev-build posted above.

[sean-perkins]

Hello everyone 👋 the team identified a few issues with the proposed dev-build that would be considering a breaking change to accessing refs and querying overlay elements.

We have collaborated on an alternative approach that could ship in v6.x.x. I have independently verified the test cases of this thread, but would appreciate additional testing/verification on the new dev-build:

npm install @ionic/[email protected] npm install @ionic/[email protected]

Pending a team review, we could ship this fix in an upcoming patch release.

Thanks!

[ionitron-bot]

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.