Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dialog): add focusTrapDisabled property for non-modal dialogs #11362

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion packages/calcite-components/src/components/dialog/dialog.e2e.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -1160,4 +1160,59 @@ describe("calcite-dialog", () => {
expect(await dialog.getProperty("open")).toBe(true);
});
});

describe("focusTrap", () => {
let page: E2EPage;

beforeEach(async () => {
page = await newE2EPage();
await page.setContent(html`
<calcite-dialog width-scale="s" focus-trap-disabled open closable
><button id="insideEl">inside</button></calcite-dialog
>
<button id="outsideEl">outside</button>
`);

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");

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");
await page.waitForChanges();

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 () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness, can you add tests that cover focusTrapDisabled = false?

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"));
});
});
});
22 changes: 21 additions & 1 deletion packages/calcite-components/src/components/dialog/dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,9 @@ export class Dialog
/** When `true`, displays a scrim blocking interaction underneath the component. */
@property({ reflect: true }) modal = false;

/** When `true` and `modal` is `false`, prevents focus trapping. */
@property({ reflect: true }) focusTrapDisabled = false;

/** When `true`, displays and positions the component. */
@property({ reflect: true })
get open(): boolean {
Expand Down Expand Up @@ -270,6 +273,11 @@ export class Dialog
updateFocusTrapElements(this);
}

/** When defined, provides a condition to disable focus trapping. When `true`, prevents focus trapping. */
focusTrapDisabledOverride(): boolean {
return !this.modal && this.focusTrapDisabled;
}

// #endregion

// #region Events
Expand Down Expand Up @@ -328,6 +336,9 @@ export class Dialog
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)) ||
Expand Down Expand Up @@ -366,6 +377,15 @@ export class Dialog
// #endregion

// #region Private Methods

private handleFocusTrapDisabled(): void {
if (!this.open) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it deactivate if not open?

Copy link
Member

@driskull driskull Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic needs to handle changes to open, modal and focusTrapDisabled.

  • if open is false => do nothing
  • modal is true or focusTrapDisabled is false => activateFocusTrap
  • if focusTrapDisabled is true and modal is false => deactivateFocusTrap
  • else deactivateFocusTrap

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this hasn't been resolved yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the onOpen/onClose handles the deactivation part. We may not need this check at all. We can probably remove this function and call activateFocusTrap in its place

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Sounds like focusTrapDisabled is intended for use at "open" time. Maybe we can clarify this in the docs. cc @geospatialem @DitwanP

Copy link
Contributor Author

@Elijbet Elijbet Feb 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we handle that in multiple places. onClose takes care of deactivateFocusTrap, and the rest happens in
focusTrapDisabledOverride(): boolean { return !this.modal && this.focusTrapDisabled; }

return;
}

activateFocusTrap(this);
}

private updateAssistiveText(): void {
const { messages } = this;
this.assistiveText =
Expand All @@ -380,7 +400,7 @@ export class Dialog

onOpen(): void {
this.calciteDialogOpen.emit();
activateFocusTrap(this);
this.handleFocusTrapDisabled();
}

onBeforeClose(): void {
Expand Down
33 changes: 33 additions & 0 deletions packages/calcite-components/src/utils/focusTrapComponent.spec.ts
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,37 @@ describe("focusTrapComponent", () => {
expect(customFocusTrapStack).toHaveLength(1);
});
});
describe("focusTrapDisabledOverride", () => {
it("should activate focus trap when focusTrapDisabledOverride returns false", () => {
const fakeComponent = {} as FocusTrapComponent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's DRY test setup by using beforeEach.

fakeComponent.el = document.createElement("div");

connectFocusTrap(fakeComponent);

const activateSpy = vi.fn();
fakeComponent.focusTrap.activate = activateSpy;

fakeComponent.focusTrapDisabledOverride = () => false;

activateFocusTrap(fakeComponent);

expect(activateSpy).toHaveBeenCalledTimes(1);
});

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);

expect(activateSpy).toHaveBeenCalledTimes(0);
});
});
});
5 changes: 4 additions & 1 deletion packages/calcite-components/src/utils/focusTrapComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ export interface FocusTrapComponent {
/** When `true`, prevents focus trapping. */
focusTrapDisabled?: boolean;

/** When defined, provides a condition to disable focus trapping. When `true`, prevents focus trapping. */
focusTrapDisabledOverride?: () => boolean;
Elijbet marked this conversation as resolved.
Show resolved Hide resolved

/** The focus trap instance. */
focusTrap: FocusTrap;

Expand Down Expand Up @@ -73,7 +76,7 @@ export function activateFocusTrap(
component: FocusTrapComponent,
options?: Parameters<_FocusTrap["activate"]>[0],
): void {
if (!component.focusTrapDisabled) {
if (component.focusTrapDisabledOverride ? !component.focusTrapDisabledOverride() : !component.focusTrapDisabled) {
component.focusTrap?.activate(options);
}
}
Expand Down