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

Problems with overwriting popover styling for Firefox #147

Closed
anna-lach opened this issue Nov 15, 2023 · 16 comments · Fixed by #153
Closed

Problems with overwriting popover styling for Firefox #147

anna-lach opened this issue Nov 15, 2023 · 16 comments · Fixed by #153
Assignees

Comments

@anna-lach
Copy link

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
Copy link
Member

jgerigmeyer commented Nov 27, 2023

@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
Copy link
Member

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
Copy link
Collaborator

keithamus commented Nov 27, 2023

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
Copy link
Author

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 jgerigmeyer linked a pull request Nov 28, 2023 that will close this issue
@jgerigmeyer
Copy link
Member

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

@jpzwarte
Copy link
Contributor

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
Copy link
Collaborator

@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
Copy link
Contributor

@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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

I guess !important for now. Thanks.

@mirisuzanne
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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
Copy link

aigan commented Mar 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants