Make dialog and drawer more resilient to upcoming CloseWatcher API (#…

…1848)

## 🤨 Rationale

There is a change to the HTML spec introducing the `CloseWatcher` API,
which affects the way the native `dialog` element behaves. Recently,
Chrome had released their implementation, and it broke our drawer and
dialog (see issue #1713, as well as
resulting [Chromium
issue](https://bugs.chromium.org/p/chromium/issues/detail?id=1512224)).
Though they rolled back their release of that feature (by moving it back
to experimental), it will eventually be implemented in Chrome and other
browsers in some form. We want to at least guard against leaving the
drawer/dialog in a bad state.

Also fixes #1445

## 👩‍💻 Implementation

The issue is that the `cancel` event is intentionally not fired in a
couple cases where the `dialog` is dismissed with ESC.
- One such case was when the there is no user interaction (e.g.
clicking, scrolling) in the `dialog` before pressing ESC. Apparently,
this is [viewed as a mistake and will likely be
changed](whatwg/html#10046).
- Another case that is [more fundamental to the `CloseWatcher`
API](https://github.com/WICG/close-watcher#abuse-analysis) is when there
are two ESC presses without an intervening user interaction.

I have added a `close` event handler in both the drawer and dialog that
resolves and clears the promise, if there is one still set. This ensures
we are not left in a bad state where the drawer/dialog can't be
reopened. Note there are still problems:
- the drawer closes without animation (because we rely on the `cancel`
event to trigger the animation)
- the drawer/dialog can be closed with ESC even if `prevent-dismiss` is
set

Depending on the ultimate `CloseWatcher` specification, we may decide to
deprecate the `prevent-dismiss` option.

OTHER CHANGES:
- Gave the dialog proper focus styling, rather than the default.

## 🧪 Testing

I added tests for the specific case now being handled, though these
won't currently be running against any browser versions that would
exhibit the problem behavior.

I did manual testing using a [version of
Chromium](https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Win_x64/1241130/)
with the `CloseWatcher` API enabled.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.

change/@ni-nimble-components-697d280f-9413-48ef-b2a4-0704abc5584c.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Clean up dialog/drawer properly if cancel event skipped",
+  "packageName": "@ni/nimble-components",
+  "email": "7282195+m-akinc@users.noreply.github.com",
+  "dependentChangeType": "patch"
+}

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

@@ -107,8 +107,7 @@ export class Dialog<CloseReason = void> extends FoundationElement {
             throw new Error('Dialog is not open');
         }
         this.dialogElement.close();
-        this.resolveShow!(reason);
-        this.resolveShow = undefined;
+        this.doResolveShow(reason);
     }
 
     public slottedFooterElementsChanged(
@@ -125,11 +124,33 @@ export class Dialog<CloseReason = void> extends FoundationElement {
         if (this.preventDismiss) {
             event.preventDefault();
         } else {
-            this.resolveShow!(UserDismissed);
-            this.resolveShow = undefined;
+            this.doResolveShow(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, but the close event still is, and the dialog just closes.
+            this.doResolveShow(UserDismissed);
+        }
+    }
+
+    private doResolveShow(reason: CloseReason | UserDismissed): void {
+        if (!this.resolveShow) {
+            throw new Error(
+                'Do not call doResolveShow unless there is a promise to resolve'
+            );
+        }
+        this.resolveShow(reason);
+        this.resolveShow = undefined;
+    }
 }
 
 // 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

@@ -173,6 +173,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();

packages/nimble-components/src/drawer/index.ts

@@ -92,6 +92,30 @@ export class Drawer<CloseReason = void> extends FoundationElement {
         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 drawer (e.g. clicking, scrolling),
+            // the cancel event is not fired, but the close event still is, and the drawer just closes.
+            // The animation is never started, so there is no animation end listener to clean up.
+            this.doResolveShow(UserDismissed);
+        }
+    }
+
+    private doResolveShow(reason: CloseReason | UserDismissed): void {
+        if (!this.resolveShow) {
+            throw new Error(
+                'Do not call doResolveShow unless there is a promise to resolve'
+            );
+        }
+        this.resolveShow(reason);
+        this.resolveShow = undefined;
+    }
+
     private readonly animationEndHandlerFunction = (): void => this.animationEndHandler();
 
     private openDialog(): void {
@@ -126,8 +150,7 @@ export class Drawer<CloseReason = void> extends FoundationElement {
             this.dialog.classList.remove('closing');
             this.dialog.close();
             this.closing = false;
-            this.resolveShow!(this.closeReason);
-            this.resolveShow = undefined;
+            this.doResolveShow(this.closeReason);
         }
     }
 }

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

@@ -6,6 +6,7 @@ export const template = html<Drawer>`
         ${ref('dialog')}
         aria-label="${x => x.ariaLabel}"
         @cancel="${(x, c) => x.cancelHandler(c.event)}"
+        @close="${x => x.closeHandler()}"
     >
         <div class="dialog-contents">
             <slot></slot>

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

@@ -161,6 +161,15 @@ describe('Drawer', () => {
             expect(element.open).toBeFalse();
         });
 
+        // 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 promise = element.show();
+            // Simulate user dismiss events in browser
+            nativeDialogElement(element).dispatchEvent(new Event('close'));
+            await expectAsync(promise).toBeResolvedTo(UserDismissed);
+            expect(element.open).toBeFalse();
+        });
+
         it('throws calling show() a second time', async () => {
             void element.show();
             await expectAsync(element.show()).toBeRejectedWithError();