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

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

Open
setyb opened this issue May 19, 2021 · 6 comments
Open
Assignees

Comments

@setyb
Copy link

setyb commented May 19, 2021

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: openstyles/stylus#1253

@skulls-dot-codes
Copy link

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 iChenLei self-assigned this Jun 18, 2021
@iChenLei iChenLei changed the title Does not support :is pseudo-class correctly support pseudo-class function which contains less Variable or other edge case Jul 7, 2021
@iChenLei
Copy link
Member

iChenLei commented Jul 7, 2021

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() 🧪
  4. :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.

e = parserInput.$re(/^(?:\d+\.\d+|\d+)%/) ||
parserInput.$re(/^(?:[.#]?|:*)(?:[\w-]|[^\x00-\x9f]|\\(?:[A-Fa-f0-9]{1,6} ?|[^A-Fa-f0-9]))+/) ||
parserInput.$char('*') || parserInput.$char('&') || this.attribute() ||
parserInput.$re(/^\([^&()@]+\)/) || parserInput.$re(/^[\.#:](?=@)/) ||
this.entities.variableCurly();
if (!e) {
parserInput.save();
if (parserInput.$char('(')) {
if ((v = this.selector(false)) && parserInput.$char(')')) {
e = new(tree.Paren)(v);
parserInput.forget();
} else {
parserInput.restore('Missing closing \')\'');

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
Copy link
Contributor

chipx86 commented Mar 7, 2024

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
Copy link
Member

@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:

  • A complete CSS parser
  • A Less parser that only extends the CSS parser, such that anything that is valid CSS is already parsed correctly, and only the Less parts are included
  • An evaluatable common AST
  • Less (and other) callable functions
  • A distinct Less "plugin" that links the parser to core functionality and adds Less functions globally

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.

@puckowski
Copy link
Contributor

I have fixes for the following syntax that passes all tests:

@-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; }
	
}
each(@color, {
  :is(.color-@{key},
  .focus-color-@{key}:focus,
  .hover-color-@{key}:hover) {
    color: @value !important;
  }
});

Will try to send a PR this weekend.

puckowski added a commit to puckowski/less.js that referenced this issue Dec 10, 2024
* Fix issue less#3622 by resolving rigid parethensis parsing issues.
puckowski added a commit to puckowski/less.js that referenced this issue Dec 11, 2024
* Improve logic for different scenarios for issue less#3622.
puckowski added a commit to puckowski/less.js that referenced this issue Dec 11, 2024
* Refactor and cleanup unused code for issue less#3622.
puckowski added a commit to puckowski/less.js that referenced this issue Dec 11, 2024
* Fix for issue less#3622. Add incorrectly removed code back in for merge.
@puckowski puckowski mentioned this issue Dec 11, 2024
3 tasks
@puckowski
Copy link
Contributor

PR #4298 should resolve some of the parsing issues with syntax raised in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants