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

:not() selector not working properly with pseudoclasses #14168

Closed
Conduitry opened this issue Nov 5, 2024 · 5 comments · Fixed by #14177
Closed

:not() selector not working properly with pseudoclasses #14168

Conduitry opened this issue Nov 5, 2024 · 5 comments · Fixed by #14177
Assignees
Labels
bug css Stuff related to Svelte's built-in CSS handling

Comments

@Conduitry
Copy link
Member

Describe the bug

As of 5.1.10, selectors like :not(:last-child) are getting erroneously detected as unused.

Reproduction

https://svelte.dev/playground/hello-world?version=5.1.10#H4sIAAAAAAAACm2NQQvCMAyF_0rJaQN17FqH4M3bLt6ch7lGOojNaKNzjP13W3DgwVPyvpf3MoNrHwgaTkjEamRPRmVoekGTwwbuPWEAfZlBpiHdJRD5N3Uchl14IUlitzbgP96xE3QSa6Cy5aF2WBVxNi6p88i_ynpc3UiCTIRpFVtqx5JpaoNsO9uTydWcjEY6JvZaeTT7BJaYK9Zg_C74FtDin7hclw8dQ5rQ7wAAAA==

This does work in the previous version, presumably because we weren't trying to scope the :not() - https://svelte.dev/playground/hello-world?version=5.1.9#H4sIAAAAAAAACm2NQQvCMAyF_0rJaQN17FqH4M3bLt6ch7lGOojNaKNzjP13W3DgwVPyvpf3MoNrHwgaTkjEamRPRmVoekGTwwbuPWEAfZlBpiHdJRD5N3Uchl14IUlitzbgP96xE3QSa6Cy5aF2WBVxNi6p88i_ynpc3UiCTIRpFVtqx5JpaoNsO9uTydWcjEY6JvZaeTT7BJaYK9Zg_C74FtDin7hclw8dQ5rQ7wAAAA==

Logs

Unused CSS selector "h1:not(:last-child)" (css_unused_selector)

System Info

n/a

Severity

annoyance

@Conduitry Conduitry added bug css Stuff related to Svelte's built-in CSS handling labels Nov 5, 2024
@kyle-leonhard
Copy link

+1, ran into this as well on several selectors of the form: .some-button:not(:disabled):hover. Perhaps related to #13999. fyi @dummdidumm

@Rich-Harris
Copy link
Member

It's not just pseudo-classes — see example from #14169 https://svelte.dev/playground/7e9220bafc094e209bb9a53e7e7bb48d?version=5.1.10

@WaltzingPenguin
Copy link

Also can be triggered from attributes: https://svelte.dev/playground/7028eeed4643409c860ce6fee36c05ca?version=5.1.10

@dummdidumm
Copy link
Member

My mistake was to turn the logic around for all matches, not just the static ones. Static matches are those for static classes, for class: or pseudo classes or attributes they count as a match but shouldn't then be discarded by the negation. I'm not really sure if the current algorithm lets us take this nuance into account when bubbling the result back up, so as a result we possibly have to mark :not selectors as always used

@Rich-Harris
Copy link
Member

I don't draw that conclusion — I think we can probably do this (pseudocode):

if (!could_match(selector, node)) {
-  return false;
+  return is_not;
}

return true; // this bit stays the same

In other words 'definitely matches' becomes 'definitely does not match' if you're dealing with a :not while 'definitely does not match' becomes a 'definitely matches', but 'maybe matches' continues to be 'maybe matches'

Tinkering in that direction but I'm not going to get it finished before I need to leave for the evening

dummdidumm added a commit that referenced this issue Nov 6, 2024
fixes #14168

This reverts the whole "selectors inside `:not` are scoped" logic. Scoping is done so that styles don't bleed. But within `:not`,everything is reversed, which means scoping the selectors now means they are more likely to bleed. That is the opposite of what we want to achieve, therefore we should just leave those selectors alone.
trueadm pushed a commit that referenced this issue Nov 7, 2024
fixes #14168

This reverts the whole "selectors inside `:not` are scoped" logic. Scoping is done so that styles don't bleed. But within `:not`,everything is reversed, which means scoping the selectors now means they are more likely to bleed. That is the opposite of what we want to achieve, therefore we should just leave those selectors alone.

The exception are `:not` selectors with descendant selectors, as that means "look up the tree" and we need to scope all ancestor elements in that case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug css Stuff related to Svelte's built-in CSS handling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants