Create a type for this that can be used.

Comments...

I don't love this but in the case of toggling the popover, it seems like we need to force it closed if it is open.

Yeah I got the same issue with Dropdown, if the invoker element is clicked, first the popover element would close since a click event happened outside of it, then the click event fires on the invoker, which is too late to prevent any default behavior. +1 to this solution.

Is there any reason we can't use this.popoverTargetElement?.matches(":popover-open")?

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 setPopoverTargetElement.

https://developer.mozilla.org/en-US/docs/Web/API/Popover_API/Using#:~:text=%7B%0A%20%20%20%20if%20(-,popover.matches(%22%3Apopover%2Dopen%22),-)%20%7B%0A%20%20%20%20%20%20popover

Oh, I like this way better.

@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 .match(':popover-open') is false, if it only checks :popover-open, the button click will open the popover again. This is handled nicely by browser if the invoker is a <button> or <input>, but we have to do it for other elements.

Yeah, unfortunately this wasn't working for me when I tried to implement it.

I'm open to alternative approaches here.

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 popover=manual would close the popover when the invoker is clicked again, but we have to manage to close them manually.

@chrisdholt I think your solution here works, only thing is you need to set this.popoverOpen after togglePopover(), so:

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 this.popoverOpen inside a beforetoggle event, which your code currently doesn't.

🕵🏾‍♀️ visual regressions to review in the fluentui-web-components-v3 Visual Regression Report