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

Hash for styles of my Svelte component is doubled #4374

Closed
Dreaminsider opened this issue Feb 5, 2020 · 23 comments
Closed

Hash for styles of my Svelte component is doubled #4374

Dreaminsider opened this issue Feb 5, 2020 · 23 comments
Labels
awaiting submitter needs a reproduction, or clarification temp-stale

Comments

@Dreaminsider
Copy link

When I put my component to REPL I have doubled styles hash and browser could not apply styles when I write them in separate css file.

component code:

<style>
.wpk-player {
  padding: 10px 15px;
}
.wpk-button svg {
  fill-opacity: 0.8;
}
</style>

<div class="wpk-player">
  <div class="wpk-player__buttons-1">
    <button class="wpk-button">
      <svg width="26" height="22" viewBox="0 0 26 22" xmlns="http://www.w3.org/2000/svg">
        <path d="M17.245 1.849c0-.92-.628-1.298-1.397-.837L.57 10.162c-.768.463-.768 1.217 0 1.678l15.278 9.148c.769.46 1.397.085 1.397-.836V1.849zm4.756 19.152H25s.956.074.994-1.001c.006-.197-.027-17.645.001-17.996.085-1.054-.996-1-.996-1h-3.001s-1.123-.059-1.001 1c.009.078.011 17.506.004 18-.016 1.161 1 .997 1 .997z" fill="#fff"/>
      </svg>
    </button>
  </div>
</div>

CSS output:
.wpk-player.svelte-4i1jb7.svelte-4i1jb7{padding:10px 15px}.wpk-button.svelte-4i1jb7 svg.svelte-4i1jb7{fill-opacity:0.8}

Also this component reproduce the same issue:

<div class="red">
  <div></div>
</div>

<style>
.red {color:red}
.red div {color: green}
</style>

CSS outut:

.red.svelte-wvyici.svelte-wvyici{color:red}.red.svelte-wvyici div.svelte-wvyici{color:green}

@PatrickG
Copy link
Member

PatrickG commented Feb 5, 2020

I noticed this as well, and I think this is only since one of the lastest updates.

Just checked, its like this since 3.16.6
https://svelte.dev/repl/183ee758c6364a1daf24e456945e4dff?version=3.16.5
https://svelte.dev/repl/183ee758c6364a1daf24e456945e4dff?version=3.16.6

#4146

@Conduitry
Copy link
Member

@tanhauhau would probably be the best person to confirm this, but this may be intended, to ensure specificity isn't messed up by the scoping class.

@Conduitry Conduitry added the awaiting submitter needs a reproduction, or clarification label Feb 6, 2020
@Dreaminsider
Copy link
Author

@tanhauhau would probably be the best person to confirm this, but this may be intended, to ensure specificity isn't messed up by the scoping class.

But if this is intended then it cause the problem: when I generate css as a separate file my styles do not apply at all to component html.

@cvlab
Copy link
Contributor

cvlab commented Feb 7, 2020

I noticed same issue.
It's not a bug in sense of produced code, but it's not smart enough. The fix is too general, it did increase specificity in too many cases when is not necessary and did not change code correctness = pass testes.

