From a2872576cbc0c7085956e4f3af0757e53b92f147 Mon Sep 17 00:00:00 2001
From: Mert Akinc <7282195+m-akinc@users.noreply.github.com>
Date: Tue, 20 Feb 2024 17:20:36 -0600
Subject: [PATCH 1/3] Clean up dialog/drawer properly if cancel event skipped

---
 .../nimble-components/src/dialog/index.ts     | 29 ++++++++++++++++---
 .../nimble-components/src/dialog/styles.ts    |  8 ++++-
 .../nimble-components/src/dialog/template.ts  |  1 +
 .../src/dialog/tests/dialog.spec.ts           | 16 ++++++++++
 .../nimble-components/src/drawer/index.ts     | 26 +++++++++++++++--
 .../nimble-components/src/drawer/template.ts  |  1 +
 .../src/drawer/tests/drawer.spec.ts           |  9 ++++++
 7 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/packages/nimble-components/src/dialog/index.ts b/packages/nimble-components/src/dialog/index.ts
index 44398168c6..26db4d0ddc 100644
--- a/packages/nimble-components/src/dialog/index.ts
+++ b/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.notifyClosed(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.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);
+        }
+    }
+
+    private 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;
+    }
 }
 
 // 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..5d0b1c3c1c 100644
--- a/packages/nimble-components/src/dialog/styles.ts
+++ b/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;
diff --git a/packages/nimble-components/src/dialog/template.ts b/packages/nimble-components/src/dialog/template.ts
index d27899a530..23f2b63a57 100644
--- a/packages/nimble-components/src/dialog/template.ts
+++ b/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">
diff --git a/packages/nimble-components/src/dialog/tests/dialog.spec.ts b/packages/nimble-components/src/dialog/tests/dialog.spec.ts
index 1c740f4daf..99ab75c853 100644
--- a/packages/nimble-components/src/dialog/tests/dialog.spec.ts
+++ b/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();
diff --git a/packages/nimble-components/src/drawer/index.ts b/packages/nimble-components/src/drawer/index.ts
index c4cdaa850d..29fc5c8348 100644
--- a/packages/nimble-components/src/drawer/index.ts
+++ b/packages/nimble-components/src/drawer/index.ts
@@ -92,6 +92,29 @@ 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 and the drawer just closes.
+            this.notifyClosed(UserDismissed);
+        }
+    }
+
+    private 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;
+    }
+
     private readonly animationEndHandlerFunction = (): void => this.animationEndHandler();
 
     private openDialog(): void {
@@ -126,8 +149,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.notifyClosed(this.closeReason);
         }
     }
 }
diff --git a/packages/nimble-components/src/drawer/template.ts b/packages/nimble-components/src/drawer/template.ts
index 11a269642c..80d0b3168c 100644
--- a/packages/nimble-components/src/drawer/template.ts
+++ b/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>
diff --git a/packages/nimble-components/src/drawer/tests/drawer.spec.ts b/packages/nimble-components/src/drawer/tests/drawer.spec.ts
index f12de2eba3..f195ad89d1 100644
--- a/packages/nimble-components/src/drawer/tests/drawer.spec.ts
+++ b/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();

From 54633982bf5ef0bfd6edaccb8847520f4fdce907 Mon Sep 17 00:00:00 2001
From: Mert Akinc <7282195+m-akinc@users.noreply.github.com>
Date: Tue, 20 Feb 2024 17:21:02 -0600
Subject: [PATCH 2/3] Change files

---
 ...le-components-697d280f-9413-48ef-b2a4-0704abc5584c.json | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 change/@ni-nimble-components-697d280f-9413-48ef-b2a4-0704abc5584c.json

diff --git a/change/@ni-nimble-components-697d280f-9413-48ef-b2a4-0704abc5584c.json b/change/@ni-nimble-components-697d280f-9413-48ef-b2a4-0704abc5584c.json
new file mode 100644
index 0000000000..38f18c98c5
--- /dev/null
+++ b/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"
+}

From 1f43bdd9dea5fecf8c7a353218d1726c06f28713 Mon Sep 17 00:00:00 2001
From: Mert Akinc <7282195+m-akinc@users.noreply.github.com>
Date: Wed, 21 Feb 2024 20:58:25 -0600
Subject: [PATCH 3/3] Feedback

---
 packages/nimble-components/src/dialog/index.ts | 12 ++++++------
 packages/nimble-components/src/drawer/index.ts | 11 ++++++-----
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/packages/nimble-components/src/dialog/index.ts b/packages/nimble-components/src/dialog/index.ts
index 26db4d0ddc..4b89ce1189 100644
--- a/packages/nimble-components/src/dialog/index.ts
+++ b/packages/nimble-components/src/dialog/index.ts
@@ -107,7 +107,7 @@ export class Dialog<CloseReason = void> extends FoundationElement {
             throw new Error('Dialog is not open');
         }
         this.dialogElement.close();
-        this.notifyClosed(reason);
+        this.doResolveShow(reason);
     }
 
     public slottedFooterElementsChanged(
@@ -124,7 +124,7 @@ export class Dialog<CloseReason = void> extends FoundationElement {
         if (this.preventDismiss) {
             event.preventDefault();
         } else {
-            this.notifyClosed(UserDismissed);
+            this.doResolveShow(UserDismissed);
         }
         return true;
     }
@@ -137,15 +137,15 @@ export class Dialog<CloseReason = void> extends FoundationElement {
             // 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);
+            // the cancel event is not fired, but the close event still is, and the dialog just closes.
+            this.doResolveShow(UserDismissed);
         }
     }
 
-    private notifyClosed(reason: CloseReason | UserDismissed): void {
+    private doResolveShow(reason: CloseReason | UserDismissed): void {
         if (!this.resolveShow) {
             throw new Error(
-                'Do not call notifyClosed unless there is a promise to resolve'
+                'Do not call doResolveShow unless there is a promise to resolve'
             );
         }
         this.resolveShow(reason);
diff --git a/packages/nimble-components/src/drawer/index.ts b/packages/nimble-components/src/drawer/index.ts
index 29fc5c8348..387201761f 100644
--- a/packages/nimble-components/src/drawer/index.ts
+++ b/packages/nimble-components/src/drawer/index.ts
@@ -100,15 +100,16 @@ export class Drawer<CloseReason = void> extends FoundationElement {
             // 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 and the drawer just closes.
-            this.notifyClosed(UserDismissed);
+            // 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 notifyClosed(reason: CloseReason | UserDismissed): void {
+    private doResolveShow(reason: CloseReason | UserDismissed): void {
         if (!this.resolveShow) {
             throw new Error(
-                'Do not call notifyClosed unless there is a promise to resolve'
+                'Do not call doResolveShow unless there is a promise to resolve'
             );
         }
         this.resolveShow(reason);
@@ -149,7 +150,7 @@ export class Drawer<CloseReason = void> extends FoundationElement {
             this.dialog.classList.remove('closing');
             this.dialog.close();
             this.closing = false;
-            this.notifyClosed(this.closeReason);
+            this.doResolveShow(this.closeReason);
         }
     }
 }