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

refactor(modal): Update modal to use focus-trap module. #5676

Merged
merged 21 commits into from
Nov 16, 2022
Merged
Show file tree
Hide file tree
Changes from 12 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
38 changes: 27 additions & 11 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,11 @@
"url": "git+https://github.com/Esri/calcite-components.git"
},
"dependencies": {
"@a11y/focus-trap": "1.0.5",
"@floating-ui/dom": "1.0.3",
"@stencil/core": "2.18.1",
"@types/color": "3.0.3",
"color": "4.2.3",
"focus-trap": "7.0.0",
"form-request-submit-polyfill": "2.0.0",
"lodash-es": "4.17.21",
"sortablejs": "1.15.0"
Expand Down
18 changes: 10 additions & 8 deletions src/components/modal/modal.e2e.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { newE2EPage } from "@stencil/core/testing";
import { focusable, renders, slots, hidden } from "../../tests/commonTests";
import { html } from "../../../support/formatting";
import { CSS, SLOTS } from "./resources";
import { CSS, SLOTS, ANIMATION_TIMEOUT } from "./resources";
import { newProgrammaticE2EPage, skipAnimations } from "../../tests/utils";

describe("calcite-modal properties", () => {
Expand Down Expand Up @@ -235,7 +235,8 @@ describe("calcite-modal accessibility checks", () => {
});

describe("setFocus", () => {
const createModalHTML = (contentHTML?: string) => `<calcite-modal active>${contentHTML}</calcite-modal>`;
const createModalHTML = (contentHTML?: string, attrs?: string) =>
`<calcite-modal active ${attrs}>${contentHTML}</calcite-modal>`;

const closeButtonFocusId = "close-button";
const closeButtonTargetSelector = ".close";
Expand All @@ -244,17 +245,18 @@ describe("calcite-modal accessibility checks", () => {
const focusableContentHTML = html`<h3 slot="header">Title</h3>
<p slot="content">This is the content <button class=${focusableContentTargetClass}>test</button></p>`;

it("focuses focusable content by default", async () =>
it("focuses close button by default", async () =>
focusable(createModalHTML(focusableContentHTML), {
focusTargetSelector: `.${focusableContentTargetClass}`
}));

it("focuses close button if there is no focusable content", async () =>
focusable(createModalHTML(), {
focusId: closeButtonFocusId,
shadowFocusTargetSelector: closeButtonTargetSelector
}));

it("focuses content if there is no close button", async () =>
focusable(createModalHTML(focusableContentHTML, "disable-close-button"), {
focusTargetSelector: `.${focusableContentTargetClass}`,
delay: ANIMATION_TIMEOUT
}));

it.skip("can focus close button directly", async () =>
focusable(createModalHTML(focusableContentHTML), {
focusId: closeButtonFocusId,
Expand Down
70 changes: 25 additions & 45 deletions src/components/modal/modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,7 @@ import {
VNode,
Watch
} from "@stencil/core";
import {
ensureId,
FocusableElement,
focusElement,
getSlotted,
isCalciteFocusable
} from "../../utils/dom";

import { queryShadowRoot } from "@a11y/focus-trap/shadow";
import { isFocusable, isHidden } from "@a11y/focus-trap/focusable";
import { ensureId, focusElement, getSlotted } from "../../utils/dom";
import { Scale } from "../interfaces";
import { ModalBackgroundColor } from "./interfaces";
import { CSS, ICONS, SLOTS, TEXT } from "./resources";
Expand All @@ -36,14 +27,13 @@ import {
connectOpenCloseComponent,
disconnectOpenCloseComponent
} from "../../utils/openCloseComponent";

const isFocusableExtended = (el: FocusableElement): boolean => {
return isCalciteFocusable(el) || isFocusable(el);
};

const getFocusableElements = (el: HTMLElement | ShadowRoot): HTMLElement[] => {
return queryShadowRoot(el, isHidden, isFocusableExtended);
};
import {
FocusTrapComponent,
FocusTrap,
connectFocusTrap,
activateFocusTrap,
deactivateFocusTrap
} from "../../utils/focusTrapComponent";

/**
* @slot header - A slot for adding header text.
Expand All @@ -58,7 +48,7 @@ const getFocusableElements = (el: HTMLElement | ShadowRoot): HTMLElement[] => {
styleUrl: "modal.scss",
shadow: true
})
export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
export class Modal implements ConditionalSlotComponent, OpenCloseComponent, FocusTrapComponent {
//--------------------------------------------------------------------------
//
// Element
Expand Down Expand Up @@ -156,6 +146,7 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
this.mutationObserver?.disconnect();
disconnectConditionalSlotComponent(this);
disconnectOpenCloseComponent(this);
deactivateFocusTrap(this);
}

render(): VNode {
Expand All @@ -175,7 +166,6 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
}}
ref={this.setTransitionEl}
>
<div data-focus-fence onFocus={this.focusLastElement} tabindex="0" />
<div class={CSS.header}>
{this.renderCloseButton()}
<header class={CSS.title}>
Expand All @@ -193,7 +183,6 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
<slot name={SLOTS.content} />
</div>
{this.renderFooter()}
<div data-focus-fence onFocus={this.focusFirstElement} tabindex="0" />
</div>
</Host>
);
Expand Down Expand Up @@ -286,14 +275,16 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
this.updateFooterVisibility()
);

previousActiveElement: HTMLElement;

titleId: string;

openTransitionProp = "opacity";

transitionEl: HTMLDivElement;

focusTrap: FocusTrap;

focusTrapEl: HTMLDivElement;

//--------------------------------------------------------------------------
//
// Event Listeners
Expand Down Expand Up @@ -355,11 +346,13 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
*/
@Method()
async setFocus(focusId?: "close-button"): Promise<void> {
const closeButton = this.closeButtonEl;
const { closeButtonEl } = this;

return focusElement(
focusId === "close-button" ? closeButton : getFocusableElements(this.el)[0] || closeButton
);
if (closeButtonEl && focusId === "close-button") {
Copy link
Member

Choose a reason for hiding this comment

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

Since close-button is the only supported ID, I think you could just check for closeButtonEl being present or not. Passing any other ID would technically be invalid, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. I think its nicer to check and fallback incase the user entered an invalid ID>

return focusElement(closeButtonEl);
}

activateFocusTrap(this);
}

/**
Expand All @@ -386,9 +379,11 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
//
//--------------------------------------------------------------------------

private setTransitionEl = (el): void => {
private setTransitionEl = (el: HTMLDivElement): void => {
this.transitionEl = el;
connectOpenCloseComponent(this);
this.focusTrapEl = el;
connectFocusTrap(this);
};

onBeforeOpen(): void {
Expand All @@ -409,6 +404,7 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
onClose(): void {
this.transitionEl.classList.remove(CSS.closingIdle, CSS.closingActive);
this.calciteModalClose.emit();
deactivateFocusTrap(this);
}

@Watch("active")
Expand All @@ -431,11 +427,11 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
private openEnd = (): void => {
this.setFocus();
this.el.removeEventListener("calciteModalOpen", this.openEnd);
activateFocusTrap(this);
};

/** Open the modal */
private openModal() {
this.previousActiveElement = document.activeElement as HTMLElement;
this.el.addEventListener("calciteModalOpen", this.openEnd);
this.open = true;
this.isOpen = true;
Expand All @@ -461,26 +457,10 @@ export class Modal implements ConditionalSlotComponent, OpenCloseComponent {
return this.beforeClose(this.el).then(() => {
this.open = false;
this.isOpen = false;
focusElement(this.previousActiveElement);
this.removeOverflowHiddenClass();
});
};

focusFirstElement = (): void => {
focusElement(this.disableCloseButton ? getFocusableElements(this.el)[0] : this.closeButtonEl);
};

focusLastElement = (): void => {
const focusableElements = getFocusableElements(this.el).filter(
(el) => !el.getAttribute("data-focus-fence")
);
if (focusableElements.length > 0) {
focusElement(focusableElements[focusableElements.length - 1]);
} else {
focusElement(this.closeButtonEl);
}
};

private removeOverflowHiddenClass(): void {
document.documentElement.classList.remove(CSS.overflowHidden);
}
Expand Down
2 changes: 2 additions & 0 deletions src/components/modal/resources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,5 @@ export const SLOTS = {
export const TEXT = {
close: "Close"
};

export const ANIMATION_TIMEOUT = 300;
9 changes: 9 additions & 0 deletions src/tests/commonTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,11 @@ interface FocusableOptions {
* selector used to assert the focused shadow DOM element
*/
shadowFocusTargetSelector?: string;

/**
* Used to delay setFocus call in ms.
*/
delay?: number;
Copy link
Member

Choose a reason for hiding this comment

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

If this is for the open/close transition, I'd suggest using the skipAnimation helper instead of introducing a delay argument. If it's something specific to focus-trap behavior, can you add more info about this to the documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcfranco the problem is the delayInitialFocus is set to true.

I tried delayInitialFocus: !Build.isTesting, but the problem is that its not returning true.

{"testing":false,"browser":true,"dev":false,"server":false} 
    Location: http://localhost:3333/build/p-371d7086.entry.js:15:2306

}

/**
Expand All @@ -210,6 +215,10 @@ export async function focusable(componentTagOrHTML: TagOrHTML, options?: Focusab
const element = await page.find(tag);
const focusTargetSelector = options?.focusTargetSelector || tag;

if (options?.delay) {
await page.waitForTimeout(options.delay);
}

await element.callMethod("setFocus", options?.focusId); // assumes element is FocusableElement

if (options?.shadowFocusTargetSelector) {
Expand Down
Loading