diff --git a/packages/react-router/test-app/src/App.tsx b/packages/react-router/test-app/src/App.tsx index 3a2314f285..cdfa67d136 100644 --- a/packages/react-router/test-app/src/App.tsx +++ b/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 = () => { + diff --git a/packages/react-router/test-app/src/pages/Main.tsx b/packages/react-router/test-app/src/pages/Main.tsx index 83c882d0eb..cd4c5f6980 100644 --- a/packages/react-router/test-app/src/pages/Main.tsx +++ b/packages/react-router/test-app/src/pages/Main.tsx @@ -55,9 +55,12 @@ const Main: React.FC = () => { Dynamic IonPage Classnames - + Refs + + Overlays + Tabs diff --git a/packages/react-router/test-app/src/pages/overlays/Overlays.tsx b/packages/react-router/test-app/src/pages/overlays/Overlays.tsx new file mode 100644 index 0000000000..9343df83b9 --- /dev/null +++ b/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 ( + <> + setIsOpen(true)}> + Open Modal + + { + setIsOpen(false); + }} + > + + + Go Back + + + Replace + + + Push + + + + + ); +}; + +export default Overlays; diff --git a/packages/react-router/test-app/tests/e2e/specs/overlays.cy.js b/packages/react-router/test-app/tests/e2e/specs/overlays.cy.js new file mode 100644 index 0000000000..2557448b20 --- /dev/null +++ b/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'); + }); +}); diff --git a/packages/react/src/components/createInlineOverlayComponent.tsx b/packages/react/src/components/createInlineOverlayComponent.tsx index 03318e231e..70ea275222 100644 --- a/packages/react/src/components/createInlineOverlayComponent.tsx +++ b/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 = ( } const displayName = dashToPascalCase(tagName); const ReactComponent = class extends React.Component, InlineOverlayState> { - ref: React.RefObject; + ref: React.RefObject; wrapperRef: React.RefObject; stableMergedRefs: React.RefCallback; @@ -54,60 +55,9 @@ export const createInlineOverlayComponent = ( 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.props.onWillPresent && this.props.onWillPresent(evt); - }); - - /** - * 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. - */ - this.ref.current?.addEventListener('didDismiss', (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 popover was still open. (For - * example, if an item contains an inline popover with a button - * that removes the item.) - */ - if (wrapper && el) { - el.append(wrapper); - this.setState({ isOpen: false }); - } - - this.props.onDidDismiss && this.props.onDidDismiss(evt); - }); + this.ref.current?.addEventListener('ionMount', this.handleIonMount); + this.ref.current?.addEventListener('willPresent', this.handleWillPresent); + this.ref.current?.addEventListener('didDismiss', this.handleDidDismiss); } componentDidUpdate(prevProps: IonicReactInternalProps) { @@ -115,6 +65,35 @@ export const createInlineOverlayComponent = ( attachProps(node, this.props, prevProps); } + componentWillUnmount() { + const node = this.ref.current; + /** + * 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. + * + */ + if (node && this.state.isOpen) { + /** + * 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. + */ + node.removeEventListener('didDismiss', this.handleDidDismiss); + node.remove(); + detachProps(node, this.props); + } + } + render() { // eslint-disable-next-line @typescript-eslint/no-unused-vars const { children, forwardedRef, style, className, ref, ...cProps } = this.props; @@ -172,6 +151,46 @@ export const createInlineOverlayComponent = ( 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(ReactComponent, displayName); }; diff --git a/packages/react/src/components/utils/detachProps.ts b/packages/react/src/components/utils/detachProps.ts new file mode 100644 index 0000000000..fb7f136ecb --- /dev/null +++ b/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; + } +};