fix(react): inline overlays dismiss when parent component unmounts

packages/react-router/test-app/src/App.tsx

@@ -37,6 +37,7 @@ import DynamicIonpageClassnames from './pages/dynamic-ionpage-classnames/Dynamic
 import Tabs from './pages/tabs/Tabs';
 import TabsSecondary from './pages/tabs/TabsSecondary';
 import Params from './pages/params/Params';
+import Overlays from './pages/overlays/Overlays';
 
 setupIonicReact();
 
@@ -60,6 +61,7 @@ const App: React.FC = () => {
           <Route path="/tabs" component={Tabs} />
           <Route path="/tabs-secondary" component={TabsSecondary} />
           <Route path="/refs" component={Refs} />
+          <Route path="/overlays" component={Overlays} />
           <Route path="/params/:id" component={Params} />
         </IonRouterOutlet>
       </IonReactRouter>

packages/react-router/test-app/src/pages/Main.tsx

@@ -55,9 +55,12 @@ const Main: React.FC<MainProps> = () => {
           <IonItem routerLink="/dynamic-ionpage-classnames">
             <IonLabel>Dynamic IonPage Classnames</IonLabel>
           </IonItem>
-          <IonItem routerLink="/Refs">
+          <IonItem routerLink="/refs">
             <IonLabel>Refs</IonLabel>
           </IonItem>
+          <IonItem routerLink="/overlays">
+            <IonLabel>Overlays</IonLabel>
+          </IonItem>
           <IonItem routerLink="/tabs" id="go-to-tabs">
             <IonLabel>Tabs</IonLabel>
           </IonItem>

packages/react-router/test-app/src/pages/overlays/Overlays.tsx

@@ -0,0 +1,41 @@
+import { IonButton, IonContent, IonModal } from '@ionic/react';
+import { useState } from 'react';
+import { useHistory } from 'react-router';
+
+const Overlays: React.FC = () => {
+  const [isOpen, setIsOpen] = useState(false);
+
+  const history = useHistory();
+
+  const goBack = () => history.goBack();
+  const replace = () => history.replace('/');
+  const push = () => history.push('/');
+
+  return (
+    <>
+      <IonButton id="openModal" onClick={() => setIsOpen(true)}>
+        Open Modal
+      </IonButton>
+      <IonModal
+        isOpen={isOpen}
+        onDidDismiss={() => {
+          setIsOpen(false);
+        }}
+      >
+        <IonContent>
+          <IonButton id="goBack" onClick={goBack}>
+            Go Back
+          </IonButton>
+          <IonButton id="replace" onClick={replace}>
+            Replace
+          </IonButton>
+          <IonButton id="push" onClick={push}>
+            Push
+          </IonButton>
+        </IonContent>
+      </IonModal>
+    </>
+  );
+};
+
+export default Overlays;

packages/react-router/test-app/tests/e2e/specs/overlays.cy.js

@@ -0,0 +1,41 @@
+const port = 3000;
+
+describe('Overlays', () => {
+  it('should remove the overlay when going back to the previous route', () => {
+    // Requires navigation history to perform a pop
+    cy.visit(`http://localhost:${port}`);
+    cy.visit(`http://localhost:${port}/overlays`);
+
+    cy.get('#openModal').click();
+
+    cy.get('ion-modal').should('exist');
+
+    cy.get('#goBack').click();
+
+    cy.get('ion-modal').should('not.exist');
+  });
+
+  it('should remove the overlay when pushing to a new route', () => {
+    cy.visit(`http://localhost:${port}/overlays`);
+
+    cy.get('#openModal').click();
+
+    cy.get('ion-modal').should('exist');
+
+    cy.get('#push').click();
+
+    cy.get('ion-modal').should('not.exist');
+  });
+
+  it('should remove the overlay when replacing the route', () => {
+    cy.visit(`http://localhost:${port}/overlays`);
+
+    cy.get('#openModal').click();
+
+    cy.get('ion-modal').should('exist');
+
+    cy.get('#replace').click();
+
+    cy.get('ion-modal').should('not.exist');
+  });
+});

packages/react/src/components/createInlineOverlayComponent.tsx

@@ -1,4 +1,4 @@
-import type { OverlayEventDetail } from '@ionic/core/components';
+import type { HTMLIonOverlayElement, OverlayEventDetail } from '@ionic/core/components';
 import React, { createElement } from 'react';
 
 import {
@@ -9,6 +9,7 @@ import {
   mergeRefs,
 } from './react-component-lib/utils';
 import { createForwardRef } from './utils';
+import { detachProps } from './utils/detachProps';
 
 // TODO(FW-2959): types
 
@@ -35,7 +36,7 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
   }
   const displayName = dashToPascalCase(tagName);
   const ReactComponent = class extends React.Component<IonicReactInternalProps<PropType>, InlineOverlayState> {
-    ref: React.RefObject<HTMLElement>;
+    ref: React.RefObject<HTMLIonOverlayElement>;
     wrapperRef: React.RefObject<HTMLElement>;
     stableMergedRefs: React.RefCallback<HTMLElement>;
 
@@ -54,65 +55,43 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
     componentDidMount() {
       this.componentDidUpdate(this.props);
 
-      /**
-       * Mount the inner component when the
-       * overlay is about to open.
-       *
-       * For ion-popover, this is when `ionMount` is emitted.
-       * For other overlays, this is when `willPresent` is emitted.
-       */
-      this.ref.current?.addEventListener('ionMount', () => {
-        this.setState({ isOpen: true });
-      });
-
-      /**
-       * Mount the inner component
-       * when overlay is about to open.
-       * Also manually call the onWillPresent
-       * handler if present as setState will
-       * cause the event handlers to be
-       * destroyed and re-created.
-       */
-      this.ref.current?.addEventListener('willPresent', (evt: any) => {
-        this.setState({ isOpen: true });
+      this.ref.current?.addEventListener('ionMount', this.handleIonMount);
+      this.ref.current?.addEventListener('willPresent', this.handleWillPresent);
+      this.ref.current?.addEventListener('didDismiss', this.handleDidDismiss);
+    }
 
-        this.props.onWillPresent && this.props.onWillPresent(evt);
-      });
+    componentDidUpdate(prevProps: IonicReactInternalProps<PropType>) {
+      const node = this.ref.current! as HTMLElement;
+      attachProps(node, this.props, prevProps);
+    }
 
+    componentWillUnmount() {
+      const node = this.ref.current;
       /**
-       * Unmount the inner component.
-       * React will call Node.removeChild
-       * which expects the child to be
-       * a direct descendent of the parent
-       * but due to the presence of
-       * Web Component slots, this is not
-       * always the case. To work around this
-       * we move the inner component to the root
-       * of the Web Component so React can
-       * cleanup properly.
+       * If the overlay is being unmounted, but is still
+       * open, this means the unmount was triggered outside
+       * of the overlay being dismissed.
+       *
+       * This can happen with:
+       * - The parent component being unmounted
+       * - The overlay being conditionally rendered
+       * - A route change (push/pop/replace)
+       *
+       * Unmounting the overlay at this stage should skip
+       * the dismiss lifecycle, including skipping the transition.
+       *
        */
-      this.ref.current?.addEventListener('didDismiss', (evt: any) => {
-        const wrapper = this.wrapperRef.current;
-        const el = this.ref.current;
-
+      if (node && this.state.isOpen) {
         /**
-         * This component might be unmounted already, if the containing
-         * element was removed while the popover was still open. (For
-         * example, if an item contains an inline popover with a button
-         * that removes the item.)
+         * Detach the local event listener that performs the state updates,
+         * before dismissing the overlay, to prevent the callback handlers
+         * executing after the component has been unmounted. This is to
+         * avoid memory leaks.
          */
-        if (wrapper && el) {
-          el.append(wrapper);
-          this.setState({ isOpen: false });
-        }
-
-        this.props.onDidDismiss && this.props.onDidDismiss(evt);
-      });
-    }
-
-    componentDidUpdate(prevProps: IonicReactInternalProps<PropType>) {
-      const node = this.ref.current! as HTMLElement;
-      attachProps(node, this.props, prevProps);
+        node.removeEventListener('didDismiss', this.handleDidDismiss);
+        node.remove();
+        detachProps(node, this.props);
+      }
     }
 
     render() {
@@ -172,6 +151,46 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
     static get displayName() {
       return displayName;
     }
+
+    private handleIonMount = () => {
+      /**
+       * Mount the inner component when the
+       * overlay is about to open.
+       *
+       * For ion-popover, this is when `ionMount` is emitted.
+       * For other overlays, this is when `willPresent` is emitted.
+       */
+      this.setState({ isOpen: true });
+    };
+
+    private handleWillPresent = (evt: any) => {
+      this.setState({ isOpen: true });
+      /**
+       * Manually call the onWillPresent
+       * handler if present as setState will
+       * cause the event handlers to be
+       * destroyed and re-created.
+       */
+      this.props.onWillPresent && this.props.onWillPresent(evt);
+    };
+
+    private handleDidDismiss = (evt: any) => {
+      const wrapper = this.wrapperRef.current;
+      const el = this.ref.current;
+
+      /**
+       * This component might be unmounted already, if the containing
+       * element was removed while the overlay was still open. (For
+       * example, if an item contains an inline overlay with a button
+       * that removes the item.)
+       */
+      if (wrapper && el) {
+        el.append(wrapper);
+        this.setState({ isOpen: false });
+      }
+
+      this.props.onDidDismiss && this.props.onDidDismiss(evt);
+    };
   };
   return createForwardRef<PropType, ElementType>(ReactComponent, displayName);
 };

packages/react/src/components/utils/detachProps.ts

@@ -0,0 +1,42 @@
+import { isCoveredByReact } from '../react-component-lib/utils';
+
+/**
+ * The @stencil/react-output-target will bind event listeners for any
+ * attached props that use the `on` prefix. This function will remove
+ * those event listeners when the component is unmounted.
+ *
+ * This prevents memory leaks and React state updates on unmounted components.
+ */
+export const detachProps = (node: HTMLElement, props: any) => {
+  if (node instanceof Element) {
+    Object.keys(props).forEach((name) => {
+      if (name.indexOf('on') === 0 && name[2] === name[2].toUpperCase()) {
+        const eventName = name.substring(2);
+        const eventNameLc = eventName[0].toLowerCase() + eventName.substring(1);
+        if (!isCoveredByReact(eventNameLc)) {
+          /**
+           * Detach custom event bindings (not built-in React events)
+           * that were added by the @stencil/react-output-target attachProps function.
+           */
+          detachEvent(node, eventNameLc);
+        }
+      }
+    });
+  }
+};
+
+const detachEvent = (
+  node: Element & { __events?: { [key: string]: ((e: Event) => any) | undefined } },
+  eventName: string
+) => {
+  const eventStore = node.__events || (node.__events = {});
+  /**
+   * If the event listener was added by attachProps, it will
+   * be stored in the __events object.
+   */
+  const eventHandler = eventStore[eventName];
+  if (eventHandler) {
+    node.removeEventListener(eventName, eventHandler);
+    eventStore[eventName] = undefined;
+  }
+};