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

CSS Adjacent Sibling Combinator (+) is causing some CSS to be erroneously removed #9274

Closed
DaneWeber opened this issue Sep 29, 2023 · 3 comments · Fixed by #9282 or #11096 · May be fixed by #13556
Closed

CSS Adjacent Sibling Combinator (+) is causing some CSS to be erroneously removed #9274

DaneWeber opened this issue Sep 29, 2023 · 3 comments · Fixed by #9282 or #11096 · May be fixed by #13556
Assignees
Labels
css Stuff related to Svelte's built-in CSS handling
Milestone

Comments

@DaneWeber
Copy link

DaneWeber commented Sep 29, 2023

Describe the bug

Vite/Svelte reports Unused CSS selector ":global(p) + .focus" (Focus.svelte:23:0) and removes the CSS when the p in the example selector comes from the Focus.svelte file's <slot />. This looks like it started in Svelte 3.27.0 and persists through 4.2.1 (latest).

Oddly, adding an additional class avoids this: :global(p) + .focus .extra will apply in the same scenario that :global(p) + .focus fails to.

Also, at present, if :global(p) + .focus applies to an element where the p exists within the Svelte file, it will also apply to the p coming from the <slot />, presumably because the CSS is "used" and not removed by the compiler. This was not the case in versions 3.27.0 - 3.32.1.

Reproduction

The Adjacent Sibling Combinator through <slot /> REPL shows a list that is missing its red border. You can use the links to easily change the Svelte version.

  • 3.0.0
  • ... the combinator works with <slot /> elements.
  • 3.26.0
  • 3.27.0
  • ... the combinator fails with <slot /> elements, even if another pair of elements match.
  • 3.32.1
  • 3.32.2
  • ... the combinator fails with <slot /> elements unless another pair of elements match.
  • 4.2.1

The current issue at the simplest I've found is the following:

// Example.svelte
<script>
	import Focus from './Focus.svelte';
</script>

<section class="parent">
	<Focus>
		<p>Paragraph in Focus' &lt;slot /&gt;</p>
	</Focus>
</section>

// Focus.svelte
<div>
	<slot />
	<ul class="focus">
		<li class="extra">This should have a red border.</li>
	</ul>
</div>

<style>
/* This works: */
:global(p) + .focus .extra {
  font-size: 2em;
}

/* But this doesn't? */
:global(p) + .focus {
  border: red 3px solid;
}
</style>

Logs

No response

System Info

I've reproduced in the online REPL and generally use Firefox (117.0.1) for development, but for what it's worth this is my system info for the app I'm currently working on:


 System:
    OS: macOS 13.3.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 73.59 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.16.1 - ~/.asdf/installs/nodejs/18.16.1/bin/node
    Yarn: 1.22.19 - ~/.asdf/shims/yarn
    npm: 9.5.1 - ~/.asdf/plugins/nodejs/shims/npm
    Watchman: 2023.09.18.00 - /opt/homebrew/bin/watchman
  Browsers:
    Brave Browser: 116.1.57.62
    Chrome: 117.0.5938.132
    Safari: 16.4
  npmPackages:
    svelte: ^3.24.1 => 3.59.2 

Severity

annoyance

@kelvinsjk
Copy link
Contributor

kelvinsjk commented Sep 30, 2023

On my initial attempt at looking into this, I identified the bug as caused by this line

if (siblings.size === 0 && get_element_parent(node) !== null) {
  return false
}

I managed to get a fix by creating a new function has_slot_sibling(node) and adding !has_slot_sibling(node) to the condition.

However, just as I was preparing a PR and a test case I realized that the bug isn't so much caused by the presence of the slot (it does play a part), but because of the surrounding container. In the reproduction REPL, if we removed the container div in the Focus.svelte file the css selector will then work as intended and be applied.

In light of this I think this bug warrants a bit more investigation. I'd likely come back to this in a couple of days once I get some more time on my hands

EDIT: without a container, the clause is guarded by get_parent_element() !== null. So I do think adding a has_slot_sibling function will fix it. Will send a PR if I'm able to get the test cases done

@DaneWeber
Copy link
Author

@kelvinsjk -- I see your proposed fix (thank you!) has been open for a week. Is there anything I can do to help move it forward?

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

@dummdidumm dummdidumm self-assigned this Apr 8, 2024
dummdidumm added a commit that referenced this issue Apr 8, 2024
Keep sibling selectors when dealing with slots/render tags/`svelte:element` tags
fixes #9274
dummdidumm added a commit that referenced this issue Apr 9, 2024
Keep sibling selectors when dealing with slots/render tags/`svelte:element` tags
fixes #9274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment