parcel-bundler / lightningcss

An extremely fast CSS parser, transformer, bundler, and minifier written in Rust.
https://lightningcss.dev
Mozilla Public License 2.0
6.37k stars 182 forks source link

nesting rules not transformed correctly when targeting older browsers #202

Open cyjake opened 2 years ago

cyjake commented 2 years ago

currently if the upper scope of the nested rule is compound selector like below:

.foo, .bar {
  & .baz {
    padding: 0;
  }
}

It will use :is() in the result:

:is(.foo, .bar) .baz {
  padding: 0;
}

which shortens the transform result but breaks the webpage when targeting older browsers, :is() was not supported until Chrome >= 88, yet there still are circumstances that Chrome 55 or even older browsers needs to be supported.

https://parcel-css.vercel.app/#%7B%22minify%22%3Atrue%2C%22nesting%22%3Atrue%2C%22customMedia%22%3Atrue%2C%22cssModules%22%3Afalse%2C%22analyzeDependencies%22%3Afalse%2C%22targets%22%3A%7B%22chrome%22%3A3604480%7D%2C%22source%22%3A%22.foo%2C%20bar%20%7B%5Cn%20%20%26%20.baz%20%7B%5Cn%20%20%20%20padding%3A%200%3B%5Cn%20%20%7D%5Cn%7D%5Cn%22%2C%22unusedSymbols%22%3A%5B%5D%2C%22version%22%3A%22local%22%7D

devongovett commented 2 years ago

Transforming :is correctly is hard if not impossible in some cases, unfortunately. The problem is that :is may contain complex selectors with multiple components, such as .foo :is(.bar .baz). This is not equivalent to .foo .bar .baz.bar in this case may actually be an ancestor of .foo rather than a descendant.

Example:

<div class="bar">
  <div class="foo">
    <div class="baz">test</div>
  </div>
</div>
<style>
.foo :is(.bar .baz) {
  color: red;
}
</style>

With this example, the color will be red, whereas if the code were compiled to .foo .bar .baz the selector would not apply at all.

In addition, specificity is hard to emulate correctly. The specificity of :is resolves to the highest weight of its arguments. So for example .foo:is(.bar, #baz), the id selector has a heavier weight than the class selector, even when it doesn't match. Expanding this to .foo.bar, #baz.foo would be incorrect, because #baz.foo doesn't match and therefore the rule takes the weight of the class selector.

Example:

<div class="foo bar">test</div>

With :is, the color will be red.

.foo:is(.bar, #baz) {
  color: red;
}

.foo.bar {
  color: green;
}

Expanded, the color will be green.

.foo.bar, #baz.foo {
  color: red;
}

.foo.bar {
  color: green;
}
cyjake commented 2 years ago

Thanks for the quick response! I think the problem isn't about transforming :is() itself, it's rather how the nesting rules should be transformed. There should be enough context about the precedency of selectors when the selectors were nested and glued together with &.

Take the html above for example,

<div class="bar">
  <div class="foo">
    <div class="baz">test</div>
  </div>
</div>

an transformed result like:

.foo :is(.bar .baz) {
  color: red;
}

might be written as:

