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

feat: CSS Nesting implementation #9549

Closed
wants to merge 78 commits into from

Conversation

AlbertMarashi
Copy link

@AlbertMarashi AlbertMarashi commented Nov 20, 2023

Implement CSS nesting

Implements

  • CSS rule nesting using & prefix (ie .foo { & div { color: red } })
  • CSS At-Rules nesting
  • CSS rule nesting without & type (element) selector (ie. .foo { div { color: red } })
  • CSS rule nesting without & for CSS Combinators (ie. .foo { + div { color: red } })
  • CSS rule nesting without & for class & ID selectors (ie. .foo { .bar { color: red } })
  • Appending the & nesting selector to reverse rule context (ie. .foo { .bar & { color: red } }, equiv to .bar { .foo { color: red } })

Closes:

Copy link

changeset-bot bot commented Nov 20, 2023

🦋 Changeset detected

Latest commit: d470e13

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

vercel bot commented Nov 20, 2023

@AlbertMarashi is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Nov 27, 2023

@dummdidumm @Rich-Harris @eltigerchino @gtm-nayan @benmccann

I believe I have fully implemented support for the different kinds of CSS nesting, including a suite of tests for each kind of nesting. All tests on my end seem to be parsing fine

Looking for feedback from a maintainer on my PR.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@AlbertMarashi
Copy link
Author

AlbertMarashi commented Nov 30, 2023

@Rich-Harris I need you or someones help that knows the analyzer code in and out to help figure out if my approach is right, or if certain parts need to be re-done.

I don't know if I'm checking for unused selectors properly

Happy to implement, just need direction, but all my tests seem to be passing - just not sure if there's a better & less cryptic way to do this

@AlbertMarashi AlbertMarashi changed the title CSS Nesting implementation fix/feat: CSS Nesting implementation Dec 14, 2023
@AlbertMarashi AlbertMarashi changed the title fix/feat: CSS Nesting implementation feat: CSS Nesting implementation Dec 14, 2023
@Rich-Harris
Copy link
Member

Bear with me for a little while longer — please hold off on making any additional commits, I need to sign off for the night but have some further changes in mind.

Re your latest commit, the .c rule is empty, because the & & within is unused.

@Rich-Harris
Copy link
Member

Unfortunately I'm starting to think we're barking up the wrong tree here. Flattening the selectors out like we're doing isn't quite correct. Consider a case like this:

<x>
  <y></y>
</x>

<style>
  x y {
    & & {
      color: red;
    }
  }
</style>

Internally, the & & is unrolled to x y x y which is incorrect, yet which is marked as used. Changing it to this...

<x>
  <y></y>
</x>

<style>
  x, y {
    & & {
      color: red;
    }
  }
</style>

...yields an internal representation of x x, y y which is also incorrect, and which is incorrectly marked as unused.

I think this is one of those cases where flattening seems like the easy and sensible thing to do, but what we actually need is a recursive approach. It turns out that the first case is equivalent to :is(x y) :is(x y) (which doesn't apply to the markup), while the second case is equivalent to :is(x, y) :is(x, y) (which does).

By doing this we also make the process much more efficient. Consider this case:

<a>
  <y></y>
</a>

<style>
  a, b, c, d, e {
    x {...}
  }
</style>

With the current approach — a x, b x, c x, d x, e x — we must try each of the five generated selectors in order to determine that none of them apply to the <y> node.

If instead we have a single :is(a, b, c, d, e) x selector, we can bail out immediately.

Unfortunately I think this means a complete overhaul of this PR. It also means that we need to implement support for :is(...) and :where(...) (today, the selector lists inside those are not actually processed — we just automatically keep everything, unencapsulated, which is a bug). Will be working on these today.

@Rich-Harris Rich-Harris mentioned this pull request Feb 14, 2024
@AlbertMarashi
Copy link
Author

AlbertMarashi commented Feb 14, 2024

.x .y {
    & & {
      color: red;
    }
  }

I'm fairly sure that is equivalent to .x .y .x .y, are you sure that you're correct?

image

because

.x .y {
  && {
    color: red;
   }
}

I believe is equivalent to
.x .y

text

The text will be red here in the browser (firefox) (but I think what we're doing though is potentially resulting in .x .y.x .y which is I think is wrong. (perhaps we need to remove adjacent & to merge them into a single &)

From my understanding, the spec doesn't really say how to handle this, but at least I think the browser handles it as .x .y


From the spec

/* Somewhat silly, but & can be used all on its own, as well. */
.foo {
  color: blue;
  & { padding: 2ch; }
}
/* equivalent to
  .foo { color: blue; }
  .foo { padding: 2ch; }

  // or

  .foo {
    color: blue;
    padding: 2ch;
  }
*/

/* Again, silly, but can even be doubled up. */
.foo {
  color: blue;
  && { padding: 2ch; }
}
/* equivalent to
  .foo { color: blue; }
  .foo.foo { padding: 2ch; }
*/
/* & can be used multiple times in a single selector */
.foo {
  color: blue;
  & .bar & .baz & .qux { color: red; }
}
/* equivalent to
  .foo { color: blue; }
  .foo .bar .foo .baz .foo .qux { color: red; }
*/

The nesting selector can be desugared by replacing it with the parent style rule’s selector, wrapped in an :is() selector. For example,

a, b {
  & c { color: blue; }
}

is equivalent to

:is(a, b) c { color: blue; }

@AlbertMarashi
Copy link
Author

:is seems to only be used when you use a selector list, eg: a, b and not a b

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Feb 14, 2024

x, y {
   & & {
     color: red;
    }
  }

...yields an internal representation of x x, y y which is also incorrect, and which is incorrectly marked as unused

Are you sure, I don't think that is how I programmed it.

In the rule, the selectors are mapped, so we get one parent x and one parent y which should create four rules that look like

  • x x
  • x y
  • y x
  • y y

But what actually happens I think right now is:

  • x x, y
  • y x, y

Which is wrong. I'm not sure what is actually occurring here, I need to test it out

We need to do

  • :is(x, y) :is(x, y)

@Rich-Harris
Copy link
Member

I'm fairly sure that is equivalent to .x .y .x .y, are you sure that you're correct?

Yes, I'm sure. Try this markup in CodePen or whatever:

<x>
  <y>
    <y>this is red</y>
  </y>
</x>

<style>
  x y {
    & & {
      color: red;
    }
  }
</style>

The text is red because the selector is equivalent to :is(x y) :is(x y). If it was equivalent to x y x y then it wouldn't apply — there's no <x> between the two <y> nodes.

:is seems to only be used when you use a selector list, eg: a, b and not a b

:is(a) {...} is equivalent to a {...}. For implementation simplicity, it makes sense to treat every & as though it were an :is(<parent-selector-list>) selector.

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Feb 14, 2024

Hmm. It seems you are correct here.

From the spec

.ancestor .el {
  .other-ancestor & { color: red; }
}
/* equivalent to
  .other-ancestor :is(.ancestor .el) { color: red; }
*/

Okay, so what do we need to do here in order to fix all of this? I've got some ideas

  • Implement proper apply logic for :is, potentially even handling selector lists (a, b) within there too.
  • Remove the selector mapping (creating multiple selectors per selector in a list inside of class Rule)
  • Remove the "double nested for loops" we're using all over the place (since we're no longer doing this kind of groups approach)
  • Update/remove the group_selectors to replace/wrap parent selectors with :is

I feel like this should simplify the code to some degree in theory

@Rich-Harris
Copy link
Member

Okay, so what do we need to do here in order to fix all of this?

I'm working on it, starting with #10482. I'm going to close this PR as it will be very onerous to keep up with inbound changes, and the implementation is going to end up looking very different. As I say, bear with me!

I appreciate you kicking this off — it's already forced some significant improvements in Svelte's CSS handling, with more to come.

@AlbertMarashi
Copy link
Author

damn 😭 I wanted to become a contributor. I'd like to help if possible

@Rich-Harris
Copy link
Member

This is not red (firefox)

Right, that's what you'd expect

@dummdidumm
Copy link
Member

damn 😭 I wanted to become a contributor. I'd like to help if possible

You already helped! It's not visible as a PR author (yet!) but this PR sparked discussion and your back and forth with Rich will result in a better CSS parser in the end - thank you for that!

@Rich-Harris
Copy link
Member

Implementing the div :global {...} idea would be a good way to earn the contributor badge, once all the other stuff is in place 😀

@AlbertMarashi
Copy link
Author

AlbertMarashi commented Feb 16, 2024

@Rich-Harris will do, I'll watch the other PR.

@Rich-Harris Rich-Harris mentioned this pull request Feb 16, 2024
5 tasks
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.

5 participants