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.
BSD 3-Clause "New" or "Revised" License
247 stars 14 forks source link

Polyfill is broken when used with nested popovers and inside shadow DOM #183

Closed jpzwarte closed 5 months ago

jpzwarte commented 6 months ago
if (element.popover === "auto") {
    const originalType = element.getAttribute("popover");
    const ancestor = topMostPopoverAncestor(element) || document2;
    hideAllPopoversUntil(ancestor, false, true);
    if (originalType !== element.getAttribute("popover") || !checkPopoverValidity(element, false)) {
      return;
    }
  }

The above snippet is part of the showPopover method in the polyfill (https://github.com/oddbird/popover-polyfill/blob/main/src/popover-helpers.ts#L268). When this is called inside a nested popover inside the shadow DOM, hideAllPopoversUntil incorrectly hides the parent popover, which doesn't happen in browsers that already support popover.

jpzwarte commented 6 months ago

The bug is that ancestor has the value of document instead of the parent popover. To actually get to the parent popover from the new popover, I have to do this in the devtools (with the new popover selected): $0.parentElement.assignedSlot.closest('[popover]'). My guess is that the polyfill doesn't handle this properly?

jpzwarte commented 6 months ago

This is the DOM structure CleanShot 2024-02-22 at 15 01 08

jgerigmeyer commented 6 months ago

@jpzwarte Thanks for reporting! Could you provide the markup for a minimal example to reproduce? I'd like to try adding it to the demo page first, so I can be sure I'm seeing what you're seeing.

ddegan commented 5 months ago

Hi! We are facing the same issue, since we're trying to include the polyfills for our Web Components. Here a small example that reproduce the issue.

Thanks!

jgerigmeyer commented 5 months ago

@ddegan It looks like your case is not the same as the one described in this issue. The polyfill is running correctly in your code, but there's a race condition where the defer attribute on the <script> tag that loads the polyfill causes the polyfill to run after attachShadow is called. The polyfill still "works", but this means the polyfill styles aren't automatically loaded in the shadow root. Either removing defer from the polyfill or adding type="module" to your scripts.js script tag fixes it. Here's a forked version that removes defer.

jpzwarte commented 5 months ago

Thanks @jgerigmeyer !

jgerigmeyer commented 5 months ago

Included in v0.4.1 release.