From 70e9234b636b0a2e67a658ecef06008ac8e96fa2 Mon Sep 17 00:00:00 2001 From: m-akinc <7282195+m-akinc@users.noreply.github.com> Date: Thu, 22 Feb 2024 13:23:01 -0600 Subject: [PATCH] Make dialog and drawer more resilient to upcoming CloseWatcher API (#1848) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## ๐Ÿคจ 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 https://github.com/ni/nimble/issues/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 https://github.com/ni/nimble/issues/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](https://github.com/whatwg/html/issues/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 - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. --- ...-697d280f-9413-48ef-b2a4-0704abc5584c.json | 7 +++++ .../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 | 27 +++++++++++++++-- .../nimble-components/src/drawer/template.ts | 1 + .../src/drawer/tests/drawer.spec.ts | 9 ++++++ 8 files changed, 91 insertions(+), 7 deletions(-) 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" +} diff --git a/packages/nimble-components/src/dialog/index.ts b/packages/nimble-components/src/dialog/index.ts index 44398168c6..4b89ce1189 100644 --- a/packages/nimble-components/src/dialog/index.ts +++ b/packages/nimble-components/src/dialog/index.ts @@ -107,8 +107,7 @@ export class Dialog 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 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 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` role="dialog" part="control" @cancel="${(x, c) => x.cancelHandler(c.event)}" + @close="${x => x.closeHandler()}" aria-labelledby="header" >