Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make dialog and drawer more resilient to upcoming CloseWatcher API #1848

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Feb 20, 2024

Pull Request

🤨 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). 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.

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 with the CloseWatcher API enabled.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job providing rationale and comments for all the subtle behaviors in this PR!

@rajsite
Copy link
Member

rajsite commented Feb 21, 2024

Sorry, but last time adding @mollykreis as they are very close to the implementation and aware of the bug. Should be a quick double check.

packages/nimble-components/src/dialog/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/dialog/index.ts Outdated Show resolved Hide resolved
@m-akinc m-akinc enabled auto-merge (squash) February 22, 2024 02:59
@m-akinc m-akinc merged commit 70e9234 into main Feb 22, 2024
9 of 10 checks passed
@m-akinc m-akinc deleted the users/makinc/dialog-handle-skipped-cancel-event branch February 22, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nimble-dialog does not have a styled focus-visible state
4 participants