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.

Sign up for GitHub

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
+114 −3
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0439c86
feat(dialog): add focusTrapDisabled attribute to for non-modal dialogs
Elijbet Jan 22, 2025
702b4c8
rework logic
Elijbet Jan 22, 2025
61cb7b2
WIP
Elijbet Jan 22, 2025
3fd589b
doc
Elijbet Jan 22, 2025
5225520
handleFocusTrap - deactivate when not open
Elijbet Jan 23, 2025
99aa605
WIP
Elijbet Jan 23, 2025
7e910f6
willUpdate changes
Elijbet Jan 23, 2025
a82272d
cleanup test and remove check from activateFocusTrap
Elijbet Jan 23, 2025
aa64339
more control in tests
Elijbet Jan 23, 2025
66736a4
adjust visibility check
Elijbet Jan 24, 2025
f4a4a99
cleanup
Elijbet Jan 24, 2025
bdf7a80
cleanup
Elijbet Jan 24, 2025
cf808df
Merge branch 'dev' into elijbet/10685-focusTrapDisabled-attribute-for…
Elijbet Jan 28, 2025
278464f
assert on the element focus in tests and add _focusTrapDisabledOverri…
Elijbet Jan 29, 2025
0e2ab22
typeError fix
Elijbet Jan 29, 2025
826d93f
refactor and cleanup
Elijbet Jan 31, 2025
0c7a4f9
consistency with focusTrapDisabled and Override
Elijbet Jan 31, 2025
2873183
corrected doc and condition, added spec test
Elijbet Jan 31, 2025
5c11de0
focusTrapDisabledOverride spec
Elijbet Jan 31, 2025
ee12e16
cleanup
Elijbet Jan 31, 2025
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
33 changes: 33 additions & 0 deletions packages/calcite-components/src/components/dialog/dialog.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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` <calcite-dialog focus-trap-disabled open closable></calcite-dialog> `);
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);
});
});
});
21 changes: 20 additions & 1 deletion packages/calcite-components/src/components/dialog/dialog.tsx
View file
Open in desktop
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`, prevents focus trapping. */
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
@property({ reflect: true }) focusTrapDisabled = false;

/** When `true`, displays and positions the component. */
@property({ reflect: true })
get open(): boolean {
Expand Down Expand Up @@ -328,6 +331,9 @@ export class Dialog
if (changes.has("modal") && (this.hasUpdated || this.modal !== false)) {
this.updateOverflowHiddenClass();
}
if (changes.has("modal") || this.focusTrapDisabled) {
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
this.handleFocusTrapDisabled(this.focusTrapDisabled);
}

if (
(changes.has("open") && (this.hasUpdated || this.open !== false)) ||
Expand Down Expand Up @@ -366,6 +372,19 @@ export class Dialog
// #endregion

// #region Private Methods

private handleFocusTrapDisabled(focusTrapDisabled: boolean): void {
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
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 •
edited
Loading

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 •
edited
Loading

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

if (this.modal || (this.modal === false && focusTrapDisabled === false)) {
Elijbet marked this conversation as resolved.
Show resolved Hide resolved
activateFocusTrap(this);
} else {
deactivateFocusTrap(this);
}
}

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

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

onBeforeClose(): void {
Expand Down
Loading