I hope, it will be fixed soon. Fixed CSS edge case (#4146) it's rare. But css it's bigger in all projects with ^3.16.6.

@tanhauhau
Copy link
Member

yes it is intended.

see this example:

<style>
  .a { color: red }
  .a .b .c .d .e .f .g { color: green }
  .a .b .c .d { color: green }
  .b .c .e { color: red }
</style>

to make sure each css selector declaration is scoped within the component, we add hash to the first and the last selector:

  .a.svelte-hash { color: red }
  .a.svelte-hash .b .c .d .e .f .g.svelte-hash { color: green }
  .a.svelte-hash .b .c .d.svelte-hash { color: green }
  .b.svelte-hash .c .e.svelte-hash { color: red }

to make sure that the class g, d or e is still within the component, not any element outside the component.

however if you noticed in the example, the specificity changed slightly, +1 and +2 in the class specificity.

this is okay in this example, but in the example #1277:

<div>
	<span>red</span>
	<span class='foo'>green</span>
</div>

<style>
  div span {
    color: red;
  }
  .foo {
    color: green;
  }
</style>

after adding the svelte-hash class, the specificity order changed:

  div.svelte-hash span.svelte-hash {
    color: red;
  }
  .foo.svelte-hash {
    color: green;
  }

to restore the specificity order, we have to make sure that each declarations have the equal number of svelte hash class added:

  div.svelte-hash span.svelte-hash {
    color: red;
  }
  .foo.svelte-hash.svelte-hash {
    color: green;
  }

which causes some selectors to have double svelte hash class.

@cvlab
Copy link
Contributor

cvlab commented Feb 13, 2020

@tanhauhau This is not smart, in many many cases with or without double/triple/... hash, specificity order is not changed. See first example (#4374 (comment)). With preprocessing you can check if it's needed or not. Right now css files are bigger in all projects.

@tanhauhau
Copy link
Member

Yea i agree with you, it's not quite smart.

@PatrickG
Copy link
Member

PatrickG commented Feb 14, 2020

My main problem with this is, that it is hard to overwrite specific things from a parent component.
I know, the svelte guys think a parent component should not overwrite styles, but coming from web-components, that just feels normal.

What i don't understand, when the component has one root element with a class, why does this root element needs a higher specificity?

<div class="Child">
  ...
</div>

So, in the parent I have to use

.Parent :global(.Child.Child.Child) {
  my: styles;
}
// or make everything !important
.Parent :global(.Child) {
  my: styles !important;
}

dritter added a commit to dritter/extended-table that referenced this issue Apr 8, 2020
Strangely, we have to add a class, so that a
parent component can generate enough specificity.
At least, if you want to overwrite only tags.
Before this commit, it was not possible to
overwrite styles for the <th> element:
"div :global(table th)" did not work, because
of sveltejs/svelte#4374

Now it is possible by targeting
"div :global(table.et th)".
@vladshcherbin
Copy link

It also looks weird in dev tools and increases css bundle size by duplicating classes. This intended behavior is highly questionable.

@insidewhy
Copy link

insidewhy commented May 7, 2020

EDIT: Replaced my comment with a new ticket #4795

Since in my case, the duplication of hashes doesn't apply consistently and introduces a bug.

@the0neWhoKnocks
Copy link

thats a feature

I spent far too much time revalidating my Webpack config and testing plugins, only to eventually end up here.

  • Applying the same scoped class to an element multiple times to seemingly patch another scoping issue is a huge code smell.
  • If this is something that is going to stick around, it should be called out in the docs. Anywhere calling out how CSS is generated, in svelte-loader, and maybe in the example configs.
  • If this was added to address some other concern, how about an option to opt in or out of it?
  • This is the first time I've come across a specificity pattern like this. Seeing as how CSS specificity can be a tricky thing at times, I propose that the class is only added once, and any further specificity that needs to be added, should be done so by the Dev.

Currently I'm having to use !important on global styles (due to the multiple levels of specificity) which leads to it's own pitfalls. Please reconsider this functionality for the reasons I and the other comments have called out.

Just say no to .modal-body.svelte-11musit.svelte-11musit.svelte-11musit

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 27, 2021
@Arkkimaagi
Copy link

Can I ask a probably very stupid question?

Why does svelte not use :where()-selector to avoid touching the specificity alltogether?

I mean, it could be built to be "transparent" on targetting the items without adding any specificity at all. It could be an option for users to choose (if old ie support is a must). This way the double class would become totally moot.

Funny fact, I am not actually using svelte. I just read an article about these issues and wondered if this has been suggested as a possible solution.

Here, I made a quick demo. https://codepen.io/Arkkimaagi/details/qBjxXwa

@Arkkimaagi
Copy link

A more relevant demonstration here solving the issue @tanhauhau described above :
https://codepen.io/Arkkimaagi/pen/zYzRPJJ

@geoffrich
Copy link
Member

You might be referring to my article? 😁 Using :where is a compelling suggestion -- I had a similar discussion with someone on r/sveltejs when I posted that article.

This would fix these specificity issues, but I also thought of a few downsides:

  • Browsers that don't support :where would get no CSS at all (13% globally). This includes the basically-dead IE11, yes, but also the previous iOS version (13) and Samsung Internet (similar market share to Firefox, according to caniuse). You can't polyfill :where like you can with JS features, so anyone using these browsers would get no styling at all, and apps for those users could break entirely.
  • It would be a breaking change, since some global styles would start applying where they didn't before.
  • CSS size would increase in some cases, though this would probably be irrelevant with compression

For those reasons, if Svelte did implement CSS scoping this way, it should definitely be an opt-in flag as opposed to a new default.

@Arkkimaagi
Copy link

You might be referring to my article? 😁

Yep 😁

I agree, definitely opt-in and not an only option. The browser support and breaking change being the biggest issues.

@the0neWhoKnocks
Copy link

Just ran into this issue again on another project. This functionality is essentially Svelte adding it's own !important but via specificity, which is very frustrating.

Svelte gets so many things right, it's disappointing that this issue isn't getting the proper attention it deserves.

@the0neWhoKnocks
Copy link

For anyone that doesn't need the duplicate classes, here's a gist with a Webpack plugin to strip out the duplicates. It's not optimized, may have some bugs, but at the moment, it's better than nothing.

@non25
Copy link

non25 commented Nov 21, 2021

Proper solution would be to use svelte-preprocess-cssmodules and forget about non-class selectors.
#2870 (comment)

@the0neWhoKnocks
Copy link

the0neWhoKnocks commented Nov 22, 2021

@non25 I looked through that module's options, and it appears to mutate the original class name and append a hash. In my particular cases, the repos utilize BEM and I'm quite familiar with the cascade - so that wouldn't solve the issue for me (hence the plugin).
That module does look like a good solution if a Dev wants the styling isolated like with a shadowDOM. It still feels like whatever the solution, it should be opt-in, unless they switch over to where, which does seem like a nice solution allowing for specificity while also allowing overrides.

@manuganji
Copy link

manuganji commented Mar 28, 2022

I have just spent several hours with this issue.

  1. I didn't understand why this hash is added twice.

  2. vite-svelte-plugin uses a different cssHash (config value) function for dev.

Together, they took me several hours to just understand what is going on. I think if something does not work transparently, it should not be added to the core. I went over the changes in #4146 and I think it can be reverted.

@janosh
Copy link
Contributor

janosh commented Oct 25, 2022

Browser support for :where() went from 86 % on 27 Sep 2021 to 92.3 % on Oct 25 2022.

@Rich-Harris
Copy link
Member

Svelte 5 uses :where(...) (alongside scoping classes where necessary — see #10443 for details), so I'll close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification temp-stale
Projects
None yet
Development

No branches or pull requests