Refactor common dialog/drawer code into a base class

change/@ni-nimble-components-dc071140-b97c-473f-b929-1268a3aa3a40.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Make dialog and drawer more resilient to behavior changes from upcoming CloseWatcher API",
+  "packageName": "@ni/nimble-components",
+  "email": "7282195+m-akinc@users.noreply.github.com",
+  "dependentChangeType": "patch"
+}

packages/nimble-components/src/dialog-base/index.ts

@@ -0,0 +1,108 @@
+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<CloseReason = void> 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<CloseReason | UserDismissed> {
+        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) {
+            // If
+            // - the browser implements dialogs with the CloseWatcher API, and
+            // - the user presses ESC without first interacting with the dialog (e.g. clicking, scrolling),
+            // the cancel event is not fired and the dialog just closes.
+            this.notifyClosed(UserDismissed);
+        }
+    }
+
+    protected openDialog(): void {
+        this.dialogElement.showModal();
+    }
+
+    protected notifyClosed(reason: CloseReason | UserDismissed): void {
+        if (!this.resolveShow) {
+            throw new Error(
+                'Do not call notifyClosed unless there is a promise to resolve'
+            );
+        }
+        this.resolveShow(reason);
+        this.resolveShow = undefined;
+    }
+}

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<CloseReason = void> extends FoundationElement {
+export class Dialog<CloseReason = void> extends DialogBase<CloseReason> {
     // 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<CloseReason = void> 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<CloseReason = void> 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<CloseReason | UserDismissed> {
-        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

packages/nimble-components/src/dialog/styles.ts

@@ -15,7 +15,8 @@ import {
     elevation3BoxShadow,
     dialogSmallWidth,
     dialogSmallHeight,
-    dialogSmallMaxHeight
+    dialogSmallMaxHeight,
+    borderHoverColor
 } from '../theme-provider/design-tokens';
 import {
     modalBackdropColorThemeColorStatic,
@@ -25,6 +26,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 +46,10 @@ export const styles = css`
         display: flex;
     }
 
+    dialog${focusVisible} {
+        outline: 2px solid ${borderHoverColor};
+    }
+
     header {
         min-height: 48px;
         padding: 24px 24px 0px 24px;

packages/nimble-components/src/dialog/template.ts

@@ -8,6 +8,7 @@ export const template = html<Dialog>`
             role="dialog"
             part="control"
             @cancel="${(x, c) => x.cancelHandler(c.event)}"
+            @close="${x => x.closeHandler()}"
             aria-labelledby="header"
         >
             <header id="header">

packages/nimble-components/src/dialog/tests/dialog.spec.ts

@@ -1,7 +1,8 @@
 import { html } from '@microsoft/fast-element';
 import { fixture, Fixture } from '../../utilities/tests/fixture';
-import { Dialog, dialogTag, ExtendedDialog, UserDismissed } from '..';
+import { Dialog, dialogTag, UserDismissed } from '..';
 import { waitForUpdatesAsync } from '../../testing/async-helpers';
+import type { ExtendedDialog } from '../../dialog-base';
 
 // eslint-disable-next-line @typescript-eslint/no-invalid-void-type
 async function setup<CloseReason = void>(
@@ -173,6 +174,22 @@ describe('Dialog', () => {
         await disconnect();
     });
 
+    // This can potentially happen if the dialog is implemented with the CloseWatcher API
+    it('should resolve promise with UserDismissed when only close event fired', async () => {
+        const { element, connect, disconnect } = await setup();
+        await connect();
+        const dialogPromise = element.show();
+        await waitForUpdatesAsync();
+        // Simulate user dismiss events in browser
+        nativeDialogElement(element).dispatchEvent(new Event('close'));
+        await waitForUpdatesAsync();
+
+        await expectAsync(dialogPromise).toBeResolvedTo(UserDismissed);
+        expect(element.open).toBeFalse();
+
+        await disconnect();
+    });
+
     it('should dismiss an attempted cancel event when prevent-dismiss is enabled', async () => {
         const { element, connect, disconnect } = await setup(true);
         await connect();