Skip to content

Commit c0e1bf9

Browse files
sean-perkinssean-perkins
authored
fix(react): inline overlays dismiss when parent component unmounts (#26245)
Resolves #25775, #26185
1 parent ac0330d commit c0e1bf9

File tree

6 files changed

+204
-56
lines changed

6 files changed

+204
-56
lines changed

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import DynamicIonpageClassnames from './pages/dynamic-ionpage-classnames/Dynamic
3737
import Tabs from './pages/tabs/Tabs';
3838
import TabsSecondary from './pages/tabs/TabsSecondary';
3939
import Params from './pages/params/Params';
40+
import Overlays from './pages/overlays/Overlays';
4041

4142
setupIonicReact();
4243

@@ -60,6 +61,7 @@ const App: React.FC = () => {
6061
<Route path="/tabs" component={Tabs} />
6162
<Route path="/tabs-secondary" component={TabsSecondary} />
6263
<Route path="/refs" component={Refs} />
64+
<Route path="/overlays" component={Overlays} />
6365
<Route path="/params/:id" component={Params} />
6466
</IonRouterOutlet>
6567
</IonReactRouter>

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,12 @@ const Main: React.FC<MainProps> = () => {
5555
<IonItem routerLink="/dynamic-ionpage-classnames">
5656
<IonLabel>Dynamic IonPage Classnames</IonLabel>
5757
</IonItem>
58-
<IonItem routerLink="/Refs">
58+
<IonItem routerLink="/refs">
5959
<IonLabel>Refs</IonLabel>
6060
</IonItem>
61+
<IonItem routerLink="/overlays">
62+
<IonLabel>Overlays</IonLabel>
63+
</IonItem>
6164
<IonItem routerLink="/tabs" id="go-to-tabs">
6265
<IonLabel>Tabs</IonLabel>
6366
</IonItem>

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import { IonButton, IonContent, IonModal } from '@ionic/react';
2+
import { useState } from 'react';
3+
import { useHistory } from 'react-router';
4+
5+
const Overlays: React.FC = () => {
6+
const [isOpen, setIsOpen] = useState(false);
7+
8+
const history = useHistory();
9+
10+
const goBack = () => history.goBack();
11+
const replace = () => history.replace('/');
12+
const push = () => history.push('/');
13+
14+
return (
15+
<>
16+
<IonButton id="openModal" onClick={() => setIsOpen(true)}>
17+
Open Modal
18+
</IonButton>
19+
<IonModal
20+
isOpen={isOpen}
21+
onDidDismiss={() => {
22+
setIsOpen(false);
23+
}}
24+
>
25+
<IonContent>
26+
<IonButton id="goBack" onClick={goBack}>
27+
Go Back
28+
</IonButton>
29+
<IonButton id="replace" onClick={replace}>
30+
Replace
31+
</IonButton>
32+
<IonButton id="push" onClick={push}>
33+
Push
34+
</IonButton>
35+
</IonContent>
36+
</IonModal>
37+
</>
38+
);
39+
};
40+
41+
export default Overlays;

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
const port = 3000;
2+
3+
describe('Overlays', () => {
4+
it('should remove the overlay when going back to the previous route', () => {
5+
// Requires navigation history to perform a pop
6+
cy.visit(`http://localhost:${port}`);
7+
cy.visit(`http://localhost:${port}/overlays`);
8+
9+
cy.get('#openModal').click();
10+
11+
cy.get('ion-modal').should('exist');
12+
13+
cy.get('#goBack').click();
14+
15+
cy.get('ion-modal').should('not.exist');
16+
});
17+
18+
it('should remove the overlay when pushing to a new route', () => {
19+
cy.visit(`http://localhost:${port}/overlays`);
20+
21+
cy.get('#openModal').click();
22+
23+
cy.get('ion-modal').should('exist');
24+
25+
cy.get('#push').click();
26+
27+
cy.get('ion-modal').should('not.exist');
28+
});
29+
30+
it('should remove the overlay when replacing the route', () => {
31+
cy.visit(`http://localhost:${port}/overlays`);
32+
33+
cy.get('#openModal').click();
34+
35+
cy.get('ion-modal').should('exist');
36+
37+
cy.get('#replace').click();
38+
39+
cy.get('ion-modal').should('not.exist');
40+
});
41+
});

‎packages/react/src/components/createInlineOverlayComponent.tsx‎

Lines changed: 74 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { OverlayEventDetail } from '@ionic/core/components';
1+
import type { HTMLIonOverlayElement, OverlayEventDetail } from '@ionic/core/components';
22
import React, { createElement } from 'react';
33

44
import {
@@ -9,6 +9,7 @@ import {
99
mergeRefs,
1010
} from './react-component-lib/utils';
1111
import { createForwardRef } from './utils';
12+
import { detachProps } from './utils/detachProps';
1213

1314
// TODO(FW-2959): types
1415

@@ -35,7 +36,7 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
3536
}
3637
const displayName = dashToPascalCase(tagName);
3738
const ReactComponent = class extends React.Component<IonicReactInternalProps<PropType>, InlineOverlayState> {
38-
ref: React.RefObject<HTMLElement>;
39+
ref: React.RefObject<HTMLIonOverlayElement>;
3940
wrapperRef: React.RefObject<HTMLElement>;
4041
stableMergedRefs: React.RefCallback<HTMLElement>;
4142

@@ -54,65 +55,43 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
5455
componentDidMount() {
5556
this.componentDidUpdate(this.props);
5657

57-
/**
58-
* Mount the inner component when the
59-
* overlay is about to open.
60-
*
61-
* For ion-popover, this is when `ionMount` is emitted.
62-
* For other overlays, this is when `willPresent` is emitted.
63-
*/
64-
this.ref.current?.addEventListener('ionMount', () => {
65-
this.setState({ isOpen: true });
66-
});
67-
68-
/**
69-
* Mount the inner component
70-
* when overlay is about to open.
71-
* Also manually call the onWillPresent
72-
* handler if present as setState will
73-
* cause the event handlers to be
74-
* destroyed and re-created.
75-
*/
76-
this.ref.current?.addEventListener('willPresent', (evt: any) => {
77-
this.setState({ isOpen: true });
58+
this.ref.current?.addEventListener('ionMount', this.handleIonMount);
59+
this.ref.current?.addEventListener('willPresent', this.handleWillPresent);
60+
this.ref.current?.addEventListener('didDismiss', this.handleDidDismiss);
61+
}
7862

79-
this.props.onWillPresent && this.props.onWillPresent(evt);
80-
});
63+
componentDidUpdate(prevProps: IonicReactInternalProps<PropType>) {
64+
const node = this.ref.current! as HTMLElement;
65+
attachProps(node, this.props, prevProps);
66+
}
8167

68+
componentWillUnmount() {
69+
const node = this.ref.current;
8270
/**
83-
* Unmount the inner component.
84-
* React will call Node.removeChild
85-
* which expects the child to be
86-
* a direct descendent of the parent
87-
* but due to the presence of
88-
* Web Component slots, this is not
89-
* always the case. To work around this
90-
* we move the inner component to the root
91-
* of the Web Component so React can
92-
* cleanup properly.
71+
* If the overlay is being unmounted, but is still
72+
* open, this means the unmount was triggered outside
73+
* of the overlay being dismissed.
74+
*
75+
* This can happen with:
76+
* - The parent component being unmounted
77+
* - The overlay being conditionally rendered
78+
* - A route change (push/pop/replace)
79+
*
80+
* Unmounting the overlay at this stage should skip
81+
* the dismiss lifecycle, including skipping the transition.
82+
*
9383
*/
94-
this.ref.current?.addEventListener('didDismiss', (evt: any) => {
95-
const wrapper = this.wrapperRef.current;
96-
const el = this.ref.current;
97-
84+
if (node && this.state.isOpen) {
9885
/**
99-
* This component might be unmounted already, if the containing
100-
* element was removed while the popover was still open. (For
101-
* example, if an item contains an inline popover with a button
102-
* that removes the item.)
86+
* Detach the local event listener that performs the state updates,
87+
* before dismissing the overlay, to prevent the callback handlers
88+
* executing after the component has been unmounted. This is to
89+
* avoid memory leaks.
10390
*/
104-
if (wrapper && el) {
105-
el.append(wrapper);
106-
this.setState({ isOpen: false });
107-
}
108-
109-
this.props.onDidDismiss && this.props.onDidDismiss(evt);
110-
});
111-
}
112-
113-
componentDidUpdate(prevProps: IonicReactInternalProps<PropType>) {
114-
const node = this.ref.current! as HTMLElement;
115-
attachProps(node, this.props, prevProps);
91+
node.removeEventListener('didDismiss', this.handleDidDismiss);
92+
node.remove();
93+
detachProps(node, this.props);
94+
}
11695
}
11796

11897
render() {
@@ -172,6 +151,46 @@ export const createInlineOverlayComponent = <PropType, ElementType>(
172151
static get displayName() {
173152
return displayName;
174153
}
154+
155+
private handleIonMount = () => {
156+
/**
157+
* Mount the inner component when the
158+
* overlay is about to open.
159+
*
160+
* For ion-popover, this is when `ionMount` is emitted.
161+
* For other overlays, this is when `willPresent` is emitted.
162+
*/
163+
this.setState({ isOpen: true });
164+
};
165+
166+
private handleWillPresent = (evt: any) => {
167+
this.setState({ isOpen: true });
168+
/**
169+
* Manually call the onWillPresent
170+
* handler if present as setState will
171+
* cause the event handlers to be
172+
* destroyed and re-created.
173+
*/
174+
this.props.onWillPresent && this.props.onWillPresent(evt);
175+
};
176+
177+
private handleDidDismiss = (evt: any) => {
178+
const wrapper = this.wrapperRef.current;
179+
const el = this.ref.current;
180+
181+
/**
182+
* This component might be unmounted already, if the containing
183+
* element was removed while the overlay was still open. (For
184+
* example, if an item contains an inline overlay with a button
185+
* that removes the item.)
186+
*/
187+
if (wrapper && el) {
188+
el.append(wrapper);
189+
this.setState({ isOpen: false });
190+
}
191+
192+
this.props.onDidDismiss && this.props.onDidDismiss(evt);
193+
};
175194
};
176195
return createForwardRef<PropType, ElementType>(ReactComponent, displayName);
177196
};

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import { isCoveredByReact } from '../react-component-lib/utils';
2+
3+
/**
4+
* The @stencil/react-output-target will bind event listeners for any
5+
* attached props that use the `on` prefix. This function will remove
6+
* those event listeners when the component is unmounted.
7+
*
8+
* This prevents memory leaks and React state updates on unmounted components.
9+
*/
10+
export const detachProps = (node: HTMLElement, props: any) => {
11+
if (node instanceof Element) {
12+
Object.keys(props).forEach((name) => {
13+
if (name.indexOf('on') === 0 && name[2] === name[2].toUpperCase()) {
14+
const eventName = name.substring(2);
15+
const eventNameLc = eventName[0].toLowerCase() + eventName.substring(1);
16+
if (!isCoveredByReact(eventNameLc)) {
17+
/**
18+
* Detach custom event bindings (not built-in React events)
19+
* that were added by the @stencil/react-output-target attachProps function.
20+
*/
21+
detachEvent(node, eventNameLc);
22+
}
23+
}
24+
});
25+
}
26+
};
27+
28+
const detachEvent = (
29+
node: Element & { __events?: { [key: string]: ((e: Event) => any) | undefined } },
30+
eventName: string
31+
) => {
32+
const eventStore = node.__events || (node.__events = {});
33+
/**
34+
* If the event listener was added by attachProps, it will
35+
* be stored in the __events object.
36+
*/
37+
const eventHandler = eventStore[eventName];
38+
if (eventHandler) {
39+
node.removeEventListener(eventName, eventHandler);
40+
eventStore[eventName] = undefined;
41+
}
42+
};

0 commit comments

Comments
 (0)