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: ensure non-matching elements are scoped for :not(...) selector #13999

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

dummdidumm
Copy link
Member

If the contents of a :not selector don't match, then it's actually a match for :not because it's inverted. Therefore, we need to scope such elements.

Fixes #13974

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

If the contents of a `:not` selector don't match, then it's actually a match for `:not` because it's inverted. Therefore, we need to scope such elements.

Fixes #13974
Copy link

changeset-bot bot commented Oct 28, 2024

🦋 Changeset detected

Latest commit: a1f3cbd

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

pkg-pr-new bot commented Oct 28, 2024

pnpm add https://pkg.pr.new/svelte@13999

commit: a1f3cbd

@tanhauhau
Copy link
Member

This change will mark elements as scoped and add the svelte-xyz class to all the elements, even though the :not statement does not match any elements

<div class="a">aaa</div>
<div>bbb</div>

<style>
	:not(.b) {
		background-color: blue;
	}
</style>

@dummdidumm
Copy link
Member Author

You're right this is a limitation of this approach - either we say "this is rare enough so we don't care" or we gotta make this lazy (i.e. "store this stuff somewhere until it matches and then scope all the other elements")

@Conduitry
Copy link
Member

Are we prevented from making this more precise in the future because it would be a breaking change for people who were inadvertently using the more generous behavior? Or do we not want to worry about those kinds of breaking changes?

@dummdidumm
Copy link
Member Author

I think we're ok with this - there's already imperfect scoping in other places (like when we can't properly statically analyze things, and with enough effort we could in the future). The question around this is more about "do we want to do the additional effort which is a bit cumbersome but ultimately not that high directly for this specific case, or do we live with this edge case?".

@7nik
Copy link

7nik commented Oct 31, 2024

This change will mark elements as scoped and add the svelte-xyz class to all the elements, even though the :not statement does not match any elements

<div class="a">aaa</div>
<div>bbb</div>

<style>
	:not(.b) {
		background-color: blue;
	}
</style>

Isn't that correct? Both divs are not .b and we don't want to leak the styles.

@dummdidumm
Copy link
Member Author

This change will mark elements as scoped and add the svelte-xyz class to all the elements, even though the :not statement does not match any elements

Revisiting this - this is actually a very interesting case. What do we do, if a :not selector always matches by having no match for any :not(...) content? Right now it's removed. But it should probably be the reverse, if it never matches then it should be marked as unused. Code example:

<foo class=x>foo</foo>
<bar class=x>bar</foo>

<style>
  :not(.x) { color: red; } /* right now this never matches, but is kept as used */
  :not(.y) { color: green; } /* right now this always matches, but is removed as unused */
</style>

Both cases arguably don't make much sense since one never matches because the contents of the :not condition always are satisfied, and the other always matches in which case you could also just leave it out. But in light of what we do elsewhere, I actually think we need to turn this around, and this would then also solve the problem @tanhauhau mentioned.

…ed, when it never matches by always matching, it's unused
@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-13999-svelte.vercel.app/

this is an automated message

@dummdidumm dummdidumm merged commit 7dbe812 into main Nov 4, 2024
11 checks passed
@dummdidumm dummdidumm deleted the not-scope-fix branch November 4, 2024 21:49
@github-actions github-actions bot mentioned this pull request Nov 4, 2024
@Serator
Copy link

Serator commented Nov 5, 2024

What if the class is wrapped in a condition? This code worked in 5.1.9 but does not work in 5.1.10.

<script>
  let isGreen = $state(false)
</script>

<button
  class:is-green={isGreen}
  onclick={() => (isGreen = !isGreen)}
>
  Toggle green
</button>

<style>
  :not(.is-green) {
    background-color: red;
  }
  .is-green {
    background-color: green;
  }
</style>

UPDATED: #14168 - looks like it's already been discussed here.

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.

CSS selector :not() no longer matches
7 participants