-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(web-components): add support for popovertarget
and popovertargetaction
to button
#32397
base: master
Are you sure you want to change the base?
Changes from all commits
993b76c
be3a20d
4e43b65
9bcb26c
96f3ac7
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": "prerelease", | ||
"comment": "add support for popovertarget and popovertargetaction in button web component", | ||
"packageName": "@fluentui/web-components", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,13 @@ import { ButtonType } from './button.options.js'; | |
* @public | ||
*/ | ||
export class BaseButton extends FASTElement { | ||
private popoverOpen?: boolean = false; | ||
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. Comments... |
||
|
||
/** | ||
* Holds an element reference for the popovertarget | ||
*/ | ||
private popoverTargetElement: HTMLElement | null | undefined; | ||
|
||
/** | ||
* Indicates the button should be focused when the page is loaded. | ||
* @see The {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#autofocus | `autofocus`} attribute | ||
|
@@ -29,6 +36,28 @@ export class BaseButton extends FASTElement { | |
@attr({ mode: 'boolean' }) | ||
public autofocus!: boolean; | ||
|
||
/** | ||
* The ID for the popover which is controlled by the element | ||
* @see The {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#popovertarget | `popovertarget`} attribute | ||
* | ||
* @public | ||
* @remarks | ||
* HTML Attribute: `popovertarget` | ||
*/ | ||
@attr | ||
public popovertarget?: string; | ||
|
||
/** | ||
* Specifies the action to be performed on a popover element being controlled by the element | ||
* @see The {@link https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#popovertargetaction | `popovertargetaction`} attribute | ||
* | ||
* @public | ||
* @remarks | ||
* HTML Attribute: `popovertargetaction` | ||
*/ | ||
@attr | ||
public popovertargetaction?: 'hide' | 'show' | 'toggle'; | ||
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. Create a type for this that can be used. |
||
|
||
/** | ||
* Default slotted content. | ||
* | ||
|
@@ -68,6 +97,17 @@ export class BaseButton extends FASTElement { | |
@attr({ attribute: 'tabindex', mode: 'fromView', converter: nullableNumberConverter }) | ||
public override tabIndex: number = 0; | ||
|
||
/** | ||
* Handles changes to the popovertarget attribute | ||
* | ||
* @param previous - the previous popovertarget value | ||
* @param next - the current popovertarget value | ||
* @internal | ||
*/ | ||
public popoverTargetChanged(previous: string | undefined, next: string | undefined): void { | ||
this.setPopoverTargetElement(next); | ||
} | ||
|
||
/** | ||
* Sets the element's internal disabled state when the element is focusable while disabled. | ||
* | ||
|
@@ -254,20 +294,64 @@ export class BaseButton extends FASTElement { | |
return; | ||
} | ||
|
||
if (this.popoverTargetElement && !!this.popoverTargetElement.hasAttribute('popover')) { | ||
switch (this.popovertargetaction) { | ||
case 'hide': | ||
// @ts-expect-error - Baseline 2024 | ||
this.popoverTargetElement?.hidePopover(); | ||
break; | ||
case 'show': | ||
// @ts-expect-error - Baseline 2024 | ||
this.popoverTargetElement?.showPopover(); | ||
break; | ||
case 'toggle': | ||
default: | ||
// @ts-expect-error - Baseline 2024 | ||
this.popoverTargetElement?.togglePopover(!this.popoverOpen); | ||
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. Is there any reason we can't use I don't know if we want to be in the business of mirroring state here when the browser gives us a pseudo-state for free. We could also wouldn't need the eventListener in 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. Oh, I like this way better. 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. @davatron5000 I think this is to cover the case that, when the popover is open, if you click the button again, it won't close the popover. Because when you click the button, the popover closes (click event outside of the popover element), and 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. Yeah, unfortunately this wasn't working for me when I tried to implement it. I'm open to alternative approaches here. 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 went into a bit of rabbit before the end of day yesterday (because I was trying to fix this issue for Dropdown as well), and created this codepen: https://codepen.io/marchbox/pen/ExBOVvE. The event order for an artificial popover invoker (non-button/input) is a bit odd, but it's consistent across Edge, Firefox, and Safari. I haven't figure out what would be the best to mitigate this issue though. Using 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. @chrisdholt I think your solution here works, only thing is you need to set if (this.popoverTargetElement && !!this.popoverTargetElement.hasAttribute('popover')) {
const nextPopoverOpen = !this.popoverOpen;
switch (this.popovertargetaction) {
case 'hide':
this.popoverTargetElement?.hidePopover();
this.popoverOpen = false;
break;
case 'show':
this.popoverTargetElement?.showPopover();
this.popoverOpen = true;
break;
default:
const nextPopoverOpen = !this.popoverOpen;
this.popoverTargetElement?.togglePopover(nextPopoverOpen);
this.popoverOpen = nextPopoverOpen;
}
} The key is NOT to update |
||
break; | ||
} | ||
} | ||
|
||
this.press(); | ||
return true; | ||
} | ||
|
||
connectedCallback(): void { | ||
public connectedCallback(): void { | ||
super.connectedCallback(); | ||
this.elementInternals.ariaDisabled = `${!!this.disabledFocusable}`; | ||
|
||
if (this.popovertarget) { | ||
this.setPopoverTargetElement(this.popovertarget); | ||
} | ||
} | ||
|
||
public disconnectedCallback(): void { | ||
super.disconnectedCallback(); | ||
|
||
if (this.popoverTargetElement) { | ||
this.popoverTargetElement.removeEventListener('toggle', this.handlePopover); | ||
} | ||
} | ||
|
||
constructor() { | ||
super(); | ||
this.elementInternals.role = 'button'; | ||
} | ||
|
||
/** | ||
* Sets up handling for the popover target element | ||
* @param element - The element to reference | ||
*/ | ||
private setPopoverTargetElement(element: string | undefined) { | ||
this.popoverTargetElement = element ? document.getElementById(element) : undefined; | ||
|
||
this.popoverTargetElement?.addEventListener('toggle', this.handlePopover); | ||
} | ||
|
||
private handlePopover = (e: Event) => { | ||
this.popoverOpen = (e as Event & { newState: 'open' | 'closed'; oldState: 'open' | 'closed' }).newState === 'open'; | ||
}; | ||
|
||
/** | ||
* This fallback creates a new slot, then creates a submit button to mirror the custom element's | ||
* properties. The submit button is then appended to the slot and the form is submitted. | ||
|
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.
🕵🏾♀️ visual regressions to review in the fluentui-web-components-v3 Visual Regression Report
Avatar 1 screenshots