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_semantic): resolve nested selectors #4658

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/biome_css_semantic/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
mod events;
mod semantic_model;
#[cfg(test)]
mod tests;

pub use events::*;
pub use semantic_model::*;
48 changes: 40 additions & 8 deletions crates/biome_css_semantic/src/semantic_model/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,26 @@ impl SemanticModelBuilder {
.map(|parent| parent.specificity.clone())
.unwrap_or_default();

if let Some(current_rule) = self.current_rule_stack.last() {
let current_rule = self.rules_by_id.get_mut(current_rule).unwrap();
current_rule.selectors.push(Selector {
name,
range,
original,
specificity: parent_specificity + specificity.clone(),
});
let parent_selectors = self
.current_rule_stack
.last()
.and_then(|rule_id| self.rules_by_id.get(rule_id))
.and_then(|rule| rule.parent_id)
.and_then(|parent_id| self.rules_by_id.get(&parent_id))
.map(|parent| parent.selectors.clone())
.unwrap_or_default();

if let Some(current_rule_id) = self.current_rule_stack.last() {
let resolved_selectors = resolve_selector(&name, &parent_selectors);
let current_rule = self.rules_by_id.get_mut(current_rule_id).unwrap();
for name in resolved_selectors.iter() {
current_rule.selectors.push(Selector {
name: name.to_string(),
Comment on lines +123 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for name in resolved_selectors.iter() {
current_rule.selectors.push(Selector {
name: name.to_string(),
for selector_name in resolved_selectors.iter() {
current_rule.selectors.push(Selector {
name: selector_name.to_string(),

name is somewhat ambiguous, and selector_name would improve readability IMHO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a String, we shouldn't create a new one

range,
original: original.clone(),
specificity: parent_specificity.clone() + specificity.clone(),
});
}

current_rule.specificity += specificity;
}
Expand Down Expand Up @@ -173,3 +185,23 @@ impl SemanticModelBuilder {
}
}
}

fn resolve_selector(current: &str, parent: &[Selector]) -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn resolve_selector(current: &str, parent: &[Selector]) -> Vec<String> {
fn resolve_selector(current: &str, parents: &[Selector]) -> Vec<String> {

nit: parents is more accurate since it's a slice of selectors?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that I am late to the party, so I am sorry if this questions come too late.

Is there a particular reason we need to track String inside the semantic model? I wonder if we can avoid that and store only TextRange and syntax nodes (syntax nodes are cheaper than strings) inside the model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there was a flaw in my implementation; I realized that storing SyntaxNode makes things easier.
Fortunately, it's not too late! I've already rewritten it on my local machine, and it's working without any regression. 👍 I'll make a PR for it

let mut resolved = Vec::new();

if parent.is_empty() {
return vec![current.to_string()];
}

for parent in parent {
let parent = parent.name.to_string();
if current.contains('&') {
let current = current.replace('&', &parent);
resolved.push(current);
} else {
resolved.push(format!("{} {}", parent, current));
}
}

resolved
Comment on lines +190 to +206
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut resolved = Vec::new();
if parent.is_empty() {
return vec![current.to_string()];
}
for parent in parent {
let parent = parent.name.to_string();
if current.contains('&') {
let current = current.replace('&', &parent);
resolved.push(current);
} else {
resolved.push(format!("{} {}", parent, current));
}
}
resolved
if parents.is_empty() {
return vec![current.to_string()];
}
parents
.iter()
.map(|parent| {
let parent_name = &parent.name;
if current.contains('&') {
current.replace('&', parent_name)
} else {
format!("{} {}", parent_name, current)
}
})
.collect()

}
12 changes: 7 additions & 5 deletions crates/biome_css_semantic/src/semantic_model/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ mod tests {
assert_eq!(rule.selectors.len(), 1);
assert_eq!(rule.declarations.len(), 1);

assert_eq!(rule.selectors[0].name, ".child");
assert_eq!(rule.selectors[0].name, "p .child");

assert_eq!(rule.declarations[0].property.name, "color");
assert_eq!(rule.declarations[0].value.text, "var(--foo)");
Expand All @@ -261,10 +261,12 @@ mod tests {
#[test]
fn quick_test() {
let parse = parse_css(
r#"@property --item-size {
syntax: "<percentage>";
inherits: true;
initial-value: 40%;
r#".parent {
color: blue;

.child {
color: red;
}
}"#,
CssParserOptions::default(),
);
Expand Down
2 changes: 2 additions & 0 deletions crates/biome_css_semantic/src/tests/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
mod selector;
mod specificity;
Loading