From 09c47ce786e1fc96f970c0cff36146a3d0ae8e33 Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 1 Feb 2024 16:17:14 -0600 Subject: [PATCH 01/16] Make drawer more robust to CloseWatcher API --- .../nimble-components/src/drawer/index.ts | 32 +++++++++++++++++++ .../nimble-components/src/drawer/template.ts | 2 ++ 2 files changed, 34 insertions(+) diff --git a/packages/nimble-components/src/drawer/index.ts b/packages/nimble-components/src/drawer/index.ts index c4cdaa850d..98f6d18cb2 100644 --- a/packages/nimble-components/src/drawer/index.ts +++ b/packages/nimble-components/src/drawer/index.ts @@ -92,6 +92,38 @@ export class Drawer extends FoundationElement { return true; } + /** + * @internal + */ + public closeHandler(): void { + if (this.resolveShow) { + // This happens if all of the following are true: + // 1. the browser implements dialogs with the CloseWatcher API + // 2. preventDismiss is true + // 3. nothing in the drawer has focus + // 4. the user pressed ESC twice without intervening user interaction (e.g. clicking) + // In that case, cancel is not fired because of #1 & #4, we don't get a keydown + // because of #3, and the dialog just closes (without animation). + // Ideally, preventDismiss should always prevent ESC from closing the dialog, + // but we can't work around this corner case. + this.resolveShow(UserDismissed); + this.resolveShow = undefined; + } + } + + /** + * @internal + */ + public keydownHandler(event: KeyboardEvent): boolean { + // Historically, we could expect a cancel event to fire every time the user presses ESC, + // so a separate keydownHandler would not be needed. But with the advent of the CloseWatcher + // API, cancel is not fired if there was no user interaction since the last ESC + // (https://github.com/WICG/close-watcher?tab=readme-ov-file#asking-for-confirmation). + // In that case, we have to rely on handling keydown instead. But we only get a keydown + // if an element within the drawer has focus. + return event.key === 'Escape' ? this.cancelHandler(event) : true; + } + private readonly animationEndHandlerFunction = (): void => this.animationEndHandler(); private openDialog(): void { diff --git a/packages/nimble-components/src/drawer/template.ts b/packages/nimble-components/src/drawer/template.ts index 11a269642c..c23ec66398 100644 --- a/packages/nimble-components/src/drawer/template.ts +++ b/packages/nimble-components/src/drawer/template.ts @@ -6,6 +6,8 @@ export const template = html` ${ref('dialog')} aria-label="${x => x.ariaLabel}" @cancel="${(x, c) => x.cancelHandler(c.event)}" + @close="${x => x.closeHandler()}" + @keydown="${(x, c) => x.keydownHandler(c.event as KeyboardEvent)}" >
From ca4377f3b0a27c42a28a425f3f1bd6618a20a8af Mon Sep 17 00:00:00 2001 From: Mert Akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 1 Feb 2024 18:22:42 -0600 Subject: [PATCH 02/16] Make dialog more robust to CloseWatcher API (refactor) --- .../src/dialog-base/index.ts | 121 ++++++++++++++++++ .../nimble-components/src/dialog/index.ts | 80 +----------- .../nimble-components/src/dialog/styles.ts | 5 + .../nimble-components/src/dialog/template.ts | 2 + .../src/dialog/tests/dialog.spec.ts | 3 +- .../nimble-components/src/drawer/index.ts | 112 ++++------------ .../nimble-components/src/drawer/template.ts | 2 +- 7 files changed, 161 insertions(+), 164 deletions(-) create mode 100644 packages/nimble-components/src/dialog-base/index.ts diff --git a/packages/nimble-components/src/dialog-base/index.ts b/packages/nimble-components/src/dialog-base/index.ts new file mode 100644 index 0000000000..3dfb4e7c48 --- /dev/null +++ b/packages/nimble-components/src/dialog-base/index.ts @@ -0,0 +1,121 @@ +import { attr } from '@microsoft/fast-element'; +import { FoundationElement } from '@microsoft/fast-foundation'; +import { UserDismissed } from '../patterns/dialog/types'; + +/** + * This is a workaround for an incomplete definition of the native dialog element. Remove when using Typescript >=4.8.3. + * https://github.com/microsoft/TypeScript/issues/48267 + * @internal + */ +export interface ExtendedDialog extends HTMLDialogElement { + showModal(): void; + close(): void; +} + +/** + * A base class for components based on a native dialog. + */ +// eslint-disable-next-line @typescript-eslint/no-invalid-void-type +export abstract class DialogBase extends FoundationElement { + /** + * @public + * @description + * Prevents dismissing via the Escape key + */ + @attr({ attribute: 'prevent-dismiss', mode: 'boolean' }) + public preventDismiss = false; + + /** + * The ref to the internal dialog element. + * + * @internal + */ + public readonly dialogElement!: ExtendedDialog; + + /** + * True if the component is open/showing, false otherwise + */ + public get open(): boolean { + return this.resolveShow !== undefined; + } + + protected resolveShow?: (reason: CloseReason | UserDismissed) => void; + + /** + * Opens the component + * @returns Promise that is resolved when the component is closed. The value of the resolved Promise is the reason value passed to the close() method, or UserDismissed if closed via the ESC key. + */ + public async show(): Promise { + if (this.open) { + throw new Error('Already open'); + } + this.openDialog(); + return new Promise((resolve, _reject) => { + this.resolveShow = resolve; + }); + } + + /** + * Closes the component + * @param reason An optional value indicating how/why the component was closed. + */ + public close(reason: CloseReason): void { + if (!this.open) { + throw new Error('Not open'); + } + this.dialogElement.close(); + this.notifyClosed(reason); + } + + /** + * @internal + */ + public cancelHandler(event: Event): boolean { + if (this.preventDismiss) { + event.preventDefault(); + } else { + this.notifyClosed(UserDismissed); + } + return true; + } + + /** + * @internal + */ + public closeHandler(): void { + if (this.resolveShow) { + // This happens if all of the following are true: + // 1. the browser implements dialogs with the CloseWatcher API + // 2. preventDismiss is true + // 3. nothing in the component has focus + // 4. the user pressed ESC twice without intervening user interaction (e.g. clicking) + // In that case, cancel is not fired because of #1 & #4, we don't get a keydown + // because of #3, and the dialog just closes. + // Ideally, preventDismiss should always prevent ESC from closing the dialog, + // but we can't work around this corner case. + this.notifyClosed(UserDismissed); + } + } + + /** + * @internal + */ + public keydownHandler(event: KeyboardEvent): boolean { + // Historically, we could expect a cancel event to fire every time the user presses ESC, + // so a separate keydownHandler would not be needed. But with the advent of the CloseWatcher + // API, cancel is not fired if there was no user interaction since the last ESC + // (https://github.com/WICG/close-watcher?tab=readme-ov-file#asking-for-confirmation). + // In that case, we have to rely on handling keydown instead. But we only get a keydown + // if an element within the component has focus. + return event.key === 'Escape' ? this.cancelHandler(event) : true; + } + + protected openDialog(): void { + this.dialogElement.showModal(); + } + + protected notifyClosed(reason: CloseReason | UserDismissed): void { + this.resolveShow!(reason); + this.resolveShow = undefined; + } +} diff --git a/packages/nimble-components/src/dialog/index.ts b/packages/nimble-components/src/dialog/index.ts index 44398168c6..57e9571a39 100644 --- a/packages/nimble-components/src/dialog/index.ts +++ b/packages/nimble-components/src/dialog/index.ts @@ -2,10 +2,10 @@ import { attr, observable } from '@microsoft/fast-element'; import { applyMixins, ARIAGlobalStatesAndProperties, - DesignSystem, - FoundationElement + DesignSystem } from '@microsoft/fast-foundation'; import { UserDismissed } from '../patterns/dialog/types'; +import { DialogBase } from '../dialog-base'; import { styles } from './styles'; import { template } from './template'; @@ -17,33 +17,15 @@ declare global { } } -/** - * This is a workaround for an incomplete definition of the native dialog element. Remove when using Typescript >=4.8.3. - * https://github.com/microsoft/TypeScript/issues/48267 - * @internal - */ -export interface ExtendedDialog extends HTMLDialogElement { - showModal(): void; - close(): void; -} - /** * A nimble-styled dialog. */ // eslint-disable-next-line @typescript-eslint/no-invalid-void-type -export class Dialog extends FoundationElement { +export class Dialog extends DialogBase { // We want the member to match the name of the constant // eslint-disable-next-line @typescript-eslint/naming-convention public static readonly UserDismissed = UserDismissed; - /** - * @public - * @description - * Prevents dismissing the dialog via the Escape key - */ - @attr({ attribute: 'prevent-dismiss', mode: 'boolean' }) - public preventDismiss = false; - /** * @public * @description @@ -60,13 +42,6 @@ export class Dialog extends FoundationElement { @attr({ attribute: 'footer-hidden', mode: 'boolean' }) public footerHidden = false; - /** - * The ref to the internal dialog element. - * - * @internal - */ - public readonly dialogElement!: ExtendedDialog; - /** @internal */ @observable public footerIsEmpty = true; @@ -75,61 +50,12 @@ export class Dialog extends FoundationElement { @observable public readonly slottedFooterElements?: HTMLElement[]; - /** - * True if the dialog is open/showing, false otherwise - */ - public get open(): boolean { - return this.resolveShow !== undefined; - } - - private resolveShow?: (reason: CloseReason | UserDismissed) => void; - - /** - * Opens the dialog - * @returns Promise that is resolved when the dialog is closed. The value of the resolved Promise is the reason value passed to the close() method, or UserDismissed if the dialog was closed via the ESC key. - */ - public async show(): Promise { - if (this.open) { - throw new Error('Dialog is already open'); - } - this.dialogElement.showModal(); - return new Promise((resolve, _reject) => { - this.resolveShow = resolve; - }); - } - - /** - * Closes the dialog - * @param reason An optional value indicating how/why the dialog was closed. - */ - public close(reason: CloseReason): void { - if (!this.open) { - throw new Error('Dialog is not open'); - } - this.dialogElement.close(); - this.resolveShow!(reason); - this.resolveShow = undefined; - } - public slottedFooterElementsChanged( _prev: HTMLElement[] | undefined, next: HTMLElement[] | undefined ): void { this.footerIsEmpty = !next?.length; } - - /** - * @internal - */ - public cancelHandler(event: Event): boolean { - if (this.preventDismiss) { - event.preventDefault(); - } else { - this.resolveShow!(UserDismissed); - this.resolveShow = undefined; - } - return true; - } } // eslint-disable-next-line @typescript-eslint/no-empty-interface diff --git a/packages/nimble-components/src/dialog/styles.ts b/packages/nimble-components/src/dialog/styles.ts index 653fcd6c0b..bedbf55222 100644 --- a/packages/nimble-components/src/dialog/styles.ts +++ b/packages/nimble-components/src/dialog/styles.ts @@ -25,6 +25,7 @@ import { import { Theme } from '../theme-provider/types'; import { themeBehavior } from '../utilities/style/theme'; import { accessiblyHidden } from '../utilities/style/accessibly-hidden'; +import { focusVisible } from '../utilities/style/focus'; export const styles = css` ${display('grid')} @@ -44,6 +45,10 @@ export const styles = css` display: flex; } + dialog${focusVisible} { + outline: none; + } + header { min-height: 48px; padding: 24px 24px 0px 24px; diff --git a/packages/nimble-components/src/dialog/template.ts b/packages/nimble-components/src/dialog/template.ts index d27899a530..9d877adac7 100644 --- a/packages/nimble-components/src/dialog/template.ts +++ b/packages/nimble-components/src/dialog/template.ts @@ -8,6 +8,8 @@ export const template = html` role="dialog" part="control" @cancel="${(x, c) => x.cancelHandler(c.event)}" + @close="${x => x.closeHandler()}" + @keydown="${(x, c) => x.keydownHandler(c.event as KeyboardEvent)}" aria-labelledby="header" >