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

Problems with overwriting popover styling for Firefox #147

Closed anna-lach closed 1 year ago

anna-lach commented 1 year ago

I’m currently working on a popover component and I’d like to overwrite some styling for the [popover] like overflow to visible etc., but in the Firefox it doesn’t work. Since there are styles added in the popover.ts file in the polyfill it looks like I cannot overwrite those styles in my app. It seems those styles from polyfill's popover.ts file have the highest specificity.

jgerigmeyer commented 1 year ago

@keithamus or @mirisuzanne I'm curious if you have suggestions for how to approach this? Seems like from an API perspective we could expose an option to omit the polyfill styles, but maybe there's a better CSS approach to allow overrides.

mirisuzanne commented 1 year ago

There are CSS-first approaches. The most relevant and simplest would be Cascade Layers. They're fairly new, but have good support for the last couple years. I'm not sure what our support matrix looks like here? The other CSS option is wrapping all provided styles in :where() selectors to remove specificity. That has browser support a bit farther back.

In either case, if there are overrides that should not be allowed, those properties can be marked as !important.

keithamus commented 1 year ago

I’ll try a solution using :where tomorrow and see what impacts that has, but it seems like a good solution.

As a workaround for now, @anna-lach, you could increase specificity by, for example, using [popover][popover] (that’s not a typo, use the popover attribute selector twice) in your selectors.

anna-lach commented 1 year ago

Thank you @keithamus. I tried with [popover][popover] but it's still not working properly. So for now maybe I'll mark some properties as !important, because I only need it for a few like border.

jgerigmeyer commented 1 year ago

@anna-lach This is released in v0.3.4.

jpzwarte commented 1 year ago

I'm working with Anna on this. I think this has something to do with the shadow DOM: no matter what selectors I add to the scoped styling within the custom element, I cannot get it to "overwrite" the border-* styling of the polyfill.

@jgerigmeyer the :where([popover]) fix also doesn't help.

So afaics the only way to work around this is to use !important atm.

To clarify, we are doing something like this:

// Doesn't matter if how many selectors you add here, the polyfill styles will always have a higher specificity
:host([popover]) {
  border: var(--_border);
}
keithamus commented 1 year ago

@jpzwarte are you using constructable stylesheets? If so, does your stylesheet appear before or after the popover stylesheet in the shadow root's adoptedStylesheets?

jpzwarte commented 1 year ago

@keithamus We use Lit, so yes. Afaics the polyfill stylesheet appears before the component stylesheet. CleanShot 2023-11-29 at 10 53 25@2x

keithamus commented 1 year ago

Do all browsers behave the same? Is the variable for sure correct? If you add !important does it fix it? What does devtools look like?

jpzwarte commented 1 year ago

This is how it looks in the dev tools atm: CleanShot 2023-11-29 at 11 20 25@2x

If I add !important, it looks like this: CleanShot 2023-11-29 at 11 22 13@2x

The only browser I have that hasn't implemented popover is FF, so I can't speak on "all browsers".

keithamus commented 1 year ago

I think I know the issue.

The popover style sheet is appended to the document, so that light DOM popovers work, but :host styles always take lower precedent than the light DOM styles.

If you open this codepen you'll see a pink border. If you comment out line 19 (this.ownerDocument.adoptedStyleSheets.push(styles)) you'll get a green one.

I'm not sure what the resolution here is...

jpzwarte commented 1 year ago

I guess !important for now. Thanks.

mirisuzanne commented 1 year ago

Yes, author styles in the light dom always override author styles in the shadow dom. The only way to override that is with !important. Unless we can provide a way for authors to load these styles into a shadow dom context, rather than putting them in the light dom? @keithamus I don't know if that's a reasonable thing to do?

keithamus commented 1 year ago

I’m not sure we could do that as the styles are required for popover to work. The only other option I can think of is inline styles but then they’ll take precedence over stylesheets anyway 🤷‍♂️

mirisuzanne commented 1 year ago

That makes sense. It's too bad there's no way for polyfills to inject styles at the 'presentational hints' level of the cascade (between browser and author styles)… It would be a difficult pitch to the working group if that's the only use-case, but might have other use-cases, the more authors are relying on shadow DOM. Should a site should be able to apply global defaults that underly custom element defaults? Seems like people would put their resets into a layer like that.

aigan commented 9 months ago

I have the same problem with Firefox 123, even without shadow dom. The only thing that worked was using !important. The :where() or @layer did not help.