oddbird / popover-polyfill

Polyfills the HTML popover attribute and showPopover/hidePopover/togglePopover methods onto HTMLElement, as well as the popovertarget and popovertargetaction attributes on <button> elements.
https://popover.oddbird.net
BSD 3-Clause "New" or "Revised" License
269 stars 14 forks source link

Allow synthetic click events to target popovers #170

Closed keithamus closed 10 months ago

keithamus commented 10 months ago

Description

In https://github.com/philc/vimium/issues/4369 it has been noted that popovers don't work with the Firefox "Vimium" extension. This is, I suspect, because Vimium is synthesising click events and the popover polyfill checks if the event isTrusted (coming from the browser and not from scripts).

Trying to reproduce this in https://codepen.io/keithamus/pen/MWxJxOR we can see that the polyfill - when used - renders the button inert due to this guard, however native popover does not have the same guard and so in browsers with native popover (Chrome, WebKit) it is not inert.

Consequently we should remove this guard to ensure behaviour aligns with native.

Related Issue(s)

https://github.com/philc/vimium/issues/4369

Steps to test/reproduce

  1. Open Firefox
  2. Ensure dom.element.popover.enabled is set to false
  3. Open https://codepen.io/keithamus/pen/MWxJxOR
  4. Click the button
  5. EXPECTED: popover shows
  6. ACTUAL: popover does not show.

Show me

Provide screenshots/animated gifs/videos if necessary.

netlify[bot] commented 10 months ago

Deploy Preview for popover-polyfill ready!

Name Link
Latest commit 8b70eafe75df29939a62d2c34ba331a1eb7a00e3
Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/65a668ba02a45400085e6592
Deploy Preview https://deploy-preview-170--popover-polyfill.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

keithamus commented 10 months ago

Do you think we want a similar change here?

I tried to use synthetic events to close an open popover and it seems as though they get ignored, so my guess for now is that we do not. If we can reproduce a discrepancy then I'd suggest we might want to, but until then I'd like to not remove that line.

jgerigmeyer commented 10 months ago

Released in v0.3.8.