Skip to content

Commit 92ce563

Browse files
fix(overlays): do not overwrite id set in htmlAttributes (#29722)
Issue number: resolves #29712 --------- <!-- 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? In every type of overlay, the auto incremented overlay id is overwriting any id set in htmlAttributes. ## What is the new behavior? The id in htmlAttributes now takes precedence. ## 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. --> --------- Co-authored-by: Sean Perkins <[email protected]>
1 parent 05913c3 commit 92ce563

21 files changed

+401
-305
lines changed

core/src/components/action-sheet/action-sheet.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,9 @@ export class ActionSheet implements ComponentInterface, OverlayInterface {
310310
}
311311

312312
componentWillLoad() {
313-
setOverlayId(this.el);
313+
if (!this.htmlAttributes?.id) {
314+
setOverlayId(this.el);
315+
}
314316
}
315317

316318
componentDidLoad() {

core/src/components/action-sheet/test/action-sheet-id.spec.ts

Lines changed: 0 additions & 41 deletions
This file was deleted.
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { newSpecPage } from '@stencil/core/testing';
2+
3+
import { ActionSheet } from '../action-sheet';
4+
import { h } from '@stencil/core';
5+
6+
describe('action-sheet: id', () => {
7+
it('action sheet should be assigned an incrementing id', async () => {
8+
const page = await newSpecPage({
9+
components: [ActionSheet],
10+
html: `<ion-action-sheet is-open="true"></ion-action-sheet>`,
11+
});
12+
let actionSheet: HTMLIonActionSheetElement;
13+
14+
actionSheet = page.body.querySelector('ion-action-sheet')!;
15+
16+
expect(actionSheet).not.toBe(null);
17+
expect(actionSheet.getAttribute('id')).toBe('ion-overlay-1');
18+
19+
// Remove the action sheet from the DOM
20+
actionSheet.remove();
21+
await page.waitForChanges();
22+
23+
// Create a new action sheet to verify the id is incremented
24+
actionSheet = document.createElement('ion-action-sheet');
25+
actionSheet.isOpen = true;
26+
page.body.appendChild(actionSheet);
27+
await page.waitForChanges();
28+
29+
actionSheet = page.body.querySelector('ion-action-sheet')!;
30+
31+
expect(actionSheet.getAttribute('id')).toBe('ion-overlay-2');
32+
33+
// Presenting the same action sheet again should reuse the existing id
34+
35+
actionSheet.isOpen = false;
36+
await page.waitForChanges();
37+
actionSheet.isOpen = true;
38+
await page.waitForChanges();
39+
40+
actionSheet = page.body.querySelector('ion-action-sheet')!;
41+
42+
expect(actionSheet.getAttribute('id')).toBe('ion-overlay-2');
43+
});
44+
45+
it('should not overwrite the id set in htmlAttributes', async () => {
46+
const id = 'custom-id';
47+
const page = await newSpecPage({
48+
components: [ActionSheet],
49+
template: () => <ion-action-sheet htmlAttributes={{ id }} overlayIndex={-1}></ion-action-sheet>,
50+
});
51+
52+
const alert = page.body.querySelector('ion-action-sheet')!;
53+
expect(alert.id).toBe(id);
54+
});
55+
});

core/src/components/alert/test/alert-id.spec.ts

Lines changed: 0 additions & 41 deletions
This file was deleted.
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { newSpecPage } from '@stencil/core/testing';
2+
3+
import { Alert } from '../alert';
4+
import { h } from '@stencil/core';
5+
6+
describe('alert: id', () => {
7+
it('alert should be assigned an incrementing id', async () => {
8+
const page = await newSpecPage({
9+
components: [Alert],
10+
html: `<ion-alert is-open="true"></ion-alert>`,
11+
});
12+
let alert: HTMLIonAlertElement;
13+
14+
alert = page.body.querySelector('ion-alert')!;
15+
16+
expect(alert).not.toBe(null);
17+
expect(alert.getAttribute('id')).toBe('ion-overlay-1');
18+
19+
// Remove the alert from the DOM
20+
alert.remove();
21+
await page.waitForChanges();
22+
23+
// Create a new alert to verify the id is incremented
24+
alert = document.createElement('ion-alert');
25+
alert.isOpen = true;
26+
page.body.appendChild(alert);
27+
await page.waitForChanges();
28+
29+
alert = page.body.querySelector('ion-alert')!;
30+
31+
expect(alert.getAttribute('id')).toBe('ion-overlay-2');
32+
33+
// Presenting the same alert again should reuse the existing id
34+
35+
alert.isOpen = false;
36+
await page.waitForChanges();
37+
alert.isOpen = true;
38+
await page.waitForChanges();
39+
40+
alert = page.body.querySelector('ion-alert')!;
41+
42+
expect(alert.getAttribute('id')).toBe('ion-overlay-2');
43+
});
44+
45+
it('should not overwrite the id set in htmlAttributes', async () => {
46+
const id = 'custom-id';
47+
const page = await newSpecPage({
48+
components: [Alert],
49+
template: () => <ion-alert htmlAttributes={{ id }} overlayIndex={-1}></ion-alert>,
50+
});
51+
52+
const alert = page.body.querySelector('ion-alert')!;
53+
expect(alert.id).toBe(id);
54+
});
55+
});

core/src/components/alert/test/alert.spec.tsx renamed to core/src/components/alert/test/alert.spec.ts

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import { newSpecPage } from '@stencil/core/testing';
22

33
import { config } from '../../../global/config';
44
import { Alert } from '../alert';
5-
import { h } from '@stencil/core';
65

76
describe('alert: custom html', () => {
87
it('should not allow for custom html by default', async () => {
@@ -39,15 +38,4 @@ describe('alert: custom html', () => {
3938
expect(content.textContent).toContain('Custom Text');
4039
expect(content.querySelector('button.custom-html')).toBe(null);
4140
});
42-
43-
it('should not overwrite the id set in htmlAttributes', async () => {
44-
const id = 'custom-id';
45-
const page = await newSpecPage({
46-
components: [Alert],
47-
template: () => <ion-alert htmlAttributes={{ id }} overlayIndex={-1}></ion-alert>,
48-
});
49-
50-
const alert = page.body.querySelector('ion-alert')!;
51-
expect(alert.id).toBe(id);
52-
});
5341
});

core/src/components/loading/loading.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,9 @@ export class Loading implements ComponentInterface, OverlayInterface {
214214
const mode = getIonMode(this);
215215
this.spinner = config.get('loadingSpinner', config.get('spinner', mode === 'ios' ? 'lines' : 'crescent'));
216216
}
217-
setOverlayId(this.el);
217+
if (!this.htmlAttributes?.id) {
218+
setOverlayId(this.el);
219+
}
218220
}
219221

220222
componentDidLoad() {

core/src/components/loading/test/loading-id.spec.ts

Lines changed: 0 additions & 41 deletions
This file was deleted.
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { newSpecPage } from '@stencil/core/testing';
2+
3+
import { Loading } from '../loading';
4+
import { h } from '@stencil/core';
5+
6+
describe('loading: id', () => {
7+
it('loading should be assigned an incrementing id', async () => {
8+
const page = await newSpecPage({
9+
components: [Loading],
10+
html: `<ion-loading is-open="true"></ion-loading>`,
11+
});
12+
let loading: HTMLIonLoadingElement;
13+
14+
loading = page.body.querySelector('ion-loading')!;
15+
16+
expect(loading).not.toBe(null);
17+
expect(loading.getAttribute('id')).toBe('ion-overlay-1');
18+
19+
// Remove the loading from the DOM
20+
loading.remove();
21+
await page.waitForChanges();
22+
23+
// Create a new loading to verify the id is incremented
24+
loading = document.createElement('ion-loading');
25+
loading.isOpen = true;
26+
page.body.appendChild(loading);
27+
await page.waitForChanges();
28+
29+
loading = page.body.querySelector('ion-loading')!;
30+
31+
expect(loading.getAttribute('id')).toBe('ion-overlay-2');
32+
33+
// Presenting the same loading again should reuse the existing id
34+
35+
loading.isOpen = false;
36+
await page.waitForChanges();
37+
loading.isOpen = true;
38+
await page.waitForChanges();
39+
40+
loading = page.body.querySelector('ion-loading')!;
41+
42+
expect(loading.getAttribute('id')).toBe('ion-overlay-2');
43+
});
44+
45+
it('should not overwrite the id set in htmlAttributes', async () => {
46+
const id = 'custom-id';
47+
const page = await newSpecPage({
48+
components: [Loading],
49+
template: () => <ion-loading htmlAttributes={{ id }} overlayIndex={-1}></ion-loading>,
50+
});
51+
52+
const alert = page.body.querySelector('ion-loading')!;
53+
expect(alert.id).toBe(id);
54+
});
55+
});

core/src/components/modal/modal.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,9 @@ export class Modal implements ComponentInterface, OverlayInterface {
415415
printIonWarning('Your breakpoints array must include the initialBreakpoint value.');
416416
}
417417

418-
setOverlayId(el);
418+
if (!this.htmlAttributes?.id) {
419+
setOverlayId(this.el);
420+
}
419421
}
420422

421423
componentDidLoad() {

core/src/components/modal/test/modal-id.spec.ts

Lines changed: 0 additions & 41 deletions
This file was deleted.

0 commit comments

Comments
 (0)