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

HTML comments break CSS :empty pseudo selector #14299

Open
jwerre opened this issue Nov 14, 2024 · 10 comments
Open

HTML comments break CSS :empty pseudo selector #14299

jwerre opened this issue Nov 14, 2024 · 10 comments
Labels
css Stuff related to Svelte's built-in CSS handling needs discussion

Comments

@jwerre
Copy link

jwerre commented Nov 14, 2024

Describe the bug

When using conditionals in Svelte the result leaves empty comments inside the node. Since comments are considered childNodes this breaks the CSS selector :empty.

We could use :not(:has(*)) as a workaround but the css output looks like this:

ul.svelte-vewu36:not(:has(/* (unused) **/)) { ... }

That said, I'd prefer not use use :not(:has(*)) since it's very inefficient.

I've also tried setting preserveComments=false in my compiler options but it doesn't seems to do anything (in development anyway). Perhaps this has re-emerged as an issue: #4730 ?

Reproduction

https://svelte.dev/playground/d1989f455149482ca0d9202e915b743f?version=5.1.16
https://jsfiddle.net/jwerre/pcqt215b/10/

System Info

System:
    OS: macOS 15.0.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 1.21 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.18.0 - /opt/homebrew/bin/node
    Yarn: 1.22.22 - ~/.npm-global/bin/yarn
    npm: 10.8.2 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 130.0.6723.117
    Edge: 117.0.2045.40
    Safari: 18.0.1

Severity

annoyance

@dummdidumm dummdidumm added the css Stuff related to Svelte's built-in CSS handling label Nov 14, 2024
@dummdidumm
Copy link
Member

dummdidumm commented Nov 14, 2024

Ah this is indeed an implementation leak of the way we construct code (we need anchor comments to properly insert and remove html blocks), and tbh I'm not totally sure if there's something we can do about this. The :not(:has(...)) workaround is a separate bug that needs fixing

@jwerre
Copy link
Author

jwerre commented Nov 14, 2024

What about preserveComments=false? Is this working as it was intended to?

@dummdidumm
Copy link
Member

That option is only about comments the developer put in their components. The internal comments cannot be removed

@jwerre
Copy link
Author

jwerre commented Nov 14, 2024

Is there an issue for the :not(:has(*)) workaround bug? I was not able to find one if there is. I'd be dig a little deeper into this and create one.

@dummdidumm
Copy link
Member

No need, already investigating it

@harrisi
Copy link

harrisi commented Nov 14, 2024

This is actually a browser implementation bug, I think. Which sounds crazy, because all browsers implement this incorrectly, although the spec changed several years ago.

If you remove the whitespace in the fiddle, it works correctly: https://jsfiddle.net/074s1e9h/

If svelte removes all whitespace outside of comments, :empty should work fine. Also, in several more years when browsers implement this correctly, it should also start to work :)

@harrisi
Copy link

harrisi commented Nov 14, 2024

This is actually caused by the whitespace between the {#if}s, I think: https://svelte.dev/playground/43ad56433106472882f6c2d48840ea1e?version=5.1.16

EDIT: I mean, in this case that's the problem. It's still the general problem of whitespaces and comments interacting with :empty. Technically the whitespace could be significant in the repl, so Svelte shouldn't get rid of it? Maybe a specific behavior for if multiple Svelte markup tags are only separated by whitespace, it's removed?

In this specific instance, it is one case where something like a {#switch} would improve things, potentially. I know there's been lots of discussion and it likely won't happen. This is actually the first time I've felt like it's actually a specific improvement over multiple {#if}s.

@dummdidumm
Copy link
Member

Oh wow you're right, it's has nothing to do with the comments, it's the whitespace - thanks for pointing that out! So yeah, avoiding the whitespace between the if elements fixes this. It's a good question whether or not we should treat whitespace between control flow blocks as signification. We're already treating whitespace between sibiling elements as significant, so from that perspective the behavior is consistent.

@harrisi
Copy link

harrisi commented Nov 15, 2024

I think it's most (except {@debug}?) svelte templating, not just control flow (repl):

<div>{''}<!-- put a space before/after comment -->{''}</div>

<style>
  div {
    width: 10%;
    height: 10%;
    background-color: red;
  }
	
  div:empty {
    background-color: green;
  }
</style>

If you remove one or both of the {''}s, you can add spaces and the :empty selector is still used, since the whitespace is thrown away.

@dummdidumm
Copy link
Member

The rule is "whitespace between siblings is preserved (trimmed down to one space), whitespace at the start/end is removed"

dummdidumm added a commit that referenced this issue Nov 15, 2024
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 needs discussion
Projects
None yet
Development

No branches or pull requests

3 participants