-
Notifications
You must be signed in to change notification settings - Fork 599
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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": "luke@warlow.dev", | ||
"dependentChangeType": "patch" | ||
} |
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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,6 +89,8 @@ export class FASTTooltip extends FASTElement { | |
@observable | ||
private controlledVisibility: boolean = false; | ||
|
||
private closeWatcher: CloseWatcher | null = null; | ||
|
||
/** | ||
* Switches between controlled and uncontrolled visibility. | ||
* | ||
|
@@ -311,7 +313,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any issues with having both a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -337,6 +341,7 @@ export class FASTTooltip extends FASTElement { | |
public hideTooltip(): void { | ||
this._visible = false; | ||
this.cleanup?.(); | ||
this.closeWatcher?.destroy(); | ||
} | ||
|
||
/** | ||
|
@@ -446,5 +451,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(); | ||
} | ||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah good catch