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

Use close watcher when supported in place of escape key handlers #6877

Closed
wants to merge 4 commits into from

Conversation

lukewarlow
Copy link

@lukewarlow lukewarlow commented Dec 13, 2023

Pull Request

📖 Description

This PR uses the CloseWatcher API [1][2] in place of escape key listeners where it's supported. This allows components such as dialog to be dismissed using extra close signals (e.g. Android back gesture)

[1] https://html.spec.whatwg.org/multipage/interaction.html#the-closewatcher-interface
[2] https://developer.chrome.com/blog/new-in-chrome-120#close-watcher

🎫 Issues

#6878

👩‍💻 Reviewer Notes

Need to test all changed components to ensure they correctly dismiss when escape key is pressed both in Chrome and a non Chromium browser (e.g. Safari or Firefox).

Additionally can test these components on Android and see the back gesture now correctly dismisses rather than unexpected navigating.

📑 Test Plan

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

@lukewarlow
Copy link
Author

@microsoft-github-policy-service agree

@lukewarlow lukewarlow marked this pull request as ready for review December 13, 2023 20:13
@KingOfTac
Copy link
Collaborator

Thanks for the PR and bringing this awesome new platform feature to my attention! I do have a couple of questions though.

  1. What would be the differences in behavior in supporting and non-supporting browsers, if any?
  2. Does this change fix any critical issues and if not, would we be better off waiting on this change until browser support has gotten better and we can avoid branching logic based on feature support?

@lukewarlow
Copy link
Author

  1. Other than in some edge case scenarios where close watchers can end up stacking where the escape key listeners might not there should be no difference.

  2. It's arguable how important this is. An alternative to the branching logic would be a polyfill that internalised the escape key listening.

One benefit to supporting this early is that you get Android back gesture support (including Talkback), you may also get support for other assistive technologies (haven't tested others).

If this isn't supported then there will be a UX difference between the native popover and dialogs and the custom ones which may lead to user frustration.

@@ -558,6 +560,9 @@ export class FASTPicker extends FormAssociatedPicker {

if (open && this.getRootActiveElement() === this.inputElement) {
this.flyoutOpen = open;
this.closeWatcher?.destroy();
this.closeWatcher = new CloseWatcher();
Copy link

Choose a reason for hiding this comment

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

do you need to wrap this construction and the one in select in 'CloseWatcher' in window checks?

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah good catch

Comment on lines +316 to +318
if (!("CloseWatcher" in window)) {
document.addEventListener("keydown", this.keydownDocumentHandler);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are there any issues with having both a closeWatcher and a keydown event handler? How might the two methods collide?

Copy link
Author

Choose a reason for hiding this comment

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

I'm unaware of what the result would be if you had both. I imagine it'd be okay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Worse case I see is that we run the "close the thing" function twice which shouldn't break regardless, but I assume closeWatcher marks the event with preventDefault or stops propagation so we can avoid acting twice?

@janechu
Copy link
Collaborator

janechu commented Jun 10, 2024

Closing this, due to #6951 we are only putting in fixes or critical features.

@janechu janechu closed this Jun 10, 2024
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.

6 participants