Skip to content
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

Any interaction closes popover on Firefox #2925

Closed
imagoiq opened this issue Apr 11, 2024 · 2 comments · Fixed by #3211
Closed

Any interaction closes popover on Firefox #2925

imagoiq opened this issue Apr 11, 2024 · 2 comments · Fixed by #3211
Assignees
Labels
🐞 bug Something isn't working 📦 components Related to the @swisspost/design-system-components package

Comments

@imagoiq
Copy link
Contributor

imagoiq commented Apr 11, 2024

When clicking or selecting text inside a popover, it closes immediately. Only on Firefox.

https://design-system.post.ch/?path=/docs/9a636763-de2d-4f72-bc81-98daf10871f7--docs

@gfellerph gfellerph added 🐞 bug Something isn't working 📦 components Related to the @swisspost/design-system-components package labels Apr 12, 2024
@imagoiq imagoiq assigned imagoiq and unassigned imagoiq Apr 15, 2024
@imagoiq
Copy link
Contributor Author

imagoiq commented Apr 15, 2024

I had a look, but I cannot figure a fix or even a workaround.

The issue is inside the lightDismissOpenPopovers function in the popover-helper.ts file of the polyfill.

The function looks for an ancestor, but there is no ancestor when we click on the <p> for example. And when there is no ancestor, as you can see in this screenshot, all popovers are hidden (with hideAllPopoversUntil.
firefox_rHBuveLvwy

The popover is actually hidden inside the shadow DOM and even if we place the popover interactivity on the host of the post-popovercontainer, the slotted content is inside the post-popover element and the first parentElement (used in nearestInclusiveOpenPopover function) of any slotted node will be post-popover.

So in my opinion, I think we should:

Remove the nested web component container and put the popover interactivity on post-popover. It should as well avoid us using cross-root id which doesn't seem to work very well with the Popover API at the moment (see https://stackoverflow.com/questions/77324143/popovertargetelement-does-not-cross-shadow-boundaries from Philipp, trigger.popoverTargetElement = popover; is not working on Chrome at the moment for some reason. You can see it as well in the polyfill demo where the demo "Click to toggle shadowed Popover (cross-tree)" doesn't work in chrome: https://popover-polyfill.netlify.app/), and also simplify the component as the polyfill if used normally should be able to handle the aria-expanded attribute.

A second review and opinion would be nice!

@gfellerph
Copy link
Member

Not happening on Mac/Firefox 125.0.1, happens on Windows/Firefox 115 and Windows/Firefox 124 (Developer Edition).

@gfellerph gfellerph assigned gfellerph and unassigned gfellerph May 1, 2024
@gfellerph gfellerph assigned gfellerph and unassigned gfellerph Jul 2, 2024
gfellerph added a commit that referenced this issue Jul 3, 2024
Firefox ESR does not support the popover attribute and therefore uses the polyfill. Because the popover was nested in a shadow root, the polyfill did not work correctly (see #2925 (comment)). Not using shadow-dom for the post-popovercontainer fixes the issue.
@gfellerph gfellerph linked a pull request Jul 3, 2024 that will close this issue
@gfellerph gfellerph self-assigned this Jul 3, 2024
gfellerph added a commit that referenced this issue Jul 11, 2024
Firefox ESR does not support the popover attribute and therefore uses
the polyfill. Because the popover was nested in a shadow root, the
polyfill did not work correctly (see
#2925 (comment)).
Not using shadow-dom for the post-popovercontainer fixes the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 components Related to the @swisspost/design-system-components package
Projects
Status: 🚀 Done
3 participants