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

fix(css_parser): fix the CSS parser doesn't support nested selectors with pseudo-classes #3287 #3318

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

denbezrukov
Copy link
Contributor

Summary

Fix #3287
Details see #3287

Test Plan

cargo test

@github-actions github-actions bot added A-Parser Area: parser A-Formatter Area: formatter L-CSS Language: CSS A-Changelog Area: changelog labels Jun 30, 2024
Copy link

codspeed-hq bot commented Jun 30, 2024

CodSpeed Performance Report

Merging #3318 will degrade performances by 8.45%

Comparing fix/css-pseudo-classes (19cc053) with main (1218c3a)

Summary

❌ 1 regressions
✅ 107 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main fix/css-pseudo-classes Change
db_2930068967297060348.json[cached] 12.7 ms 13.9 ms -8.45%

@ematipico
Copy link
Member

ematipico commented Jul 1, 2024

It seems your last commit fixes the perf regression: https://codspeed.io/biomejs/biome/branches/fix/css-pseudo-classes

@denbezrukov
Copy link
Contributor Author

@ematipico Hm, I need to check the snapshots, though. I might break something😥

I'd like to try one more approach by checking the identifier after : to understand if it is a selector. I don't know what the best solution here=(

@ematipico
Copy link
Member

ematipico commented Jul 1, 2024

What are you trying to solve? What are you trying to distinguish? It isn't clear from the issue or the PR description

@denbezrukov
Copy link
Contributor Author

denbezrukov commented Jul 1, 2024

What are you trying to solve? What are you trying to distinguish? It isn't clear from the issue or the PR description

Sorry for the misleading description.

I'm trying to distinguish this example:

.class {
     div:hover {.  <--- here it's a selector
        color: green;
     }
     
     background: red;  <--- here it's a declaration
}

A nested selector with a pseudo-class has the same structure as a declaration: [indent][colon]

I've tried to parse a nested selector and, if we encountered an error, to reparse it as a declaration. It worked but had a negative performance impact. Now, I am trying to parse a declaration and reparse it as a selector if it fails. One more idea is to check all possible pseudo-class selectors.

#3287 (comment)

@ematipico
Copy link
Member

ematipico commented Jul 1, 2024

Can't we break down the parsing of the selector to a minimum?

For example, we know that if we find a {, it means we are paring a selector. If we encounter a ; - in this example - we are parsing a declaration.

So we could do the parsing until { inside its own function. I believe we use this strategy inside our JS parser here:

match try_parse(p, |p| {
try_parse_parenthesized_arrow_function_head(p, Ambiguity::Disallowed)
}) {

@denbezrukov
Copy link
Contributor Author

Can't we break down the parsing of the selector to a minimum?

For example, we know that if we find a {, it means we are paring a selector. If we encounter a ; - in this example - we are parsing a declaration.

So we could do the parsing until { inside its own function. I believe we use this strategy inside our JS parser here:

match try_parse(p, |p| {
try_parse_parenthesized_arrow_function_head(p, Ambiguity::Disallowed)
}) {

You're right. I've tried both approaches; however, when I tried to parse as a nested selector, we experienced a performance degradation. But vice versa, we don't face the performance issue because we don't have test cases with nested selectors.

@ematipico
Copy link
Member

Yeah, I understand now. I wonder if we should check how other parsers solved this issue

@ematipico
Copy link
Member

ematipico commented Jul 2, 2024

I thought about it more, and I think we should attempt the parsing of declarations first, then fallback to selectors.

Nested selectors are a newer syntax, less spread across the current CSS files. Plus, ignoring how old their grammar is, I believe it's more probable to find declarations inside a selector, not other nested selectors.

@denbezrukov
Copy link
Contributor Author

denbezrukov commented Jul 2, 2024

I thought about it more, and I think we should attempt the parsing of declarations first, then fallback to selectors.

Nested selectors are a newer syntax, less spread across the current CSS files. Plus, ignoring how old their grammar is, I believe it's more probable to find declarations inside a selector, not other nested selectors.

Yeah, I agree. Let’s try to keep this approach. Anyway, I’d like to try comparing the next token after a colon. Actually, it can just be a range comparison and shouldn’t introduce a huge impact in theory.

@denbezrukov
Copy link
Contributor Author

I've checked the Lightlight CSS library, and they have a logic where they first parse as a declaration and, if it fails, they parse as a rule. I'd suggest choosing this approach.

https://github.com/servo/rust-cssparser/blob/b75ce6a8df2dbd712fac9d49ba38ee09b96d0d52/src/rules_and_declarations.rs#L263-L300

@ematipico
Copy link
Member

Looks good to me!

@denbezrukov denbezrukov merged commit 40727ca into main Jul 7, 2024
14 of 15 checks passed
@denbezrukov denbezrukov deleted the fix/css-pseudo-classes branch July 7, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Formatter Area: formatter A-Parser Area: parser L-CSS Language: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 The CSS parser doesn't support nested selectors with pseudo-classes
2 participants