less / less.js

Less. The dynamic stylesheet language.
http://lesscss.org
Apache License 2.0
17k stars 3.41k forks source link

support pseudo-class function which contains less `Variable` or other edge case #3622

Open setyb opened 3 years ago

setyb commented 3 years ago

Here are 2 examples of valid code using the :is pseudo-class that are not handled by LESS correctly. Can you please allow valid code like this to work with LESS?

Tested on: https://lesscss.org/less-preview/ which erroneously issues the error message: Missing closing ')' for both examples.

@-moz-document domain("example.com") {
    :is([foo], [bar] /* :is(a, b) */, [baz]) {
        color: red; }
}
@-moz-document domain("example.com") {
        :is(
            [a1],
            [a2]
        ):is(
            [a3],
            [a4]:not(
                :is([a5],[a6])
            ):is(
                [a7]:not(:is([a8],[a9])),
                [b1]:not(:is([b2],[b3]))
            ):is(
                [b4],[b5]
            ),
            [b6],
            [b7]
        ) {
            color: red; }

}

See also: https://github.com/openstyles/stylus/issues/1253

skulls-dot-codes commented 3 years ago

The bug includes comments or variables inside of the :is() selectors

Example with the Missing closing ')' error

each(@color, {
  :is(.color-@{key},
  .focus-color-@{key}:focus,
  .hover-color-@{key}:hover) {
    color: @value !important;
  }
});
iChenLei commented 3 years ago

TL;DR

Less not support write expression(and comment which contain &()@ character ) in pseudo-class function currently.


functional css pseudo-class from MDN:

  1. :dir() 🧪
  2. :has() 🧪
  3. [:host()](https://developer.mozilla.org/en-US/docs/Web/CSS/:host()) 🧪
  4. [:host-context()](https://developer.mozilla.org/en-US/docs/Web/CSS/:host-context()) 🧪
  5. :is() 🧪
  6. :lang()
  7. :not()
  8. :nth-child()
  9. :nth-col() 🧪
  10. :nth-last-child()
  11. :nth-last-col() 🧪
  12. :nth-last-of-type()
  13. :nth-of-type()
  14. :state() 🧪
  15. :where() 🧪

🧪 means experimental feature

A minimal reproducation code snippet for skuIIs and setyb's issue.

@selector: p;
:is(@selector) {
  color: red;
}
- Missing closing ')'

Why these code work?

@color: red;
:is(/* :is */ .color-lei, .color) {
  color: @color;
}

compile to

:is(/* :is */ .color-lei, .color) {
  color: red;
}

Let's try to debug it ->

2021-07-06 20 32 22

:is(/* :is */ .color-lei, .color) entity was treated like selectors, :is and (/* :is */ .color-lei, .color) are two elements of selectors.

From a source code(master branch) perspective, we can look the detail of less parse process.

https://github.com/less/less.js/blob/36bb6cef7c23034e9fdd4b64bd123257ac73a445/packages/less/src/less/parser/parser.js#L1269-L1282

parserInput.$re(/^\([^&()@]+\)/) match (balabala....) as a element, but it can't contain & ( ) and @ otherwise parse fail.

2021-07-07 11 30 47

That's why when you add a comment node which contain ( will cause parse fail.

// error
:is([foo], [bar] /* :is(a, b) */, [baz]) {
    color: red; 
}
// ok
:is([foo], [bar] /* :is */, [baz]) {
    color: red; 
}
// error
@selector: p;
:is(@selector) {
    color: red; 
}
// ok
:is(p) {
    color: red; 
}

Conclusion

I think this is a feature request and not bug fix, terminal error message ouput misleading us. The question is Less not support write expression(and comment which contain &()@ character ) in pseudo-class function currently.

/cc @matthew-dean Sir, How do you thought about this ?

chipx86 commented 6 months ago

The analysis here is great.

As these new selectors have matured, we've started to investigate moving over to them incrementally, but got bit by this bug. I'm curious if there are any plans to rework the affected parts of the parser that are getting tripped up here.

If not, is there at least a favored approach to resolving it in case we decide to attempt a fix? A cursory, naive read through the parser code suggests that these newer selectors probably would need to be explicitly handled, and that logic could be less strict than the general selector logic here.

I wouldn't mind playing around with a potential fix if the team here is open to it, but I don't want to go down the wrong path in a fix, or duplicate existing work.

matthew-dean commented 6 months ago

@iChenLei @chipx86 So, I've been working off and on for the past 2-3 years (just that long because it's when I have time) on a ground-up re-write of Less parsing and evaluation. (It's actually a parsing and AST library that can support multiple CSS pre-processing languages). I haven't written much about it here because I've considered it sort of experimental and I don't know all the trade-offs, but that is to say that I think that approach probably has more merit than getting too far into the weeds on the current Less parser.

The reason why I feel that's a better approach is because it's divided into separate installable packages:

This is still experimental and pre-alpha, so there's a couple possibilities:

  1. Using that pattern as "inspiration" for reworking some of the current parser
  2. Waiting until that parser & all packages are finished (for which I have no timeline)

Specifically for this (and some other issues), it also solves a few other problems, in that:

  1. It auto-absorbs comments without any extra work in a parsing function.
  2. As it's a conformant CSS parser first, it has a suite of basic CSS tests, and thus ends up being a far more accurate CSS parser than the current Less parser (which allows a lot of invalid CSS that would be discarded by the browser).
  3. It is more up to spec with current selectors and the CSS specification in a ground-up, organized way, whereas the Less parser is old enough that some CSS patterns are sort of "hacked in". So, selectors like :is() and :where() (and others) have proper parsing according to their respective specs.
  4. It uses a proper parsing engine vs. regular expressions, so it ends up being a little more readable.
  5. It seems to be about 2x as fast for parsing + evaluation than the current Less engine (but I can't really prove that until there's 100% feature parity, which isn't there yet).

So yeah, just wanted to add that, FWIW. @chipx86 that isn't to say you aren't welcome to improve the current Less parser as-is and submit a PR, since it would still be some time before these particular issues are addressed. Just wanted to offer that as context.