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

fix: use hybrid scoping strategy for consistent specificity increase #10443

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Feb 9, 2024

The context for this is #9549, though I think it may also address #7991.

Today, Svelte uses class selectors to scope local styles to the component. In other words, this...

<p>red</p>

<style>
  p {
    color: red;
  }
</style>

...is effectively turned into this, so that <p> elements outside the component are not affected by this component's styles:

<p class="svelte-xyz123">red</p>

<style>
  p.svelte-xyz123 {
    color: red;
  }
</style>

The wrinkle is that this affects the selector's specificity, in a way that could cause bugs — #1277 — unless we add redundant-looking duplicate class selectors so that the specificity increase is uniform within a component.

Unfortunately, this approach also has its flaws:

Happily, it's 2024, and we now have a way of scoping selectors without increasing specificity — :where.

One approach would be to use :where(.svelte-xyz) everywhere instead of just .svelte-xyz. We briefly tried that, but found that it violated people's expectations that component styles would be more specific than global styles that were appended to the document later. In practice, people want a specificity bump, they just want it to be predictable.

Another approach would be to make it an option. This is what Astro does with scopedStyleStrategy, for example, but I'm not personally a fan of this sort of approach — it forces the developer to think about the problem, and yields subtle differences between different apps (which might affect e.g. component libraries that they use in common). (In Astro's case there's an option to use data attributes instead of classnames — this arguably yields cleaner markup, though when we've investigated this in the past we've found that class selectors have better performance, which eventually starts to matter. For now I think we should stick with classnames, though that could be decided independently of the :where question.)

So this PR proposes a third option — a hybrid approach, that uses a class selector exactly once per compound selector, for a uniform specificity bump of +0-1-0, and uses :where for any subsequent selectors. As a bonus, it allows us to simplify the code a bit.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Feb 9, 2024

🦋 Changeset detected

Latest commit: 9312d55

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good!
We should note this in the list of breaking changes along with the regexp quickfix for backwards compatibility in case you need it (mentioning the selector specificity caveat).
Also: Is this in any way less performant (additional :where wrappers could add up, I have no idea)? Is this measurable somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that changeset filename lol

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

@Rich-Harris
Copy link
Member Author

Added a note on the breaking change. I did some naive benchmarking locally and could find no performance impact, which I think makes sense

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 this pull request may close these issues.

None yet

3 participants