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

Compound global selector check is too restrictive #9207

Closed
TGlide opened this issue Sep 14, 2023 · 6 comments · Fixed by #11658
Closed

Compound global selector check is too restrictive #9207

TGlide opened this issue Sep 14, 2023 · 6 comments · Fixed by #11658
Labels
css Stuff related to Svelte's built-in CSS handling
Milestone

Comments

@TGlide
Copy link
Member

TGlide commented Sep 14, 2023

Describe the bug

I was trying to do the following:

.anim-checkbox:global([data-state='checked']) {
	background-color: red;
}

But received an error: :global(...) not at the start of a selector sequence should not contain type or universal selectors(css-invalid-global-selector-position)

Digging into Svelte's issues, I found that this check comes from issue #6272. I fear the fix implemented is too restrictive, as I think my syntax would be perfectly valid.

For now, I have this work-around:

.anim-checkbox:global(.anim-checkbox[data-state='checked']) {
	background-color: red;
}

Reproduction

Write the following into any style tag in a .svelte file:

 .a:global([data-state]) {
  /* ... */
}

Logs

No response

System Info

System:
    OS: Linux 5.15 Ubuntu 20.04.4 LTS (Focal Fossa)
    CPU: (12) x64 11th Gen Intel(R) Core(TM) i5-11400F @ 2.60GHz
    Memory: 12.15 GB / 15.57 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 20.4.0 - ~/.nvm/versions/node/v20.4.0/bin/node
    Yarn: 1.22.15 - ~/.nvm/versions/node/v20.4.0/bin/yarn
    npm: 9.7.2 - ~/.nvm/versions/node/v20.4.0/bin/npm
    pnpm: 8.6.7 - ~/.nvm/versions/node/v20.4.0/bin/pnpm
  npmPackages:
    svelte: ^4.2.0 => 4.2.0

Severity

annoyance

@brunnerh
Copy link
Member

brunnerh commented Sep 16, 2023

The selector makes little sense, it claims two conflicting things.

In words it would be something like: "The element is from this component, its attribute is maybe from somewhere else."
The selector will get the scoping class, what happens with the attribute is irrelevant at that point as Svelte does not track attributes the same way it tracks element selectors (scoped CSS for elements not found in the markup is removed).

I only see three real alternatives here:

  • The entire selector is global, if the element is not from this component's markup and it does not matter whether it is an ancestor or descendant:
     :global(.anim-checkbox[data-state='checked']) { ... }
  • The elements not from the component are descendants, you then can have a container with a scoped selector:
     .local-container :global(.anim-checkbox[data-state='checked'])  { ... }
  • Everything is just scoped:
     .anim-checkbox[data-state='checked'] { ... }

Your workaround is also contradictory, it will just lead to a selector with the scoping class .svelte-<hash> once, .anim-checkbox twice and an attribute selector. Since the scoping class is already in there, you might as well just not scope at all?

If I am missing something, please give a concrete example with markup showing what you are trying to do here.

@TGlide
Copy link
Member Author

TGlide commented Sep 17, 2023

@brunnerh The selector I'm using does make sense, as shown here: #6222. I need it because, just like the example in the issue shown, the data-attribute comes from a svelte action.

In words it would be something like: "The element is from this component, its attribute is maybe from somewhere else."

I'd express it differently: the selector is found to be present by static analysis of the code, while this data-attribrute is not, but may be present through outside influence.

The entire selector is global, if the element is not from this component's markup and it does not matter whether it is an ancestor or descendant:

I need the scoping, as the element is present in the markup.

The elements not from the component are descendants, you then can have a container with a scoped selector:

Requiring a container for this is a bad workaround IMO. If doing .a:global(.b) works and is supported, so should what I'm trying to achieve.

Everything is just scoped:

This won't trigger, as the data-attribute comes from an action.

Your workaround is also contradictory, it will just lead to a selector with the scoping class .svelte- once, .anim-checkbox twice and an attribute selector. Since the scoping class is already in there, you might as well just not scope at all?

It's not contradictory, it's exactly what I want, but I'd love to not have .anim-checkbox twice.

Here is a REPL illustrating the issue https://svelte.dev/repl/f6fb6eb216aa440881a8a9bac03def6f?version=4.2.0

@brunnerh
Copy link
Member

brunnerh commented Sep 17, 2023

I see the issue now. Svelte is being stingy in applying the scoping class to the elements, it only does so if it detects a matching selector in the CSS. (Which is probably a reasonable thing to do, especially in SSR, where everything needs to be transmitted as well.)

Other workarounds are:

  • Swapping the selectors like this:

    :global([data-state='checked']).anim-checkbox

    Of course that is not how one would intuitively write that.

  • Add an empty style for just the class to make Svelte apply the scoping class to the elements:

    .anim-checkbox { }
    .anim-checkbox[data-state='checked'] { ... }

    Linters may yell at you, though.

So I agree with you, this kind of selector has to be allowed, my mistake.

@TGlide
Copy link
Member Author

TGlide commented Sep 18, 2023

Swapping the selectors like this:

This works quite well! Thank you for the suggestion 😄

Add an empty style for just the class to make Svelte apply the scoping class to the elements:

Hmm, this did not work on my REPL to be honest 🤔

So I agree with you, this kind of selector has to be allowed, my mistake.

No worries. I'd love to add a contribution myself, will try and give it a go. No idea how to get started but I've always wanted to contribute directly to Svelte.

@brunnerh
Copy link
Member

Hmm, this did not work on my REPL to be honest 🤔

Right, that only works if there also is an element which statically matches the scoped selector.
Otherwise the entire rule is thrown out; you get an unused CSS selector warning in that case (Example).

@Rich-Harris Rich-Harris added the css Stuff related to Svelte's built-in CSS handling label Apr 3, 2024
@Rich-Harris Rich-Harris added this to the 5.0 milestone Apr 3, 2024
@dummdidumm
Copy link
Member

works in Svelte 5, but we should add a test for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css Stuff related to Svelte's built-in CSS handling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants