Skip to content

Commit 850338c

Browse files
ShaneKShaneK
authored
fix(modal): dismiss modal when parent element is removed from DOM (#30544)
Issue number: resolves #30389 --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> Currently, when the element an ion-modal was presented from is removed, the modal stays presented and can be broken depending on the framework. This is unlike #30540, where children of open modals were being kept open. In this case, specifically the DOM element is being removed for whatever reason and the modal is staying open. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> We're now identifying our parent component on load and watching it with a mutation observer to determine if it gets removed from the DOM. If it does, we trigger a dismiss. This, conveniently, works nicely with #30540 and will dismiss all children and grandchildren as well. ## Does this introduce a breaking change? - [ ] Yes - [X] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> The issue this resolves was already marked closed, but on closer inspection I determined that was a mistake. I believed this issue was related to another one I was dealing with and it is, but it wasn't quite the same. After this issue is merged, I believe we will have handled all avenues of possibly ending up with broken modals because of parent elements or modals being removed. [Relevant Test Page](https://ionic-framework-git-fix-remove-modal-when-parent-removed-ionic1.vercel.app/src/components/modal/test/inline) **Current dev build:** ``` 8.6.5-dev.11752329407.10f7fc80 ```
1 parent b3b93c1 commit 850338c

File tree

3 files changed

+232
-2
lines changed

3 files changed

+232
-2
lines changed

‎core/src/components/modal/modal.tsx

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,11 @@ export class Modal implements ComponentInterface, OverlayInterface {
9696
private viewTransitionAnimation?: Animation;
9797
private resizeTimeout?: any;
9898

99+
// Mutation observer to watch for parent removal
100+
private parentRemovalObserver?: MutationObserver;
101+
// Cached original parent from before modal is moved to body during presentation
102+
private cachedOriginalParent?: HTMLElement;
103+
99104
lastFocus?: HTMLElement;
100105
animation?: Animation;
101106

@@ -398,6 +403,7 @@ export class Modal implements ComponentInterface, OverlayInterface {
398403
disconnectedCallback() {
399404
this.triggerController.removeClickListener();
400405
this.cleanupViewTransitionListener();
406+
this.cleanupParentRemovalObserver();
401407
}
402408

403409
componentWillLoad() {
@@ -407,6 +413,11 @@ export class Modal implements ComponentInterface, OverlayInterface {
407413
const attributesToInherit = ['aria-label', 'role'];
408414
this.inheritedAttributes = inheritAttributes(el, attributesToInherit);
409415

416+
// Cache original parent before modal gets moved to body during presentation
417+
if (el.parentNode) {
418+
this.cachedOriginalParent = el.parentNode as HTMLElement;
419+
}
420+
410421
/**
411422
* When using a controller modal you can set attributes
412423
* using the htmlAttributes property. Since the above attributes
@@ -642,6 +653,9 @@ export class Modal implements ComponentInterface, OverlayInterface {
642653
// Initialize view transition listener for iOS card modals
643654
this.initViewTransitionListener();
644655

656+
// Initialize parent removal observer
657+
this.initParentRemovalObserver();
658+
645659
unlock();
646660
}
647661

@@ -847,6 +861,7 @@ export class Modal implements ComponentInterface, OverlayInterface {
847861
this.gesture.destroy();
848862
}
849863
this.cleanupViewTransitionListener();
864+
this.cleanupParentRemovalObserver();
850865
}
851866
this.currentBreakpoint = undefined;
852867
this.animation = undefined;
@@ -1150,6 +1165,61 @@ export class Modal implements ComponentInterface, OverlayInterface {
11501165
});
11511166
}
11521167

1168+
private initParentRemovalObserver() {
1169+
if (typeof MutationObserver === 'undefined') {
1170+
return;
1171+
}
1172+
1173+
// Only observe if we have a cached parent and are in browser environment
1174+
if (typeof window === 'undefined' || !this.cachedOriginalParent) {
1175+
return;
1176+
}
1177+
1178+
// Don't observe document or fragment nodes as they can't be "removed"
1179+
if (
1180+
this.cachedOriginalParent.nodeType === Node.DOCUMENT_NODE ||
1181+
this.cachedOriginalParent.nodeType === Node.DOCUMENT_FRAGMENT_NODE
1182+
) {
1183+
return;
1184+
}
1185+
1186+
this.parentRemovalObserver = new MutationObserver((mutations) => {
1187+
mutations.forEach((mutation) => {
1188+
if (mutation.type === 'childList' && mutation.removedNodes.length > 0) {
1189+
// Check if our cached original parent was removed
1190+
const cachedParentWasRemoved = Array.from(mutation.removedNodes).some((node) => {
1191+
const isDirectMatch = node === this.cachedOriginalParent;
1192+
const isContainedMatch = this.cachedOriginalParent
1193+
? (node as HTMLElement).contains?.(this.cachedOriginalParent)
1194+
: false;
1195+
return isDirectMatch || isContainedMatch;
1196+
});
1197+
1198+
// Also check if parent is no longer connected to DOM
1199+
const cachedParentDisconnected = this.cachedOriginalParent && !this.cachedOriginalParent.isConnected;
1200+
1201+
if (cachedParentWasRemoved || cachedParentDisconnected) {
1202+
this.dismiss(undefined, 'parent-removed');
1203+
// Release the reference to the cached original parent
1204+
// so we don't have a memory leak
1205+
this.cachedOriginalParent = undefined;
1206+
}
1207+
}
1208+
});
1209+
});
1210+
1211+
// Observe document body with subtree to catch removals at any level
1212+
this.parentRemovalObserver.observe(document.body, {
1213+
childList: true,
1214+
subtree: true,
1215+
});
1216+
}
1217+
1218+
private cleanupParentRemovalObserver() {
1219+
this.parentRemovalObserver?.disconnect();
1220+
this.parentRemovalObserver = undefined;
1221+
}
1222+
11531223
render() {
11541224
const {
11551225
handle,

‎core/src/components/modal/test/inline/index.html

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,8 @@
2222
</ion-header>
2323

2424
<ion-content class="ion-padding">
25-
<button id="open-inline-modal" onclick="openModal(event)">Open Modal</button>
26-
2725
<div id="modal-container">
26+
<button id="open-inline-modal" onclick="openModal(event)">Open Modal</button>
2827
<ion-modal swipe-to-close="true">
2928
<ion-header>
3029
<ion-toolbar>
@@ -34,6 +33,9 @@
3433
<ion-content class="ion-padding">
3534
<p>This is my inline modal content!</p>
3635
<button id="open-child-modal" onclick="openChildModal(event)">Open Child Modal</button>
36+
<button id="remove-modal-container" onclick="removeModalContainer(event)">
37+
Remove Modal Container
38+
</button>
3739

3840
<ion-modal id="child-modal" swipe-to-close="true">
3941
<ion-header>
@@ -46,6 +48,9 @@
4648
<p>When the parent modal is dismissed, this child modal should also be dismissed automatically.</p>
4749
<button id="dismiss-parent" onclick="dismissParent(event)">Dismiss Parent Modal</button>
4850
<button id="dismiss-child" onclick="dismissChild(event)">Dismiss Child Modal</button>
51+
<button id="child-remove-modal-container" onclick="removeModalContainer(event)">
52+
Remove Modal Container
53+
</button>
4954
</ion-content>
5055
</ion-modal>
5156
</ion-content>
@@ -78,6 +83,14 @@
7883
childModal.isOpen = false;
7984
};
8085

86+
const removeModalContainer = () => {
87+
const container = document.querySelector('#modal-container');
88+
if (container) {
89+
container.remove();
90+
console.log('Modal container removed from DOM');
91+
}
92+
};
93+
8194
modal.addEventListener('didDismiss', () => {
8295
modal.isOpen = false;
8396
});

‎core/src/components/modal/test/inline/modal.e2e.ts

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,5 +122,152 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
122122
await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.firstElementChild!.className)
123123
).not.toContain('ion-page');
124124
});
125+
126+
test('it should dismiss modal when parent container is removed from DOM', async ({ page }) => {
127+
await page.goto('/src/components/modal/test/inline', config);
128+
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
129+
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');
130+
131+
const modal = page.locator('ion-modal').first();
132+
const modalContainer = page.locator('#modal-container');
133+
134+
// Open the modal
135+
await page.click('#open-inline-modal');
136+
await ionModalDidPresent.next();
137+
await expect(modal).toBeVisible();
138+
139+
// Remove the modal container from DOM
140+
await page.click('#remove-modal-container');
141+
142+
// Wait for modal to be dismissed
143+
const dismissEvent = await ionModalDidDismiss.next();
144+
145+
// Verify the modal was dismissed with the correct role
146+
expect(dismissEvent.detail.role).toBe('parent-removed');
147+
148+
// Verify the modal is no longer visible
149+
await expect(modal).toBeHidden();
150+
151+
// Verify the container was actually removed
152+
await expect(modalContainer).not.toBeAttached();
153+
});
154+
155+
test('it should dismiss both parent and child modals when parent container is removed from DOM', async ({
156+
page,
157+
}) => {
158+
await page.goto('/src/components/modal/test/inline', config);
159+
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
160+
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');
161+
162+
const parentModal = page.locator('ion-modal').first();
163+
const childModal = page.locator('#child-modal');
164+
const modalContainer = page.locator('#modal-container');
165+
166+
// Open the parent modal
167+
await page.click('#open-inline-modal');
168+
await ionModalDidPresent.next();
169+
await expect(parentModal).toBeVisible();
170+
171+
// Open the child modal
172+
await page.click('#open-child-modal');
173+
await ionModalDidPresent.next();
174+
await expect(childModal).toBeVisible();
175+
176+
// Remove the modal container from DOM
177+
await page.click('#child-remove-modal-container');
178+
179+
// Wait for both modals to be dismissed
180+
const firstDismissEvent = await ionModalDidDismiss.next();
181+
const secondDismissEvent = await ionModalDidDismiss.next();
182+
183+
// Verify at least one modal was dismissed with 'parent-removed' role
184+
const dismissRoles = [firstDismissEvent.detail.role, secondDismissEvent.detail.role];
185+
expect(dismissRoles).toContain('parent-removed');
186+
187+
// Verify both modals are no longer visible
188+
await expect(parentModal).toBeHidden();
189+
await expect(childModal).toBeHidden();
190+
191+
// Verify the container was actually removed
192+
await expect(modalContainer).not.toBeAttached();
193+
});
194+
195+
test('it should dismiss modals when top-level ancestor is removed', async ({ page }) => {
196+
// We need to make sure we can close a modal when a much higher
197+
// element is removed from the DOM. This will be a common
198+
// use case in frameworks like Angular and React, where an entire
199+
// page container for much more than the modal might be swapped out.
200+
await page.setContent(
201+
`
202+
<ion-app>
203+
<div class="ion-page">
204+
<ion-header>
205+
<ion-toolbar>
206+
<ion-title>Top Level Removal Test</ion-title>
207+
</ion-toolbar>
208+
</ion-header>
209+
<ion-content class="ion-padding">
210+
<div id="top-level-container">
211+
<div id="nested-container">
212+
<button id="open-nested-modal">Open Nested Modal</button>
213+
<ion-modal id="nested-modal">
214+
<ion-header>
215+
<ion-toolbar>
216+
<ion-title>Nested Modal</ion-title>
217+
</ion-toolbar>
218+
</ion-header>
219+
<ion-content class="ion-padding">
220+
<p>This modal's original parent is deeply nested</p>
221+
<button id="remove-top-level">Remove Top Level Container</button>
222+
</ion-content>
223+
</ion-modal>
224+
</div>
225+
</div>
226+
</ion-content>
227+
</div>
228+
</ion-app>
229+
230+
<script>
231+
const nestedModal = document.querySelector('#nested-modal');
232+
nestedModal.presentingElement = document.querySelector('.ion-page');
233+
234+
document.getElementById('open-nested-modal').addEventListener('click', () => {
235+
nestedModal.isOpen = true;
236+
});
237+
238+
document.getElementById('remove-top-level').addEventListener('click', () => {
239+
document.querySelector('#top-level-container').remove();
240+
});
241+
</script>
242+
`,
243+
config
244+
);
245+
246+
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
247+
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');
248+
249+
const nestedModal = page.locator('#nested-modal');
250+
const topLevelContainer = page.locator('#top-level-container');
251+
252+
// Open the nested modal
253+
await page.click('#open-nested-modal');
254+
await ionModalDidPresent.next();
255+
await expect(nestedModal).toBeVisible();
256+
257+
// Remove the top-level container
258+
await page.click('#remove-top-level');
259+
260+
// Wait for modal to be dismissed
261+
const dismissEvent = await ionModalDidDismiss.next();
262+
263+
// Verify the modal was dismissed with the correct role
264+
expect(dismissEvent.detail.role).toBe('parent-removed');
265+
266+
// Verify the modal is no longer visible
267+
await expect(nestedModal).toBeHidden();
268+
269+
// Verify the container was actually removed
270+
await expect(topLevelContainer).not.toBeAttached();
271+
});
125272
});
126273
});

0 commit comments

Comments
 (0)