Skip to content

Commit 9b0099f

Browse files
ShaneKShaneK
authored
fix(modal): dismiss child modals when parent is dismissed (#30540)
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 a child modal is present and a parent modal is somehow dismissed, the child modal stays open. This can cause issues in some frameworks like React and Angular, where this cuts the connection to the child modal and it can no longer be dismissed programmatically. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> This change enables modals to identify their children and close the children when they're closed. This prevents orphaned modals that can cause a poor UX. Note: This fix is only for inline modals, which is the biggest cause of the above issue. ## 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. --> [Relevant test page](https://ionic-framework-git-fw-6597-ionic1.vercel.app/src/components/modal/test/inline) **Current dev build**: ``` 8.6.5-dev.11752242329.17d249a3 ```
1 parent 8bfd6d9 commit 9b0099f

File tree

3 files changed

+153
-10
lines changed

3 files changed

+153
-10
lines changed

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

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -784,6 +784,13 @@ export class Modal implements ComponentInterface, OverlayInterface {
784784
*/
785785
const unlock = await this.lockController.lock();
786786

787+
/**
788+
* Dismiss all child modals. This is especially important in
789+
* Angular and React because it's possible to lose control of a child
790+
* modal when the parent modal is dismissed.
791+
*/
792+
await this.dismissNestedModals();
793+
787794
/**
788795
* If a canDismiss handler is responsible
789796
* for calling the dismiss method, we should
@@ -1115,6 +1122,34 @@ export class Modal implements ComponentInterface, OverlayInterface {
11151122
}
11161123
}
11171124

1125+
/**
1126+
* When the slot changes, we need to find all the modals in the slot
1127+
* and set the data-parent-ion-modal attribute on them so we can find them
1128+
* and dismiss them when we get dismissed.
1129+
* We need to do it this way because when a modal is opened, it's moved to
1130+
* the end of the body and is no longer an actual child of the modal.
1131+
*/
1132+
private onSlotChange = ({ target }: Event) => {
1133+
const slot = target as HTMLSlotElement;
1134+
slot.assignedElements().forEach((el) => {
1135+
el.querySelectorAll('ion-modal').forEach((childModal) => {
1136+
// We don't need to write to the DOM if the modal is already tagged
1137+
// If this is a deeply nested modal, this effect should cascade so we don't
1138+
// need to worry about another modal claiming the same child.
1139+
if (childModal.getAttribute('data-parent-ion-modal') === null) {
1140+
childModal.setAttribute('data-parent-ion-modal', this.el.id);
1141+
}
1142+
});
1143+
});
1144+
};
1145+
1146+
private async dismissNestedModals(): Promise<void> {
1147+
const nestedModals = document.querySelectorAll(`ion-modal[data-parent-ion-modal="${this.el.id}"]`);
1148+
nestedModals?.forEach(async (modal) => {
1149+
await (modal as HTMLIonModalElement).dismiss(undefined, 'parent-dismissed');
1150+
});
1151+
}
1152+
11181153
render() {
11191154
const {
11201155
handle,
@@ -1192,7 +1227,7 @@ export class Modal implements ComponentInterface, OverlayInterface {
11921227
ref={(el) => (this.dragHandleEl = el)}
11931228
></button>
11941229
)}
1195-
<slot></slot>
1230+
<slot onSlotchange={this.onSlotChange}></slot>
11961231
</div>
11971232
</Host>
11981233
);

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

Lines changed: 55 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,29 +24,76 @@
2424
<ion-content class="ion-padding">
2525
<button id="open-inline-modal" onclick="openModal(event)">Open Modal</button>
2626

27-
<ion-modal swipe-to-close="true">
28-
<ion-header>
29-
<ion-toolbar>
30-
<ion-title> Modal </ion-title>
31-
</ion-toolbar>
32-
</ion-header>
33-
<ion-content class="ion-padding"> This is my inline modal content! </ion-content>
34-
</ion-modal>
27+
<div id="modal-container">
28+
<ion-modal swipe-to-close="true">
29+
<ion-header>
30+
<ion-toolbar>
31+
<ion-title> Modal </ion-title>
32+
</ion-toolbar>
33+
</ion-header>
34+
<ion-content class="ion-padding">
35+
<p>This is my inline modal content!</p>
36+
<button id="open-child-modal" onclick="openChildModal(event)">Open Child Modal</button>
37+
38+
<ion-modal id="child-modal" swipe-to-close="true">
39+
<ion-header>
40+
<ion-toolbar>
41+
<ion-title>Child Modal</ion-title>
42+
</ion-toolbar>
43+
</ion-header>
44+
<ion-content class="ion-padding">
45+
<p>This is the child modal content!</p>
46+
<p>When the parent modal is dismissed, this child modal should also be dismissed automatically.</p>
47+
<button id="dismiss-parent" onclick="dismissParent(event)">Dismiss Parent Modal</button>
48+
<button id="dismiss-child" onclick="dismissChild(event)">Dismiss Child Modal</button>
49+
</ion-content>
50+
</ion-modal>
51+
</ion-content>
52+
</ion-modal>
53+
</div>
3554
</ion-content>
3655
</div>
3756
</ion-app>
3857

3958
<script>
4059
const modal = document.querySelector('ion-modal');
60+
const childModal = document.querySelector('#child-modal');
61+
4162
modal.presentingElement = document.querySelector('.ion-page');
63+
childModal.presentingElement = modal;
4264

4365
const openModal = () => {
4466
modal.isOpen = true;
4567
};
4668

69+
const openChildModal = () => {
70+
childModal.isOpen = true;
71+
};
72+
73+
const dismissParent = () => {
74+
modal.isOpen = false;
75+
};
76+
77+
const dismissChild = () => {
78+
childModal.isOpen = false;
79+
};
80+
4781
modal.addEventListener('didDismiss', () => {
4882
modal.isOpen = false;
4983
});
84+
85+
childModal.addEventListener('didDismiss', () => {
86+
childModal.isOpen = false;
87+
});
88+
89+
// Add event listeners to demonstrate the new functionality
90+
modal.addEventListener('ionModalDidDismiss', (event) => {
91+
console.log('Parent modal dismissed with role:', event.detail.role);
92+
});
93+
94+
childModal.addEventListener('ionModalDidDismiss', (event) => {
95+
console.log('Child modal dismissed with role:', event.detail.role);
96+
});
5097
</script>
5198
</body>
5299
</html>

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

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
77
await page.goto('/src/components/modal/test/inline', config);
88
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
99
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');
10-
const modal = page.locator('ion-modal');
10+
const modal = page.locator('ion-modal').first();
1111

1212
await page.click('#open-inline-modal');
1313

@@ -22,6 +22,67 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
2222
await expect(modal).toBeHidden();
2323
});
2424

