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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8158b15
Use close watcher when supported in place of escape key handlers
lukewarlow Dec 13, 2023
5721aff
Change files
lukewarlow Dec 13, 2023
3987d78
Address nit
lukewarlow Jan 2, 2024
33e5fd6
Merge branch 'master' into close-watcher
lukewarlow Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@microsoft-fast-foundation-82cb2686-8df8-4bdc-a688-f0dab9cb1651.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Use close watcher when supported in place of escape key handlers",
"packageName": "@microsoft/fast-foundation",
"email": "[email protected]",
"dependentChangeType": "patch"
}
19 changes: 19 additions & 0 deletions packages/web-components/fast-foundation/src/close-watcher.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
interface CloseWatcher extends EventTarget {
new (options?: CloseWatcherOptions): CloseWatcher;
requestClose(): void;
close(): void;
destroy(): void;

oncancel: (event: Event) => any | null;
onclose: (event: Event) => any | null;
}

declare const CloseWatcher: CloseWatcher;

interface CloseWatcherOptions {
signal: AbortSignal;
}

declare interface Window {
CloseWatcher?: CloseWatcher;
}
18 changes: 16 additions & 2 deletions packages/web-components/fast-foundation/src/dialog/dialog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ export class FASTDialog extends FASTElement {
*/
private notifier: Notifier;

/**
* @internal
lukewarlow marked this conversation as resolved.
Show resolved Hide resolved
*/
private closeWatcher: CloseWatcher | null = null;

/**
* @internal
*/
Expand All @@ -119,6 +124,11 @@ export class FASTDialog extends FASTElement {
*/
public show(): void {
this.hidden = false;
if ("CloseWatcher" in window) {
this.closeWatcher?.destroy();
this.closeWatcher = new CloseWatcher();
this.closeWatcher.onclose = () => this.hide();
}
}

/**
Expand All @@ -128,6 +138,7 @@ export class FASTDialog extends FASTElement {
*/
public hide(): void {
this.hidden = true;
this.closeWatcher?.destroy();
// implement `<dialog>` interface
this.$emit("close");
}
Expand All @@ -152,6 +163,7 @@ export class FASTDialog extends FASTElement {

// remove keydown event listener
document.removeEventListener("keydown", this.handleDocumentKeydown);
this.closeWatcher?.destroy();

// if we are trapping focus remove the focusin listener
this.updateTrapFocus(false);
Expand All @@ -176,8 +188,10 @@ export class FASTDialog extends FASTElement {
if (!e.defaultPrevented && !this.hidden) {
switch (e.key) {
case keyEscape:
this.dismiss();
e.preventDefault();
if (!this.closeWatcher) {
this.dismiss();
e.preventDefault();
}
break;

case keyTab:
Expand Down
9 changes: 9 additions & 0 deletions packages/web-components/fast-foundation/src/picker/picker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,8 @@ export class FASTPicker extends FormAssociatedPicker {
private inputElementView: HTMLView | null = null;
private behaviorOrchestrator: ViewBehaviorOrchestrator | null = null;

private closeWatcher: CloseWatcher | null = null;

/**
* @internal
*/
Expand Down Expand Up @@ -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

this.closeWatcher.onclose = () => this.toggleFlyout(false);
Updates.enqueue(() => {
if (this.menuElement !== undefined) {
this.setFocusedOption(0);
Expand All @@ -570,6 +575,7 @@ export class FASTPicker extends FormAssociatedPicker {

this.flyoutOpen = false;
this.disableMenu();
this.closeWatcher?.destroy();
return;
}

Expand Down Expand Up @@ -660,6 +666,9 @@ export class FASTPicker extends FormAssociatedPicker {
}

case keyEscape: {
if (this.closeWatcher) {
return true;
}
this.toggleFlyout(false);
return false;
}
Expand Down
10 changes: 9 additions & 1 deletion packages/web-components/fast-foundation/src/select/select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ export class FASTSelect extends FormAssociatedSelect {
@attr({ attribute: "open", mode: "boolean" })
public open: boolean = false;

private closeWatcher: CloseWatcher | null = null;

/**
* Sets focus and synchronizes ARIA attributes when the open property changes.
*
Expand All @@ -78,10 +80,16 @@ export class FASTSelect extends FormAssociatedSelect {
this.focusAndScrollOptionIntoView();
this.indexWhenOpened = this.selectedIndex;

this.closeWatcher?.destroy();
this.closeWatcher = new CloseWatcher();
this.closeWatcher.onclose = () => (this.open = false);

// focus is directed to the element when `open` is changed programmatically
Updates.enqueue(() => this.focus());

return;
} else {
this.closeWatcher?.destroy();
}

this.cleanup?.();
Expand Down Expand Up @@ -523,7 +531,7 @@ export class FASTSelect extends FormAssociatedSelect {
}

case keyEscape: {
if (this.collapsible && this.open) {
if (this.collapsible && this.open && !this.closeWatcher) {
e.preventDefault();
this.open = false;
}
Expand Down
13 changes: 12 additions & 1 deletion packages/web-components/fast-foundation/src/tooltip/tooltip.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ export class FASTTooltip extends FASTElement {
@observable
private controlledVisibility: boolean = false;

/**
* @internal
*/
private closeWatcher: CloseWatcher | null = null;

/**
* Switches between controlled and uncontrolled visibility.
*
Expand Down Expand Up @@ -311,7 +316,9 @@ export class FASTTooltip extends FASTElement {
this.addEventListener("mouseout", this.mouseoutAnchorHandler);
this.addEventListener("mouseover", this.mouseoverAnchorHandler);

document.addEventListener("keydown", this.keydownDocumentHandler);
if (!("CloseWatcher" in window)) {
document.addEventListener("keydown", this.keydownDocumentHandler);
}
Comment on lines +316 to +318
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?

}

public connectedCallback(): void {
Expand All @@ -337,6 +344,7 @@ export class FASTTooltip extends FASTElement {
public hideTooltip(): void {
this._visible = false;
this.cleanup?.();
this.closeWatcher?.destroy();
}

/**
Expand Down Expand Up @@ -446,5 +454,8 @@ export class FASTTooltip extends FASTElement {
private showTooltip(): void {
this._visible = true;
Updates.enqueue(() => this.setPositioning());
this.closeWatcher?.destroy();
this.closeWatcher = new CloseWatcher();
this.closeWatcher.onclose = () => this.dismiss();
}
}
Loading