From e96b6a8b7135490e7893df324400544eb898ccc6 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 19 Oct 2022 15:26:36 -0400 Subject: [PATCH 1/6] fix(overlays): presenting an overlay does not create nested elements --- .../test/overlays/datetime-button.e2e.ts | 20 +++++++++++++++++++ core/src/utils/framework-delegate.ts | 4 +++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts b/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts index 7fdab33f003..b00f3b2768f 100644 --- a/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts +++ b/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts @@ -159,4 +159,24 @@ test.describe('datetime-button: modal', () => { await ionModalDidPresent.next(); expect(datetime).toBeVisible(); }); + test('datetime should be the first child of the modal', async ({ page }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/26117', + }); + + await page.locator('#date-button').click(); + await ionModalDidPresent.next(); + expect(datetime).toBeVisible(); + + expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.tagName)).toBe('ION-DATETIME'); + await modal.evaluate((el: HTMLIonModalElement) => el.dismiss()); + await ionModalDidDismiss.next(); + + await page.locator('#date-button').click(); + await ionModalDidPresent.next(); + expect(datetime).toBeVisible(); + + expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.tagName)).toBe('ION-DATETIME'); + }); }); diff --git a/core/src/utils/framework-delegate.ts b/core/src/utils/framework-delegate.ts index d3b6ff382c7..2667708df64 100644 --- a/core/src/utils/framework-delegate.ts +++ b/core/src/utils/framework-delegate.ts @@ -53,6 +53,8 @@ export const CoreDelegate = () => { userComponentProps: any = {}, cssClasses: string[] = [] ) => { + const hasUserDefinedComponent = userComponent !== undefined; + BaseComponent = parentElement; /** * If passing in a component via the `component` props @@ -86,7 +88,7 @@ export const CoreDelegate = () => { BaseComponent.appendChild(el); await new Promise((resolve) => componentOnReady(el, resolve)); - } else if (BaseComponent.children.length > 0) { + } else if (hasUserDefinedComponent && BaseComponent.children.length > 0) { // If there is no component, then we need to create a new parent // element to apply the css classes to. const el = BaseComponent.ownerDocument?.createElement('div'); From 803af45bb175324dff8aa2c26f35d295021bf813 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 19 Oct 2022 15:53:26 -0400 Subject: [PATCH 2/6] fix(datetime): presentation is reflected --- core/src/components/datetime/datetime.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/datetime/datetime.tsx b/core/src/components/datetime/datetime.tsx index e7410c90861..2e82335e9e7 100644 --- a/core/src/components/datetime/datetime.tsx +++ b/core/src/components/datetime/datetime.tsx @@ -203,7 +203,7 @@ export class Datetime implements ComponentInterface { * AM/PM. `'date-time'` will show the date picker first and time picker second. * `'time-date'` will show the time picker first and date picker second. */ - @Prop() presentation: DatetimePresentation = 'date-time'; + @Prop({ reflect: true }) presentation: DatetimePresentation = 'date-time'; /** * The text to display on the picker's cancel button. From 65d5c3134bcad16ae2512221fb69eb9e74ed6ec8 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 19 Oct 2022 15:55:33 -0400 Subject: [PATCH 3/6] test(): wait for changes before reading prop --- .../datetime-button/test/basic/datetime-button.e2e.ts | 2 ++ core/src/components/datetime/datetime.tsx | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/components/datetime-button/test/basic/datetime-button.e2e.ts b/core/src/components/datetime-button/test/basic/datetime-button.e2e.ts index c236fc8d2b0..cca426a6fab 100644 --- a/core/src/components/datetime-button/test/basic/datetime-button.e2e.ts +++ b/core/src/components/datetime-button/test/basic/datetime-button.e2e.ts @@ -17,6 +17,7 @@ test.describe('datetime-button: switching to correct view', () => { expect(datetime).toHaveJSProperty('presentation', 'date-time'); await page.locator('#date-button').click(); + await page.waitForChanges(); expect(datetime).toHaveJSProperty('presentation', 'date'); }); @@ -25,6 +26,7 @@ test.describe('datetime-button: switching to correct view', () => { expect(datetime).toHaveJSProperty('presentation', 'date-time'); await page.locator('#time-button').click(); + await page.waitForChanges(); expect(datetime).toHaveJSProperty('presentation', 'time'); }); diff --git a/core/src/components/datetime/datetime.tsx b/core/src/components/datetime/datetime.tsx index 2e82335e9e7..e7410c90861 100644 --- a/core/src/components/datetime/datetime.tsx +++ b/core/src/components/datetime/datetime.tsx @@ -203,7 +203,7 @@ export class Datetime implements ComponentInterface { * AM/PM. `'date-time'` will show the date picker first and time picker second. * `'time-date'` will show the time picker first and date picker second. */ - @Prop({ reflect: true }) presentation: DatetimePresentation = 'date-time'; + @Prop() presentation: DatetimePresentation = 'date-time'; /** * The text to display on the picker's cancel button. From 054a45c1d2bf09b25e67389c9631ff2cedb7dfaf Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Thu, 27 Oct 2022 12:27:49 -0400 Subject: [PATCH 4/6] fix: only construct delegate host element on first present --- core/src/utils/framework-delegate.ts | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/core/src/utils/framework-delegate.ts b/core/src/utils/framework-delegate.ts index 2667708df64..24746977c2a 100644 --- a/core/src/utils/framework-delegate.ts +++ b/core/src/utils/framework-delegate.ts @@ -53,8 +53,6 @@ export const CoreDelegate = () => { userComponentProps: any = {}, cssClasses: string[] = [] ) => { - const hasUserDefinedComponent = userComponent !== undefined; - BaseComponent = parentElement; /** * If passing in a component via the `component` props @@ -88,15 +86,23 @@ export const CoreDelegate = () => { BaseComponent.appendChild(el); await new Promise((resolve) => componentOnReady(el, resolve)); - } else if (hasUserDefinedComponent && BaseComponent.children.length > 0) { - // If there is no component, then we need to create a new parent - // element to apply the css classes to. - const el = BaseComponent.ownerDocument?.createElement('div'); - cssClasses.forEach((c) => el.classList.add(c)); - // Move each child from the original template to the new parent element. - el.append(...BaseComponent.children); - // Append the new parent element to the original parent element. - BaseComponent.appendChild(el); + } else if (BaseComponent.children.length > 0) { + const root = BaseComponent.children[0] as HTMLElement & { __delegateHost?: boolean }; + if (root.__delegateHost !== true) { + /** + * If the root element is not a delegate host, it means + * that the overlay has not been presented yet and we need + * to create the containing element with the specified classes. + */ + const el = BaseComponent.ownerDocument?.createElement('div'); + // Internal flag to track if the element is a delegate host + el.__delegateHost = true; + cssClasses.forEach((c) => el.classList.add(c)); + // Move each child from the original template to the new parent element. + el.append(...BaseComponent.children); + // Append the new parent element to the original parent element. + BaseComponent.appendChild(el); + } } /** From 03c16bca0421ad6bc899968c6b19cd82d2bfe485 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Thu, 27 Oct 2022 12:39:20 -0400 Subject: [PATCH 5/6] test(modal): presenting only creates a single root element with ion-page --- .../test/overlays/datetime-button.e2e.ts | 20 ---------- .../components/modal/test/inline/modal.e2e.ts | 40 +++++++++++++++++++ 2 files changed, 40 insertions(+), 20 deletions(-) diff --git a/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts b/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts index b00f3b2768f..7fdab33f003 100644 --- a/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts +++ b/core/src/components/datetime-button/test/overlays/datetime-button.e2e.ts @@ -159,24 +159,4 @@ test.describe('datetime-button: modal', () => { await ionModalDidPresent.next(); expect(datetime).toBeVisible(); }); - test('datetime should be the first child of the modal', async ({ page }, testInfo) => { - testInfo.annotations.push({ - type: 'issue', - description: 'https://github.com/ionic-team/ionic-framework/issues/26117', - }); - - await page.locator('#date-button').click(); - await ionModalDidPresent.next(); - expect(datetime).toBeVisible(); - - expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.tagName)).toBe('ION-DATETIME'); - await modal.evaluate((el: HTMLIonModalElement) => el.dismiss()); - await ionModalDidDismiss.next(); - - await page.locator('#date-button').click(); - await ionModalDidPresent.next(); - expect(datetime).toBeVisible(); - - expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.tagName)).toBe('ION-DATETIME'); - }); }); diff --git a/core/src/components/modal/test/inline/modal.e2e.ts b/core/src/components/modal/test/inline/modal.e2e.ts index c215da0a4be..e5449b2d7a6 100644 --- a/core/src/components/modal/test/inline/modal.e2e.ts +++ b/core/src/components/modal/test/inline/modal.e2e.ts @@ -21,4 +21,44 @@ test.describe('modal: inline', () => { expect(await page.screenshot()).toMatchSnapshot(`modal-inline-dismiss-${page.getSnapshotSettings()}.png`); }); + + test('presenting should create a single root element with the ion-page class', async ({ page, skip }, testInfo) => { + skip.mode('md'); + skip.rtl(); + + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/26117', + }); + + await page.setContent(` + + + + + + `); + + const ionModalDidPresent = await page.spyOnEvent('ionModalDidPresent'); + const ionModalDidDismiss = await page.spyOnEvent('ionModalDidDismiss'); + const modal = page.locator('ion-modal'); + + await page.locator('#date-button').click(); + await ionModalDidPresent.next(); + + // Verifies that the host element exists with the .ion-page class + expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.className)).toBe('ion-page'); + + await modal.evaluate((el: HTMLIonModalElement) => el.dismiss()); + await ionModalDidDismiss.next(); + + await page.locator('#date-button').click(); + await ionModalDidPresent.next(); + + // Verifies that presenting the overlay again does not create a new host element + expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.className)).toBe('ion-page'); + expect( + await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.firstElementChild!.className) + ).not.toBe('ion-page'); + }); }); From 5daa0ddd1542bfe3f72a33faa1eab2bbfd83774e Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 2 Nov 2022 16:31:22 -0400 Subject: [PATCH 6/6] refactor: internal prop vs. class name --- core/src/components/modal/test/inline/modal.e2e.ts | 6 +++--- core/src/utils/framework-delegate.ts | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/core/src/components/modal/test/inline/modal.e2e.ts b/core/src/components/modal/test/inline/modal.e2e.ts index e5449b2d7a6..75f0546f0f9 100644 --- a/core/src/components/modal/test/inline/modal.e2e.ts +++ b/core/src/components/modal/test/inline/modal.e2e.ts @@ -47,7 +47,7 @@ test.describe('modal: inline', () => { await ionModalDidPresent.next(); // Verifies that the host element exists with the .ion-page class - expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.className)).toBe('ion-page'); + expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.className)).toContain('ion-page'); await modal.evaluate((el: HTMLIonModalElement) => el.dismiss()); await ionModalDidDismiss.next(); @@ -56,9 +56,9 @@ test.describe('modal: inline', () => { await ionModalDidPresent.next(); // Verifies that presenting the overlay again does not create a new host element - expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.className)).toBe('ion-page'); + expect(await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.className)).toContain('ion-page'); expect( await modal.evaluate((el: HTMLIonModalElement) => el.firstElementChild!.firstElementChild!.className) - ).not.toBe('ion-page'); + ).not.toContain('ion-page'); }); }); diff --git a/core/src/utils/framework-delegate.ts b/core/src/utils/framework-delegate.ts index 24746977c2a..af4f24a2be7 100644 --- a/core/src/utils/framework-delegate.ts +++ b/core/src/utils/framework-delegate.ts @@ -87,16 +87,16 @@ export const CoreDelegate = () => { await new Promise((resolve) => componentOnReady(el, resolve)); } else if (BaseComponent.children.length > 0) { - const root = BaseComponent.children[0] as HTMLElement & { __delegateHost?: boolean }; - if (root.__delegateHost !== true) { + const root = BaseComponent.children[0] as HTMLElement; + if (!root.classList.contains('ion-delegate-host')) { /** * If the root element is not a delegate host, it means * that the overlay has not been presented yet and we need * to create the containing element with the specified classes. */ const el = BaseComponent.ownerDocument?.createElement('div'); - // Internal flag to track if the element is a delegate host - el.__delegateHost = true; + // Add a class to track if the root element was created by the delegate. + el.classList.add('ion-delegate-host'); cssClasses.forEach((c) => el.classList.add(c)); // Move each child from the original template to the new parent element. el.append(...BaseComponent.children);