From 0439c86775b373d67674b3d2f333ee46fc69126c Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 22 Jan 2025 12:40:52 -0800 Subject: [PATCH 01/26] feat(dialog): add focusTrapDisabled attribute to for non-modal dialogs --- .../src/components/dialog/dialog.e2e.ts | 33 +++++++++++++++++++ .../src/components/dialog/dialog.tsx | 24 ++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index d9acdb326c8..2b071eb3b47 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1160,4 +1160,37 @@ describe("calcite-dialog", () => { expect(await dialog.getProperty("open")).toBe(true); }); }); + + describe("focusTrap", () => { + it("closes when Escape key is pressed and focusTrapDisabled=true", async () => { + const page = await newE2EPage(); + await page.setContent(html` `); + await skipAnimations(page); + await page.waitForChanges(); + + const dialog = await page.find("calcite-dialog"); + const button = await page.find("calcite-dialog >>> calcite-action"); + + expect(await dialog.isVisible()).toBe(true); + + await page.keyboard.press("Tab"); + await page.waitForChanges(); + + await page.keyboard.press("Escape"); + await page.waitForChanges(); + + expect(await dialog.isVisible()).toBe(true); + + dialog.setAttribute("modal", "true"); + button.callMethod("setFocus"); + + await page.keyboard.press("Tab"); + await page.waitForChanges(); + + await page.keyboard.press("Escape"); + await page.waitForChanges(); + + expect(await dialog.isVisible()).toBe(false); + }); + }); }); diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index 1b1f65df7b2..130809b7001 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -187,6 +187,9 @@ export class Dialog /** When `true`, displays a scrim blocking interaction underneath the component. */ @property({ reflect: true }) modal = false; + /** When `true`, prevents focus trapping. */ + @property({ reflect: true }) focusTrapDisabled = false; + /** When `true`, displays and positions the component. */ @property({ reflect: true }) get open(): boolean { @@ -329,6 +332,14 @@ export class Dialog this.updateOverflowHiddenClass(); } + if ( + changes.has("focusTrapDisabled") && + !this.modal && + (this.hasUpdated || this.focusTrapDisabled !== false) + ) { + this.handleFocusTrapDisabled(this.focusTrapDisabled); + } + if ( (changes.has("open") && (this.hasUpdated || this.open !== false)) || (changes.has("placement") && (this.hasUpdated || this.placement !== "center")) || @@ -366,6 +377,19 @@ export class Dialog // #endregion // #region Private Methods + + private handleFocusTrapDisabled(focusTrapDisabled: boolean): void { + if (!this.open) { + return; + } + + if (focusTrapDisabled && !this.modal) { + deactivateFocusTrap(this); + } else { + activateFocusTrap(this); + } + } + private updateAssistiveText(): void { const { messages } = this; this.assistiveText = From 702b4c86e58a785d96d6a6aa83e758ea023ed33e Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 22 Jan 2025 14:02:09 -0800 Subject: [PATCH 02/26] rework logic --- .../src/components/dialog/dialog.tsx | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index 130809b7001..a1e87e2225e 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -332,14 +332,6 @@ export class Dialog this.updateOverflowHiddenClass(); } - if ( - changes.has("focusTrapDisabled") && - !this.modal && - (this.hasUpdated || this.focusTrapDisabled !== false) - ) { - this.handleFocusTrapDisabled(this.focusTrapDisabled); - } - if ( (changes.has("open") && (this.hasUpdated || this.open !== false)) || (changes.has("placement") && (this.hasUpdated || this.placement !== "center")) || @@ -383,7 +375,7 @@ export class Dialog return; } - if (focusTrapDisabled && !this.modal) { + if (focusTrapDisabled) { deactivateFocusTrap(this); } else { activateFocusTrap(this); @@ -404,7 +396,9 @@ export class Dialog onOpen(): void { this.calciteDialogOpen.emit(); - activateFocusTrap(this); + if (this.modal || (this.modal === false && this.focusTrapDisabled === false)) { + activateFocusTrap(this); + } } onBeforeClose(): void { From 61cb7b2fdd4575f249fbb949b03a81f3b7fb9c9e Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 22 Jan 2025 15:33:16 -0800 Subject: [PATCH 03/26] WIP --- .../src/components/dialog/dialog.tsx | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index a1e87e2225e..bfefa9c9832 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -331,6 +331,9 @@ export class Dialog if (changes.has("modal") && (this.hasUpdated || this.modal !== false)) { this.updateOverflowHiddenClass(); } + if (changes.has("modal") || this.focusTrapDisabled) { + this.handleFocusTrapDisabled(this.focusTrapDisabled); + } if ( (changes.has("open") && (this.hasUpdated || this.open !== false)) || @@ -375,10 +378,10 @@ export class Dialog return; } - if (focusTrapDisabled) { - deactivateFocusTrap(this); - } else { + if (this.modal || (this.modal === false && focusTrapDisabled === false)) { activateFocusTrap(this); + } else { + deactivateFocusTrap(this); } } @@ -396,9 +399,7 @@ export class Dialog onOpen(): void { this.calciteDialogOpen.emit(); - if (this.modal || (this.modal === false && this.focusTrapDisabled === false)) { - activateFocusTrap(this); - } + this.handleFocusTrapDisabled(this.focusTrapDisabled); } onBeforeClose(): void { From 3fd589b808a8fbc792721340d4c8999410b4b9a2 Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 22 Jan 2025 15:34:54 -0800 Subject: [PATCH 04/26] doc --- packages/calcite-components/src/components/dialog/dialog.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index bfefa9c9832..fe27a90a2c1 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -187,7 +187,7 @@ export class Dialog /** When `true`, displays a scrim blocking interaction underneath the component. */ @property({ reflect: true }) modal = false; - /** When `true`, prevents focus trapping. */ + /** When `true`, prevents focus trapping when modal is `true`. */ @property({ reflect: true }) focusTrapDisabled = false; /** When `true`, displays and positions the component. */ From 5225520899e065018411bccaa5db388a64436b64 Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 22 Jan 2025 16:09:34 -0800 Subject: [PATCH 05/26] handleFocusTrap - deactivate when not open --- .../src/components/dialog/dialog.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index fe27a90a2c1..92ad9a94ee6 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -331,8 +331,8 @@ export class Dialog if (changes.has("modal") && (this.hasUpdated || this.modal !== false)) { this.updateOverflowHiddenClass(); } - if (changes.has("modal") || this.focusTrapDisabled) { - this.handleFocusTrapDisabled(this.focusTrapDisabled); + if (changes.has("modal") || changes.has("focusTrapDisabled")) { + this.handleFocusTrapDisabled(); } if ( @@ -373,12 +373,12 @@ export class Dialog // #region Private Methods - private handleFocusTrapDisabled(focusTrapDisabled: boolean): void { + private handleFocusTrapDisabled(): void { if (!this.open) { - return; + deactivateFocusTrap(this); } - if (this.modal || (this.modal === false && focusTrapDisabled === false)) { + if (this.modal || (!this.modal && !this.focusTrapDisabled)) { activateFocusTrap(this); } else { deactivateFocusTrap(this); @@ -399,7 +399,7 @@ export class Dialog onOpen(): void { this.calciteDialogOpen.emit(); - this.handleFocusTrapDisabled(this.focusTrapDisabled); + this.handleFocusTrapDisabled(); } onBeforeClose(): void { From 99aa605b638334e8ce4f7942af0e33abab644ba4 Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 22 Jan 2025 16:33:15 -0800 Subject: [PATCH 06/26] WIP --- packages/calcite-components/src/components/dialog/dialog.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index 92ad9a94ee6..18f3c6a436e 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -331,7 +331,7 @@ export class Dialog if (changes.has("modal") && (this.hasUpdated || this.modal !== false)) { this.updateOverflowHiddenClass(); } - if (changes.has("modal") || changes.has("focusTrapDisabled")) { + if (changes.has("modal") || (changes.has("focusTrapDisabled") && this.hasUpdated)) { this.handleFocusTrapDisabled(); } @@ -375,7 +375,7 @@ export class Dialog private handleFocusTrapDisabled(): void { if (!this.open) { - deactivateFocusTrap(this); + return; } if (this.modal || (!this.modal && !this.focusTrapDisabled)) { From 7e910f62275a80c29e5bd2391c86d71deb4522be Mon Sep 17 00:00:00 2001 From: eliza Date: Thu, 23 Jan 2025 10:05:39 -0800 Subject: [PATCH 07/26] willUpdate changes --- packages/calcite-components/src/components/dialog/dialog.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index 18f3c6a436e..81aa74e7e3f 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -331,7 +331,7 @@ export class Dialog if (changes.has("modal") && (this.hasUpdated || this.modal !== false)) { this.updateOverflowHiddenClass(); } - if (changes.has("modal") || (changes.has("focusTrapDisabled") && this.hasUpdated)) { + if ((changes.has("modal") || changes.has("focusTrapDisabled")) && this.hasUpdated) { this.handleFocusTrapDisabled(); } From a82272d63176c5c17cb40c5c6ea88ab125b0d88a Mon Sep 17 00:00:00 2001 From: eliza Date: Thu, 23 Jan 2025 14:22:12 -0800 Subject: [PATCH 08/26] cleanup test and remove check from activateFocusTrap --- .../src/components/dialog/dialog.e2e.ts | 15 ++++++++++----- .../src/utils/focusTrapComponent.ts | 4 +--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 2b071eb3b47..70b1c74b027 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1162,15 +1162,13 @@ describe("calcite-dialog", () => { }); describe("focusTrap", () => { - it("closes when Escape key is pressed and focusTrapDisabled=true", async () => { + it("can tab out of non-modal dialog when focusTrapDisabled=true", async () => { const page = await newE2EPage(); await page.setContent(html` `); await skipAnimations(page); await page.waitForChanges(); const dialog = await page.find("calcite-dialog"); - const button = await page.find("calcite-dialog >>> calcite-action"); - expect(await dialog.isVisible()).toBe(true); await page.keyboard.press("Tab"); @@ -1180,9 +1178,16 @@ describe("calcite-dialog", () => { await page.waitForChanges(); expect(await dialog.isVisible()).toBe(true); + }); + + it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { + const page = await newE2EPage(); + await page.setContent(html` `); + await skipAnimations(page); + await page.waitForChanges(); - dialog.setAttribute("modal", "true"); - button.callMethod("setFocus"); + const dialog = await page.find("calcite-dialog"); + expect(await dialog.isVisible()).toBe(true); await page.keyboard.press("Tab"); await page.waitForChanges(); diff --git a/packages/calcite-components/src/utils/focusTrapComponent.ts b/packages/calcite-components/src/utils/focusTrapComponent.ts index 8c5c560526e..be77b8f1d72 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.ts @@ -73,9 +73,7 @@ export function activateFocusTrap( component: FocusTrapComponent, options?: Parameters<_FocusTrap["activate"]>[0], ): void { - if (!component.focusTrapDisabled) { - component.focusTrap?.activate(options); - } + component.focusTrap?.activate(options); } /** From aa64339be2ab74a3fba249f6d04732305971849e Mon Sep 17 00:00:00 2001 From: eliza Date: Thu, 23 Jan 2025 15:25:54 -0800 Subject: [PATCH 09/26] more control in tests --- .../src/components/dialog/dialog.e2e.ts | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 70b1c74b027..a1da9daa9fe 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1164,17 +1164,25 @@ describe("calcite-dialog", () => { describe("focusTrap", () => { it("can tab out of non-modal dialog when focusTrapDisabled=true", async () => { const page = await newE2EPage(); - await page.setContent(html` `); + await page.setContent(html` + + + `); await skipAnimations(page); await page.waitForChanges(); const dialog = await page.find("calcite-dialog"); expect(await dialog.isVisible()).toBe(true); + await page.keyboard.press("Tab"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); + await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Escape"); + await page.waitForTimeout(200); await page.waitForChanges(); expect(await dialog.isVisible()).toBe(true); @@ -1182,19 +1190,28 @@ describe("calcite-dialog", () => { it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { const page = await newE2EPage(); - await page.setContent(html` `); + await page.setContent(html` + + + `); await skipAnimations(page); await page.waitForChanges(); const dialog = await page.find("calcite-dialog"); expect(await dialog.isVisible()).toBe(true); + await page.keyboard.press("Tab"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); + await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Escape"); + await page.waitForTimeout(200); await page.waitForChanges(); + // expect(await dialog.getAttribute("open")).toBe(false); expect(await dialog.isVisible()).toBe(false); }); }); From 66736a43a30f6ff9f6025b014ac635eb1ae27d7f Mon Sep 17 00:00:00 2001 From: eliza Date: Fri, 24 Jan 2025 13:35:39 -0800 Subject: [PATCH 10/26] adjust visibility check --- .../src/components/dialog/dialog.e2e.ts | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index a1da9daa9fe..d5dcdcee9eb 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1166,7 +1166,6 @@ describe("calcite-dialog", () => { const page = await newE2EPage(); await page.setContent(html` - `); await skipAnimations(page); await page.waitForChanges(); @@ -1178,17 +1177,14 @@ describe("calcite-dialog", () => { await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); - await page.keyboard.press("Tab"); - await page.waitForChanges(); await page.keyboard.press("Escape"); - await page.waitForTimeout(200); await page.waitForChanges(); expect(await dialog.isVisible()).toBe(true); }); - it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { + it.only("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { const page = await newE2EPage(); await page.setContent(html` @@ -1197,7 +1193,7 @@ describe("calcite-dialog", () => { await skipAnimations(page); await page.waitForChanges(); - const dialog = await page.find("calcite-dialog"); + const dialog = await page.find("calcite-dialog >>> .container"); expect(await dialog.isVisible()).toBe(true); await page.keyboard.press("Tab"); @@ -1205,13 +1201,10 @@ describe("calcite-dialog", () => { await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); - await page.waitForChanges(); await page.keyboard.press("Escape"); - await page.waitForTimeout(200); await page.waitForChanges(); - // expect(await dialog.getAttribute("open")).toBe(false); expect(await dialog.isVisible()).toBe(false); }); }); From f4a4a998f32c8f29127e90af964a71fea309f055 Mon Sep 17 00:00:00 2001 From: eliza Date: Fri, 24 Jan 2025 14:20:56 -0800 Subject: [PATCH 11/26] cleanup --- .../src/components/dialog/dialog.e2e.ts | 10 +++++++--- .../src/components/dialog/dialog.tsx | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index d5dcdcee9eb..1ebcc018cbb 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1166,13 +1166,18 @@ describe("calcite-dialog", () => { const page = await newE2EPage(); await page.setContent(html` + `); await skipAnimations(page); await page.waitForChanges(); - const dialog = await page.find("calcite-dialog"); + const dialog = await page.find("calcite-dialog >>> .container"); + const action = await page.find("calcite-dialog >>> calcite-action"); expect(await dialog.isVisible()).toBe(true); + await action.callMethod("setFocus"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); @@ -1184,7 +1189,7 @@ describe("calcite-dialog", () => { expect(await dialog.isVisible()).toBe(true); }); - it.only("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { + it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { const page = await newE2EPage(); await page.setContent(html` @@ -1200,7 +1205,6 @@ describe("calcite-dialog", () => { await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); - await page.keyboard.press("Tab"); await page.keyboard.press("Escape"); await page.waitForChanges(); diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index 81aa74e7e3f..717d14bb9d7 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -187,7 +187,7 @@ export class Dialog /** When `true`, displays a scrim blocking interaction underneath the component. */ @property({ reflect: true }) modal = false; - /** When `true`, prevents focus trapping when modal is `true`. */ + /** When `true` and `modal` is `false`, prevents focus trapping. */ @property({ reflect: true }) focusTrapDisabled = false; /** When `true`, displays and positions the component. */ From bdf7a80f0593c6259ca9bf761769c7bafd341d1b Mon Sep 17 00:00:00 2001 From: eliza Date: Fri, 24 Jan 2025 14:50:41 -0800 Subject: [PATCH 12/26] cleanup --- packages/calcite-components/src/components/dialog/dialog.e2e.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 1ebcc018cbb..56391c1dc2d 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1182,6 +1182,8 @@ describe("calcite-dialog", () => { await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); + await page.keyboard.press("Tab"); + await page.waitForChanges(); await page.keyboard.press("Escape"); await page.waitForChanges(); From 278464fbb4891c8dc15f15b04f00c266268b3e4c Mon Sep 17 00:00:00 2001 From: eliza Date: Tue, 28 Jan 2025 18:01:17 -0800 Subject: [PATCH 13/26] assert on the element focus in tests and add _focusTrapDisabledOverride method --- .../src/components/dialog/dialog.e2e.ts | 22 +++++++++++-------- .../src/components/dialog/dialog.tsx | 12 +++++++++- .../src/utils/focusTrapComponent.ts | 11 +++++++++- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 56391c1dc2d..b703a1d5aae 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1166,13 +1166,15 @@ describe("calcite-dialog", () => { const page = await newE2EPage(); await page.setContent(html` - + `); await skipAnimations(page); await page.waitForChanges(); const dialog = await page.find("calcite-dialog >>> .container"); const action = await page.find("calcite-dialog >>> calcite-action"); + const outsideEl = await page.find("#outsideEl"); + expect(await dialog.isVisible()).toBe(true); await action.callMethod("setFocus"); @@ -1185,33 +1187,35 @@ describe("calcite-dialog", () => { await page.keyboard.press("Tab"); await page.waitForChanges(); - await page.keyboard.press("Escape"); - await page.waitForChanges(); - - expect(await dialog.isVisible()).toBe(true); + const activeElementId = await page.evaluate(() => document.activeElement.id); + expect(activeElementId).toBe(await outsideEl.getProperty("id")); }); it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { const page = await newE2EPage(); await page.setContent(html` - + `); await skipAnimations(page); await page.waitForChanges(); const dialog = await page.find("calcite-dialog >>> .container"); + const insideEl = await page.find("#insideEl"); + expect(await dialog.isVisible()).toBe(true); await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); - - await page.keyboard.press("Escape"); + await page.keyboard.press("Tab"); await page.waitForChanges(); - expect(await dialog.isVisible()).toBe(false); + const activeElementId = await page.evaluate(() => document.activeElement.id); + expect(activeElementId).toBe(await insideEl.getProperty("id")); }); }); }); diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index 717d14bb9d7..f2ee247268c 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -273,6 +273,16 @@ export class Dialog updateFocusTrapElements(this); } + /** + * When defined, provides a condition for focusTrapDisabled override. + * + * @private + */ + @method() + _focusTrapDisabledOverride(): boolean { + return this.modal || !this.focusTrapDisabled; + } + // #endregion // #region Events @@ -378,7 +388,7 @@ export class Dialog return; } - if (this.modal || (!this.modal && !this.focusTrapDisabled)) { + if (this._focusTrapDisabledOverride()) { activateFocusTrap(this); } else { deactivateFocusTrap(this); diff --git a/packages/calcite-components/src/utils/focusTrapComponent.ts b/packages/calcite-components/src/utils/focusTrapComponent.ts index be77b8f1d72..6b6f15da92b 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.ts @@ -10,6 +10,13 @@ export interface FocusTrapComponent { /** When `true`, prevents focus trapping. */ focusTrapDisabled?: boolean; + /** + * When defined, provides a condition for focusTrapDisabled override. + * + * @internal + */ + _focusTrapDisabledOverride?: () => boolean; + /** The focus trap instance. */ focusTrap: FocusTrap; @@ -73,7 +80,9 @@ export function activateFocusTrap( component: FocusTrapComponent, options?: Parameters<_FocusTrap["activate"]>[0], ): void { - component.focusTrap?.activate(options); + if (component._focusTrapDisabledOverride() || !component.focusTrapDisabled) { + component.focusTrap?.activate(options); + } } /** From 0e2ab22e1a5981e0cbefafe87d02c20b4adc126c Mon Sep 17 00:00:00 2001 From: eliza Date: Tue, 28 Jan 2025 18:17:09 -0800 Subject: [PATCH 14/26] typeError fix --- packages/calcite-components/src/utils/focusTrapComponent.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/calcite-components/src/utils/focusTrapComponent.ts b/packages/calcite-components/src/utils/focusTrapComponent.ts index 6b6f15da92b..eb01e536294 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.ts @@ -80,7 +80,10 @@ export function activateFocusTrap( component: FocusTrapComponent, options?: Parameters<_FocusTrap["activate"]>[0], ): void { - if (component._focusTrapDisabledOverride() || !component.focusTrapDisabled) { + if ( + (component._focusTrapDisabledOverride && component._focusTrapDisabledOverride()) || + !component.focusTrapDisabled + ) { component.focusTrap?.activate(options); } } From 826d93f792d68d0f39a1d0e283011337ef5db037 Mon Sep 17 00:00:00 2001 From: eliza Date: Thu, 30 Jan 2025 16:31:44 -0800 Subject: [PATCH 15/26] refactor and cleanup --- .../src/components/dialog/dialog.tsx | 15 +++------------ .../src/utils/focusTrapComponent.ts | 13 +++---------- 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index f2ee247268c..bf66f76fe30 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -273,13 +273,8 @@ export class Dialog updateFocusTrapElements(this); } - /** - * When defined, provides a condition for focusTrapDisabled override. - * - * @private - */ - @method() - _focusTrapDisabledOverride(): boolean { + /** When defined, provides a condition for focusTrapDisabled override. */ + focusTrapDisabledOverride(): boolean { return this.modal || !this.focusTrapDisabled; } @@ -388,11 +383,7 @@ export class Dialog return; } - if (this._focusTrapDisabledOverride()) { - activateFocusTrap(this); - } else { - deactivateFocusTrap(this); - } + activateFocusTrap(this); } private updateAssistiveText(): void { diff --git a/packages/calcite-components/src/utils/focusTrapComponent.ts b/packages/calcite-components/src/utils/focusTrapComponent.ts index eb01e536294..4f125af2ad0 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.ts @@ -10,12 +10,8 @@ export interface FocusTrapComponent { /** When `true`, prevents focus trapping. */ focusTrapDisabled?: boolean; - /** - * When defined, provides a condition for focusTrapDisabled override. - * - * @internal - */ - _focusTrapDisabledOverride?: () => boolean; + /** When defined, provides a condition for focusTrapDisabled override. */ + focusTrapDisabledOverride?: () => boolean; /** The focus trap instance. */ focusTrap: FocusTrap; @@ -80,10 +76,7 @@ export function activateFocusTrap( component: FocusTrapComponent, options?: Parameters<_FocusTrap["activate"]>[0], ): void { - if ( - (component._focusTrapDisabledOverride && component._focusTrapDisabledOverride()) || - !component.focusTrapDisabled - ) { + if ((component.focusTrapDisabledOverride && component.focusTrapDisabledOverride()) || !component.focusTrapDisabled) { component.focusTrap?.activate(options); } } From 0c7a4f906645a9a345ebd3156918695dca4bc98b Mon Sep 17 00:00:00 2001 From: eliza Date: Thu, 30 Jan 2025 21:46:56 -0800 Subject: [PATCH 16/26] consistency with focusTrapDisabled and Override --- packages/calcite-components/src/components/dialog/dialog.tsx | 2 +- packages/calcite-components/src/utils/focusTrapComponent.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index bf66f76fe30..2861fbba15c 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -275,7 +275,7 @@ export class Dialog /** When defined, provides a condition for focusTrapDisabled override. */ focusTrapDisabledOverride(): boolean { - return this.modal || !this.focusTrapDisabled; + return !this.modal && this.focusTrapDisabled; } // #endregion diff --git a/packages/calcite-components/src/utils/focusTrapComponent.ts b/packages/calcite-components/src/utils/focusTrapComponent.ts index 4f125af2ad0..ae04b86ed16 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.ts @@ -76,7 +76,7 @@ export function activateFocusTrap( component: FocusTrapComponent, options?: Parameters<_FocusTrap["activate"]>[0], ): void { - if ((component.focusTrapDisabledOverride && component.focusTrapDisabledOverride()) || !component.focusTrapDisabled) { + if ((component.focusTrapDisabledOverride && !component.focusTrapDisabledOverride()) || !component.focusTrapDisabled) { component.focusTrap?.activate(options); } } From 28731838c602faf6bd58b40756dc14947009c0ae Mon Sep 17 00:00:00 2001 From: eliza Date: Fri, 31 Jan 2025 13:37:22 -0800 Subject: [PATCH 17/26] corrected doc and condition, added spec test --- .../src/components/dialog/dialog.e2e.ts | 25 ++++++++----------- .../src/components/dialog/dialog.tsx | 2 +- .../src/utils/focusTrapComponent.spec.ts | 23 +++++++++++++++++ .../src/utils/focusTrapComponent.ts | 4 +-- 4 files changed, 37 insertions(+), 17 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index b703a1d5aae..c96ed09dfbc 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1,6 +1,6 @@ // @ts-strict-ignore import { newE2EPage, E2EPage } from "@arcgis/lumina-compiler/puppeteerTesting"; -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { accessible, defaults, @@ -1162,15 +1162,22 @@ describe("calcite-dialog", () => { }); describe("focusTrap", () => { - it("can tab out of non-modal dialog when focusTrapDisabled=true", async () => { - const page = await newE2EPage(); + let page: E2EPage; + + beforeEach(async () => { + page = await newE2EPage(); await page.setContent(html` - + `); + await skipAnimations(page); await page.waitForChanges(); + }); + it("can tab out of non-modal dialog when focusTrapDisabled=true", async () => { const dialog = await page.find("calcite-dialog >>> .container"); const action = await page.find("calcite-dialog >>> calcite-action"); const outsideEl = await page.find("#outsideEl"); @@ -1192,16 +1199,6 @@ describe("calcite-dialog", () => { }); it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { - const page = await newE2EPage(); - await page.setContent(html` - - - `); - await skipAnimations(page); - await page.waitForChanges(); - const dialog = await page.find("calcite-dialog >>> .container"); const insideEl = await page.find("#insideEl"); diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index 2861fbba15c..40466cd0856 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -273,7 +273,7 @@ export class Dialog updateFocusTrapElements(this); } - /** When defined, provides a condition for focusTrapDisabled override. */ + /** When defined, provides a condition to disable focus trapping. When `true`, prevents focus trapping. */ focusTrapDisabledOverride(): boolean { return !this.modal && this.focusTrapDisabled; } diff --git a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts index 8fd9f02b5b1..5cea72a000a 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts @@ -112,4 +112,27 @@ describe("focusTrapComponent", () => { expect(customFocusTrapStack).toHaveLength(1); }); }); + describe("focusTrapDisabledOverride", () => { + it("should deactivate focus trap when focusTrapDisabledOverride returns true", () => { + const fakeComponent = {} as FocusTrapComponent; + fakeComponent.el = document.createElement("div"); + + connectFocusTrap(fakeComponent); + + const deactivateSpy = vi.fn(); + fakeComponent.focusTrap.deactivate = deactivateSpy; + + const updateSpy = vi.fn(); + fakeComponent.focusTrap.updateContainerElements = updateSpy; + + activateFocusTrap(fakeComponent); + + fakeComponent.focusTrapDisabledOverride = () => true; + + updateFocusTrapElements(fakeComponent); + expect(updateSpy).toHaveBeenCalledTimes(1); + + expect(deactivateSpy).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/calcite-components/src/utils/focusTrapComponent.ts b/packages/calcite-components/src/utils/focusTrapComponent.ts index ae04b86ed16..d41fffadb3c 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.ts @@ -10,7 +10,7 @@ export interface FocusTrapComponent { /** When `true`, prevents focus trapping. */ focusTrapDisabled?: boolean; - /** When defined, provides a condition for focusTrapDisabled override. */ + /** When defined, provides a condition to disable focus trapping. When `true`, prevents focus trapping. */ focusTrapDisabledOverride?: () => boolean; /** The focus trap instance. */ @@ -76,7 +76,7 @@ export function activateFocusTrap( component: FocusTrapComponent, options?: Parameters<_FocusTrap["activate"]>[0], ): void { - if ((component.focusTrapDisabledOverride && !component.focusTrapDisabledOverride()) || !component.focusTrapDisabled) { + if (component.focusTrapDisabledOverride ? !component.focusTrapDisabledOverride() : !component.focusTrapDisabled) { component.focusTrap?.activate(options); } } From 5c11de0e743150ec018a4610653ba5ba8d87399f Mon Sep 17 00:00:00 2001 From: eliza Date: Fri, 31 Jan 2025 15:29:47 -0800 Subject: [PATCH 18/26] focusTrapDisabledOverride spec --- .../src/utils/focusTrapComponent.spec.ts | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts index 5cea72a000a..e7b40f21b64 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts @@ -113,26 +113,36 @@ describe("focusTrapComponent", () => { }); }); describe("focusTrapDisabledOverride", () => { - it("should deactivate focus trap when focusTrapDisabledOverride returns true", () => { + it("should activate focus trap when focusTrapDisabledOverride returns true", () => { const fakeComponent = {} as FocusTrapComponent; fakeComponent.el = document.createElement("div"); connectFocusTrap(fakeComponent); - const deactivateSpy = vi.fn(); - fakeComponent.focusTrap.deactivate = deactivateSpy; + const activateSpy = vi.fn(); + fakeComponent.focusTrap.activate = activateSpy; - const updateSpy = vi.fn(); - fakeComponent.focusTrap.updateContainerElements = updateSpy; + fakeComponent.focusTrapDisabledOverride = () => false; activateFocusTrap(fakeComponent); + expect(activateSpy).toHaveBeenCalledTimes(1); + }); + + it("should deactivate focus trap when focusTrapDisabledOverride returns true", () => { + const fakeComponent = {} as FocusTrapComponent; + fakeComponent.el = document.createElement("div"); + + connectFocusTrap(fakeComponent); + + const activateSpy = vi.fn(); + fakeComponent.focusTrap.activate = activateSpy; + fakeComponent.focusTrapDisabledOverride = () => true; - updateFocusTrapElements(fakeComponent); - expect(updateSpy).toHaveBeenCalledTimes(1); + activateFocusTrap(fakeComponent); - expect(deactivateSpy).toHaveBeenCalledTimes(1); + expect(activateSpy).toHaveBeenCalledTimes(0); }); }); }); From ee12e16cecb7f6cefff32a2b2b5ac1353e5ba98d Mon Sep 17 00:00:00 2001 From: eliza Date: Fri, 31 Jan 2025 15:53:16 -0800 Subject: [PATCH 19/26] cleanup --- .../calcite-components/src/utils/focusTrapComponent.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts index e7b40f21b64..993f81c215d 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts @@ -113,7 +113,7 @@ describe("focusTrapComponent", () => { }); }); describe("focusTrapDisabledOverride", () => { - it("should activate focus trap when focusTrapDisabledOverride returns true", () => { + it("should activate focus trap when focusTrapDisabledOverride returns false", () => { const fakeComponent = {} as FocusTrapComponent; fakeComponent.el = document.createElement("div"); @@ -129,7 +129,7 @@ describe("focusTrapComponent", () => { expect(activateSpy).toHaveBeenCalledTimes(1); }); - it("should deactivate focus trap when focusTrapDisabledOverride returns true", () => { + it("should not activate focus trap when focusTrapDisabledOverride returns true", () => { const fakeComponent = {} as FocusTrapComponent; fakeComponent.el = document.createElement("div"); From 3cbdaa54554e919bc1bbd5091e39a0c2d5047479 Mon Sep 17 00:00:00 2001 From: eliza Date: Mon, 3 Feb 2025 13:11:05 -0800 Subject: [PATCH 20/26] DRY spec test to use beforeEach, add more test coverage --- .../src/components/dialog/dialog.e2e.ts | 38 +++++++++++++++++++ .../src/utils/focusTrapComponent.spec.ts | 18 ++++----- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index c96ed09dfbc..f0dad155dc7 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1198,6 +1198,23 @@ describe("calcite-dialog", () => { expect(activeElementId).toBe(await outsideEl.getProperty("id")); }); + it("cannot tab out of non-modal dialog when focusTrapDisabled=false", async () => { + const dialog = await page.find("calcite-dialog >>> .container"); + const insideEl = await page.find("#insideEl"); + + expect(await dialog.isVisible()).toBe(true); + + await page.keyboard.press("Tab"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); + await page.waitForChanges(); + + const activeElementId = await page.evaluate(() => document.activeElement.id); + expect(activeElementId).toBe(await insideEl.getProperty("id")); + }); + it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { const dialog = await page.find("calcite-dialog >>> .container"); const insideEl = await page.find("#insideEl"); @@ -1214,5 +1231,26 @@ describe("calcite-dialog", () => { const activeElementId = await page.evaluate(() => document.activeElement.id); expect(activeElementId).toBe(await insideEl.getProperty("id")); }); + + it("can tab out of dialog when modal=true and focusTrapDisabled=false", async () => { + const dialog = await page.find("calcite-dialog >>> .container"); + const action = await page.find("calcite-dialog >>> calcite-action"); + const outsideEl = await page.find("#outsideEl"); + + await action.callMethod("setFocus"); + await page.waitForChanges(); + + expect(await dialog.isVisible()).toBe(true); + + await page.keyboard.press("Tab"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); + await page.waitForChanges(); + + const activeElementId = await page.evaluate(() => document.activeElement.id); + expect(activeElementId).toBe(await outsideEl.getProperty("id")); + }); }); }); diff --git a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts index 993f81c215d..d8a27d0c89b 100644 --- a/packages/calcite-components/src/utils/focusTrapComponent.spec.ts +++ b/packages/calcite-components/src/utils/focusTrapComponent.spec.ts @@ -113,15 +113,19 @@ describe("focusTrapComponent", () => { }); }); describe("focusTrapDisabledOverride", () => { - it("should activate focus trap when focusTrapDisabledOverride returns false", () => { - const fakeComponent = {} as FocusTrapComponent; + const fakeComponent = {} as FocusTrapComponent; + let activateSpy: ReturnType; + + beforeEach(() => { fakeComponent.el = document.createElement("div"); connectFocusTrap(fakeComponent); - const activateSpy = vi.fn(); + activateSpy = vi.fn(); fakeComponent.focusTrap.activate = activateSpy; + }); + it("should activate focus trap when focusTrapDisabledOverride returns false", () => { fakeComponent.focusTrapDisabledOverride = () => false; activateFocusTrap(fakeComponent); @@ -130,14 +134,6 @@ describe("focusTrapComponent", () => { }); it("should not activate focus trap when focusTrapDisabledOverride returns true", () => { - const fakeComponent = {} as FocusTrapComponent; - fakeComponent.el = document.createElement("div"); - - connectFocusTrap(fakeComponent); - - const activateSpy = vi.fn(); - fakeComponent.focusTrap.activate = activateSpy; - fakeComponent.focusTrapDisabledOverride = () => true; activateFocusTrap(fakeComponent); From 360898bb9d09bd105da49fb36b99a3a3a3b95e47 Mon Sep 17 00:00:00 2001 From: eliza Date: Tue, 4 Feb 2025 13:36:42 -0800 Subject: [PATCH 21/26] change type on focusTrapComponent interface for useFocusTrap controller to correspond to component types and clean up the e2e test --- .../src/components/dialog/dialog.e2e.ts | 10 +++------- .../calcite-components/src/controllers/useFocusTrap.ts | 4 ++-- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 20473a4c163..538445773ed 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1232,13 +1232,9 @@ describe("calcite-dialog", () => { expect(activeElementId).toBe(await insideEl.getProperty("id")); }); - it("can tab out of dialog when modal=true and focusTrapDisabled=false", async () => { + it("cannot tab out of dialog when modal=true and focusTrapDisabled=false", async () => { const dialog = await page.find("calcite-dialog >>> .container"); - const action = await page.find("calcite-dialog >>> calcite-action"); - const outsideEl = await page.find("#outsideEl"); - - await action.callMethod("setFocus"); - await page.waitForChanges(); + const insideEl = await page.find("#insideEl"); expect(await dialog.isVisible()).toBe(true); @@ -1250,7 +1246,7 @@ describe("calcite-dialog", () => { await page.waitForChanges(); const activeElementId = await page.evaluate(() => document.activeElement.id); - expect(activeElementId).toBe(await outsideEl.getProperty("id")); + expect(activeElementId).toBe(await insideEl.getProperty("id")); }); }); }); diff --git a/packages/calcite-components/src/controllers/useFocusTrap.ts b/packages/calcite-components/src/controllers/useFocusTrap.ts index 13a5e58e04f..90d5398ae3f 100644 --- a/packages/calcite-components/src/controllers/useFocusTrap.ts +++ b/packages/calcite-components/src/controllers/useFocusTrap.ts @@ -47,12 +47,12 @@ interface FocusTrapComponent extends LitElement { /** * When `true` prevents focus trapping. */ - focusTrapDisabled?: boolean | (() => boolean); + focusTrapDisabled?: boolean; /** * When defined, provides a condition to disable focus trapping. When `true`, prevents focus trapping. */ - focusTrapDisabledOverride?: boolean | (() => boolean); + focusTrapDisabledOverride?: () => boolean; } /** From 2187f3c814772eebbfb3911106d97f96bfa1e31e Mon Sep 17 00:00:00 2001 From: eliza Date: Tue, 4 Feb 2025 17:59:50 -0800 Subject: [PATCH 22/26] set modal and focusTrapDisabled properties --- .../src/components/dialog/dialog.e2e.ts | 40 ++++++++++++------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 538445773ed..231db04d12c 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1167,9 +1167,7 @@ describe("calcite-dialog", () => { beforeEach(async () => { page = await newE2EPage(); await page.setContent(html` - + `); @@ -1178,10 +1176,15 @@ describe("calcite-dialog", () => { }); it("can tab out of non-modal dialog when focusTrapDisabled=true", async () => { - const dialog = await page.find("calcite-dialog >>> .container"); + const dialog = await page.find("calcite-dialog"); const action = await page.find("calcite-dialog >>> calcite-action"); const outsideEl = await page.find("#outsideEl"); + dialog.setProperty("modal", false); + dialog.setProperty("focusTrapDisabled", true); + + await page.waitForChanges(); + expect(await dialog.isVisible()).toBe(true); await action.callMethod("setFocus"); @@ -1191,21 +1194,22 @@ describe("calcite-dialog", () => { await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); - await page.keyboard.press("Tab"); - await page.waitForChanges(); const activeElementId = await page.evaluate(() => document.activeElement.id); expect(activeElementId).toBe(await outsideEl.getProperty("id")); }); it("cannot tab out of non-modal dialog when focusTrapDisabled=false", async () => { - const dialog = await page.find("calcite-dialog >>> .container"); + const dialog = await page.find("calcite-dialog"); const insideEl = await page.find("#insideEl"); - expect(await dialog.isVisible()).toBe(true); + dialog.setProperty("modal", false); + dialog.setProperty("focusTrapDisabled", false); - await page.keyboard.press("Tab"); await page.waitForChanges(); + + expect(await dialog.isVisible()).toBe(true); + await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); @@ -1216,13 +1220,16 @@ describe("calcite-dialog", () => { }); it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { - const dialog = await page.find("calcite-dialog >>> .container"); + const dialog = await page.find("calcite-dialog"); const insideEl = await page.find("#insideEl"); - expect(await dialog.isVisible()).toBe(true); + dialog.setProperty("modal", true); + dialog.setProperty("focusTrapDisabled", true); - await page.keyboard.press("Tab"); await page.waitForChanges(); + + expect(await dialog.isVisible()).toBe(true); + await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); @@ -1233,13 +1240,16 @@ describe("calcite-dialog", () => { }); it("cannot tab out of dialog when modal=true and focusTrapDisabled=false", async () => { - const dialog = await page.find("calcite-dialog >>> .container"); + const dialog = await page.find("calcite-dialog"); const insideEl = await page.find("#insideEl"); - expect(await dialog.isVisible()).toBe(true); + dialog.setProperty("modal", true); + dialog.setProperty("focusTrapDisabled", false); - await page.keyboard.press("Tab"); await page.waitForChanges(); + + expect(await dialog.isVisible()).toBe(true); + await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); From b0792a67df78afe6ed2971b9ab42cf040810e486 Mon Sep 17 00:00:00 2001 From: eliza Date: Tue, 4 Feb 2025 18:34:09 -0800 Subject: [PATCH 23/26] e2e set focus --- .../calcite-components/src/components/dialog/dialog.e2e.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 231db04d12c..8d3bf8bb2a6 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1241,6 +1241,7 @@ describe("calcite-dialog", () => { it("cannot tab out of dialog when modal=true and focusTrapDisabled=false", async () => { const dialog = await page.find("calcite-dialog"); + const action = await page.find("calcite-dialog >>> calcite-action"); const insideEl = await page.find("#insideEl"); dialog.setProperty("modal", true); @@ -1250,6 +1251,11 @@ describe("calcite-dialog", () => { expect(await dialog.isVisible()).toBe(true); + await action.callMethod("setFocus"); + await page.waitForChanges(); + + await page.keyboard.press("Tab"); + await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); From 11f8a0b7b5faa0933625c3fdd75805a44aac741d Mon Sep 17 00:00:00 2001 From: eliza Date: Tue, 4 Feb 2025 21:33:09 -0800 Subject: [PATCH 24/26] no need to react to the prop change outside of onOpen --- .../src/components/dialog/dialog.tsx | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index ae1fdf2f2e4..f6695a4e114 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -323,9 +323,6 @@ export class Dialog extends LitElement implements OpenCloseComponent, LoadableCo if (changes.has("modal") && (this.hasUpdated || this.modal !== false)) { this.updateOverflowHiddenClass(); } - if ((changes.has("modal") || changes.has("focusTrapDisabled")) && this.hasUpdated) { - this.handleFocusTrapDisabled(); - } if ( (changes.has("open") && (this.hasUpdated || this.open !== false)) || @@ -364,14 +361,6 @@ export class Dialog extends LitElement implements OpenCloseComponent, LoadableCo // #region Private Methods - private handleFocusTrapDisabled(): void { - if (!this.open) { - return; - } - - this.focusTrap.activate(); - } - private updateAssistiveText(): void { const { messages } = this; this.assistiveText = @@ -386,7 +375,12 @@ export class Dialog extends LitElement implements OpenCloseComponent, LoadableCo onOpen(): void { this.calciteDialogOpen.emit(); - this.handleFocusTrapDisabled(); + + if (!this.open) { + return; + } + + this.focusTrap.activate(); } onBeforeClose(): void { From 96a6d1d051064066655ac2f13730f76ba4e4afbd Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 5 Feb 2025 11:23:35 -0800 Subject: [PATCH 25/26] group e2e tests based on whether it's a modal or non-modal, cleanup --- .../src/components/dialog/dialog.e2e.ts | 59 +++++++++++-------- .../src/components/dialog/dialog.tsx | 5 -- 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 8d3bf8bb2a6..4e7b68afe57 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1161,13 +1161,13 @@ describe("calcite-dialog", () => { }); }); - describe("focusTrap", () => { + describe("focusTrap behavior for modal dialogs", () => { let page: E2EPage; beforeEach(async () => { page = await newE2EPage(); await page.setContent(html` - + `); @@ -1175,41 +1175,41 @@ describe("calcite-dialog", () => { await page.waitForChanges(); }); - it("can tab out of non-modal dialog when focusTrapDisabled=true", async () => { + it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { const dialog = await page.find("calcite-dialog"); - const action = await page.find("calcite-dialog >>> calcite-action"); - const outsideEl = await page.find("#outsideEl"); + const insideEl = await page.find("#insideEl"); - dialog.setProperty("modal", false); dialog.setProperty("focusTrapDisabled", true); await page.waitForChanges(); expect(await dialog.isVisible()).toBe(true); - await action.callMethod("setFocus"); - await page.waitForChanges(); - await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); const activeElementId = await page.evaluate(() => document.activeElement.id); - expect(activeElementId).toBe(await outsideEl.getProperty("id")); + expect(activeElementId).toBe(await insideEl.getProperty("id")); }); - it("cannot tab out of non-modal dialog when focusTrapDisabled=false", async () => { + it("cannot tab out of dialog when modal=true and focusTrapDisabled=false", async () => { const dialog = await page.find("calcite-dialog"); + const action = await page.find("calcite-dialog >>> calcite-action"); const insideEl = await page.find("#insideEl"); - dialog.setProperty("modal", false); dialog.setProperty("focusTrapDisabled", false); await page.waitForChanges(); expect(await dialog.isVisible()).toBe(true); + await action.callMethod("setFocus"); + await page.waitForChanges(); + + await page.keyboard.press("Tab"); + await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); @@ -1218,44 +1218,55 @@ describe("calcite-dialog", () => { const activeElementId = await page.evaluate(() => document.activeElement.id); expect(activeElementId).toBe(await insideEl.getProperty("id")); }); + }); - it("cannot tab out of dialog when modal=true and focusTrapDisabled=true", async () => { + describe("focusTrap behavior for non-modal dialogs", () => { + let page: E2EPage; + + beforeEach(async () => { + page = await newE2EPage(); + await page.setContent(html` + + + `); + + await skipAnimations(page); + await page.waitForChanges(); + }); + + it("can tab out of non-modal dialog when focusTrapDisabled=true", async () => { const dialog = await page.find("calcite-dialog"); - const insideEl = await page.find("#insideEl"); + const action = await page.find("calcite-dialog >>> calcite-action"); + const outsideEl = await page.find("#outsideEl"); - dialog.setProperty("modal", true); dialog.setProperty("focusTrapDisabled", true); await page.waitForChanges(); expect(await dialog.isVisible()).toBe(true); + await action.callMethod("setFocus"); + await page.waitForChanges(); + await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); const activeElementId = await page.evaluate(() => document.activeElement.id); - expect(activeElementId).toBe(await insideEl.getProperty("id")); + expect(activeElementId).toBe(await outsideEl.getProperty("id")); }); - it("cannot tab out of dialog when modal=true and focusTrapDisabled=false", async () => { + it("cannot tab out of non-modal dialog when focusTrapDisabled=false", async () => { const dialog = await page.find("calcite-dialog"); - const action = await page.find("calcite-dialog >>> calcite-action"); const insideEl = await page.find("#insideEl"); - dialog.setProperty("modal", true); dialog.setProperty("focusTrapDisabled", false); await page.waitForChanges(); expect(await dialog.isVisible()).toBe(true); - await action.callMethod("setFocus"); - await page.waitForChanges(); - - await page.keyboard.press("Tab"); - await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab"); diff --git a/packages/calcite-components/src/components/dialog/dialog.tsx b/packages/calcite-components/src/components/dialog/dialog.tsx index f6695a4e114..eea354ce3e1 100644 --- a/packages/calcite-components/src/components/dialog/dialog.tsx +++ b/packages/calcite-components/src/components/dialog/dialog.tsx @@ -375,11 +375,6 @@ export class Dialog extends LitElement implements OpenCloseComponent, LoadableCo onOpen(): void { this.calciteDialogOpen.emit(); - - if (!this.open) { - return; - } - this.focusTrap.activate(); } From ff3413d92b5672a913d4007f482bc9e762188ad8 Mon Sep 17 00:00:00 2001 From: eliza Date: Wed, 5 Feb 2025 13:09:37 -0800 Subject: [PATCH 26/26] safeguard e2e --- .../calcite-components/src/components/dialog/dialog.e2e.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/calcite-components/src/components/dialog/dialog.e2e.ts b/packages/calcite-components/src/components/dialog/dialog.e2e.ts index 4e7b68afe57..b7719c9fe5c 100644 --- a/packages/calcite-components/src/components/dialog/dialog.e2e.ts +++ b/packages/calcite-components/src/components/dialog/dialog.e2e.ts @@ -1259,6 +1259,7 @@ describe("calcite-dialog", () => { it("cannot tab out of non-modal dialog when focusTrapDisabled=false", async () => { const dialog = await page.find("calcite-dialog"); + const action = await page.find("calcite-dialog >>> calcite-action"); const insideEl = await page.find("#insideEl"); dialog.setProperty("focusTrapDisabled", false); @@ -1267,6 +1268,11 @@ describe("calcite-dialog", () => { expect(await dialog.isVisible()).toBe(true); + await action.callMethod("setFocus"); + await page.waitForChanges(); + + await page.keyboard.press("Tab"); + await page.waitForChanges(); await page.keyboard.press("Tab"); await page.waitForChanges(); await page.keyboard.press("Tab");