25+
test('it should dismiss child modals when parent modal is dismissed', async ({ page }) => {
26+
await page.goto('/src/components/modal/test/inline', config);
27+
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
28+
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');
29+
30+
const parentModal = page.locator('ion-modal').first();
31+
const childModal = page.locator('#child-modal');
32+
33+
// Open the parent modal
34+
await page.click('#open-inline-modal');
35+
await ionModalDidPresent.next();
36+
await expect(parentModal).toBeVisible();
37+
38+
// Open the child modal
39+
await page.click('#open-child-modal');
40+
await ionModalDidPresent.next();
41+
await expect(childModal).toBeVisible();
42+
43+
// Both modals should be visible
44+
await expect(parentModal).toBeVisible();
45+
await expect(childModal).toBeVisible();
46+
47+
// Dismiss the parent modal
48+
await page.click('#dismiss-parent');
49+
50+
// Wait for both modals to be dismissed
51+
await ionModalDidDismiss.next(); // child modal dismissed
52+
await ionModalDidDismiss.next(); // parent modal dismissed
53+
54+
// Both modals should be hidden
55+
await expect(parentModal).toBeHidden();
56+
await expect(childModal).toBeHidden();
57+
});
58+
59+
test('it should only dismiss child modal when child dismiss button is clicked', async ({ page }) => {
60+
await page.goto('/src/components/modal/test/inline', config);
61+
const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent');
62+
const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss');
63+
64+
const parentModal = page.locator('ion-modal').first();
65+
const childModal = page.locator('#child-modal');
66+
67+
// Open the parent modal
68+
await page.click('#open-inline-modal');
69+
await ionModalDidPresent.next();
70+
await expect(parentModal).toBeVisible();
71+
72+
// Open the child modal
73+
await page.click('#open-child-modal');
74+
await ionModalDidPresent.next();
75+
await expect(childModal).toBeVisible();
76+
77+
// Dismiss only the child modal
78+
await page.click('#dismiss-child');
79+
await ionModalDidDismiss.next();
80+
81+
// Parent modal should still be visible, child modal should be hidden
82+
await expect(parentModal).toBeVisible();
83+
await expect(childModal).toBeHidden();
84+
});
85+
2586
test('presenting should create a single root element with the ion-page class', async ({ page }, testInfo) => {
2687
testInfo.annotations.push({
2788
type: 'issue',

0 commit comments

Comments
 (0)