.foo {
  @nest .bar  & .baz {
}

which can be transformed as .bar .foo .baz. Please correct me if I misunderstood anything, thanks for this great tool again, looking forward to use it more in production.

devongovett commented 2 years ago

Yup, the nesting spec says that & should behave the same way as :is when there are multiple selectors in the parent rule. It is not a simple substitution – it represents the elements matched by the parent selector, not the parent selector itself.

So, in an example like this:

.foo .bar, .foo .baz {
  @nest .test & {
    color: red;
  }
}

we must output: .test :is(.foo .bar, .foo .baz) and not .test .foo .bar, .test .foo .baz. Same applies for the specificity issue I described above.

Personally, I think this behavior is somewhat confusing and non-intuitive (especially that the behavior changes depending on whether the parent rule has multiple selectors), but that's something that should be brought up with the CSS working group.

cyjake commented 2 years ago

I see... so what about browsers that doesn't support :is()? can we provide a fallback transform result if the specified targets might not support is()?

devongovett commented 2 years ago

I don't think it's fully possible to support all cases, but I can think of two possibilities:

  1. For :is, we output :-webkit-any/:moz-any which are similar to :is but supported in some older browsers. We already do this if you write :is manually, but not for nesting. However, these don't support complex selectors with multiple components (e.g. .foo .bar), so we couldn't convert in all cases.
  2. We might be able to convert nesting and :is to regular selectors in more cases (like your initial example), but again, we couldn't support complex selectors or cases with non-equal specificity.

My main concern is that it'll be confusing in which cases it's possible to downlevel or not, so we'd need a way to warn the user when we can't.

yisibl commented 1 year ago

The :is() selector introduces a very new CSS feature called <forgiving-selector-list>, which is implemented differently in different browsers and has complex compatibility issues.

yisibl commented 1 year ago

In the future, it might be possible to package two CSS files.

However, the addition of :is() causes the selector specificity to change, which results in the above two CSS rendering results being different and breaking the page.

This severely discourages users from using native CSS nesting, so I suggest adding adding an option to let the user choose whether to generate :is(). Or customize an at rule to enable compilation to :is() for specific files.

@nest-expansion-to-is: false;

.foo, .bar {
  & .baz {
    padding: 0;
  }
}

/* Output */
.foo .baz, .bar .baz {
  padding: 0;
}

In the browser, the only concern is that removing :is() will cause the selector performance to explode exponentially. However, this should be left to the user's choice, and if they really need :is() to improve performance, then they can just add :is() where they need it. Also, we already had a lot of best practices in Sass/Less before native nesting arrived, such as stylelint to keep nesting levels to no more than 3 levels.

https://drafts.csswg.org/css-nesting/#:~:text=why%20is%20the%20specificity%20different%20than%20non-nested%20rules%3F

That skirts the question, tho: why do we define & in terms of :is()? Some non-browser implementations of Nesting-like functionality do not desugar to :is(), largely because they predate the introduction of :is() as well. Instead, they desugar directly; however, this comes with its own significant problems, as some (reasonably common) cases can accidentally produce massive selectors, due to the exponential explosion of possibilities.

devongovett commented 1 year ago

However, the addition of :is() causes the selector specificity to change, which results in the above two CSS rendering results being different and breaking the page.

I think nesting and :is behave the same way regarding specificity. The spec says

The nesting selector intentionally uses the same specificity rules as the :is() pseudoclass, which just uses the largest specificity among its arguments

So compiling to :is as we do today is correct. If we compiled down further than :is, this would be incorrect in some cases.

yisibl commented 1 year ago

@devongovett Ultimately, this may require a change in the specification to add an option to allow desugaring in a way that is not :is().

cyjake commented 1 year ago

compiling to :is() is correct ideally but not very practically, specially if the user needs to be dealing with Chrome < 88, which still is quite often across those electron based apps. I've added lightningcss in our bundler for a while, but still can not enable it by default because of this.

otoh, can :is() be compiled to :--webkit-any() or :matches()? can the newly added plugin system be used to overcome this short coming?

at present, I think when compiling with browsers query lower than chrome 88, there should at least be a warning.

yisibl commented 1 year ago

otoh, can :is() be compiled to :--webkit-any() or :matches()? can the newly added plugin system be used to overcome this short coming?

cyjake commented 1 year ago

aha, totally missed that comment, I won't how postcss-nesting does it, which is the one we're using at the moment.

yisibl commented 1 year ago

postcss-nesting is no longer maintained, and past implementations are far from the current CSS Nesting specification.

cyjake commented 1 year ago

so that leaves us with not much options then. Projects that still need to support chrome < 88 should be using postcard-nesting even though it is no longer maintained.

devongovett commented 1 year ago

Chrome 88 was released in January 2021, over 2 years ago. Is anyone still using it? Have you considered bumping your minimum target?

cyjake commented 1 year ago

I did, here's the top 6 browsers accessing our web app

browser version  
Chrome 90 0.733443826863615
Chrome 86 0.134079940265269
Chrome 110 0.0639012330185159
Chrome 108 0.0304644181633339
Chrome 80 0.0191422565948676
Chrome 95 0.0189683250943989

There are electron apps using legacy chromium that are not easy to bump (native addons probably).

lecodeur commented 1 week ago

What is confusing is the different output depending on the top selector ?

For exemple for a single class/element selector such as :

.bar {
  .foo & {
    color: red;
  }
}

the output is straightforward and is in line with what the developer is expecting :

.foo .bar {
  color: red;
}

Whereas the following :

.bar .baz {
  .foo & {
      color: red;
  }
}

is transpiled to the :is version (which on the minifying side of things is also slightly less efficient?)

.foo :is(.bar .baz) {
  color: red;
}

Haven't found any workaround apart from being careful to use only single selectors, would love it if nesting always worked as in the first example. (we often still need to target older browsers in our apps)

Note: esbuild is explicite on the issue

▲ [WARNING] Transforming this CSS nesting syntax is not supported in the configured target environment ("chrome109", "edge125", "firefox115", "ios15.6", "opera108", "safari13") [unsupported-css-nesting]

    src/components/article-card/index.css:64:27:
      64 │       [class*="_layout"] + & {
         ╵                            ^

  The nesting transform for this case must generate an ":is(...)" but the configured
  target environment does not support the ":is" pseudo-class.

And from the spec this is indeed the desired output for .ancestor .el constructions, and therefore the lightningcss output is correct :

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