feat(css): add support for @container scroll-state at-rule and associated queries#9273
feat(css): add support for @container scroll-state at-rule and associated queries#9273denbezrukov wants to merge 4 commits intomainfrom
@container scroll-state at-rule and associated queries#9273Conversation
🦋 Changeset detectedLatest commit: 6e3154b The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
|
Does this PR fix #9253? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughThis PR adds comprehensive support for CSS Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss (1)
1-9: Good test coverage, but consider adding a named container case.The PR description mentions support for
@container main-layout scroll-state(not ((stuck) and (scrolled: bottom))). Consider adding a test case with a named container to ensure that path is exercised.The Stylelint empty block warnings are expected for parser test fixtures.
💡 Optional: Add named container test case
`@container` scroll-state((stuck) or (scrolled: bottom)) { } + +@container main-layout scroll-state(stuck) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss` around lines 1 - 9, Add a test line that exercises a named container variant of the scroll-state at-rule, e.g. a rule using a named container identifier like "main-layout" together with a nested negation expression such as not ((stuck) and (scrolled: bottom)); update the test file containing the existing `@container` rules (look for the lines with scroll-state(...) variants) by adding a similar empty block that uses the named container to ensure the parser covers the path mentioned in the PR description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xtask/codegen/css.ungram`:
- Around line 976-980: The grammar change adding
CssContainerScrollStateQueryInParens (with name: CssIdentifier and
AnyCssContainerScrollStateQuery in parens) requires regenerating parser
artifacts; run the codegen step (just gen-grammar css) locally before updating
the PR and include the generated outputs so the build/tests reflect the new
.ungram change—ensure the generated code is committed alongside the .ungram edit
so container/mod.rs’s runtime validation remains in sync.
---
Nitpick comments:
In
`@crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss`:
- Around line 1-9: Add a test line that exercises a named container variant of
the scroll-state at-rule, e.g. a rule using a named container identifier like
"main-layout" together with a nested negation expression such as not ((stuck)
and (scrolled: bottom)); update the test file containing the existing `@container`
rules (look for the lines with scroll-state(...) variants) by adding a similar
empty block that uses the named container to ensure the parser covers the path
mentioned in the PR description.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
crates/biome_css_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_formatter/tests/specs/css/scss/at-rule/container-scroll-state.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/at_rule/at_rule_container/at_rule_container_and_query_error.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/at_rule/at_rule_container/at_rule_container_error.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/at_rule/at_rule_container/at_rule_container_or_query_error.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/container-scroll-state.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/at_rule/at_rule_container.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (21)
crates/biome_css_formatter/src/css/any/container_query_in_parens.rscrates/biome_css_formatter/src/css/any/container_scroll_state_and_combinable_query.rscrates/biome_css_formatter/src/css/any/container_scroll_state_in_parens.rscrates/biome_css_formatter/src/css/any/container_scroll_state_or_combinable_query.rscrates/biome_css_formatter/src/css/any/container_scroll_state_query.rscrates/biome_css_formatter/src/css/any/mod.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_and_query.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_in_parens.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_not_query.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_or_query.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_query_in_parens.rscrates/biome_css_formatter/src/css/auxiliary/mod.rscrates/biome_css_formatter/src/generated.rscrates/biome_css_formatter/tests/specs/css/scss/at-rule/container-scroll-state.scsscrates/biome_css_parser/src/syntax/at_rule/container/error.rscrates/biome_css_parser/src/syntax/at_rule/container/mod.rscrates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/container-scroll-state.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scsscrates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rsxtask/codegen/css.ungramxtask/codegen/src/css_kinds_src.rs
| CssContainerScrollStateQueryInParens = | ||
| name: CssIdentifier | ||
| '(' | ||
| query: AnyCssContainerScrollStateQuery | ||
| ')' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n "scroll.?state" --type rust crates/biome_css_parser/src/ -iRepository: biomejs/biome
Length of output: 7793
🏁 Script executed:
# Look for where CssContainerScrollStateQueryInParens is used/validated
rg -n "CssContainerScrollStateQueryInParens" crates/biome_css_parser/src/Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Check the parser rule for container scroll-state queries
rg -n "parse.*scroll.*state\|scroll.*state.*parse" crates/biome_css_parser/src/ -iRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look at the grammar generation to understand the scope
fd "*.ungram" --type fRepository: biomejs/biome
Length of output: 344
🏁 Script executed:
fd --glob "*.ungram" --type fRepository: biomejs/biome
Length of output: 389
🏁 Script executed:
# Look at the specific validation on line 208
sed -n '207,210p' crates/biome_css_parser/src/syntax/at_rule/container/mod.rsRepository: biomejs/biome
Length of output: 208
Run just gen-grammar css before opening the PR.
The .ungram file changes to grammar require code generation. (Also, the name: CssIdentifier design is intentional—the parser validates that only scroll-state is accepted at runtime on line 208 of container/mod.rs, so this is fine.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtask/codegen/css.ungram` around lines 976 - 980, The grammar change adding
CssContainerScrollStateQueryInParens (with name: CssIdentifier and
AnyCssContainerScrollStateQuery in parens) requires regenerating parser
artifacts; run the codegen step (just gen-grammar css) locally before updating
the PR and include the generated outputs so the build/tests reflect the new
.ungram change—ensure the generated code is committed alongside the .ungram edit
so container/mod.rs’s runtime validation remains in sync.
Merging this PR will not alter performance
Comparing Footnotes
|
6661fd1 to
49fd6b1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss (1)
1-9: Consider one more OK fixture for named containers withscroll-state(...).These cases are good; adding one
@container <name> scroll-state(...)sample (ideally withsnapped: x|y) would lock that parser path too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss` around lines 1 - 9, Add an additional OK fixture that exercises the named-container parsing path by using a named container before the scroll-state condition; e.g. include a rule with the pattern "@container my-container scroll-state(snapped: x)" (or "snapped: y") so the parser sees a named identifier then scroll-state(...); ensure the new case mirrors the style of existing fixtures (spacing/curly braces) and uses "snapped" as the property to cover that variant of scroll-state.crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/container-scroll-state.scss (1)
1-5: Add one invalid case for wrong function name.Given the grammar allows an identifier before
(...), adding something like@container foo(stuck) { }would guard the runtimescroll-statename validation against regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/container-scroll-state.scss` around lines 1 - 5, Add an additional invalid test case that uses a wrong function/identifier before the parentheses to ensure runtime validation catches non-scroll-state names; specifically add an `@container` rule like "@container foo(stuck) { }" alongside the existing `@container` scroll-state(...) cases so the parser/runtime will reject a non-"scroll-state" identifier.crates/biome_css_parser/src/syntax/at_rule/container/mod.rs (1)
219-221: Useexpectfor the mandatory(in this rule.Line 220 currently uses
p.bump(T!['(']); switching top.expect(T!['('])keeps required-token diagnostics consistent with the rest of the parser.Suggested patch
- p.bump(T!['(']); + p.expect(T!['(']);Based on learnings: Use
p.eat(token)for optional tokens,p.expect(token)for required tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/at_rule/container/mod.rs` around lines 219 - 221, Replace the mandatory-token call to p.bump(T!['(']) with p.expect(T!['(']) so the parser emits the same required-token diagnostics used elsewhere; in the sequence inside the function that calls parse_regular_identifier(p) and then parses the container query, ensure you call p.expect(T!['(']) before calling parse_any_container_scroll_state_query(p) to enforce the required '(' and produce a consistent diagnostic on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_css_parser/src/syntax/at_rule/container/mod.rs`:
- Around line 219-221: Replace the mandatory-token call to p.bump(T!['(']) with
p.expect(T!['(']) so the parser emits the same required-token diagnostics used
elsewhere; in the sequence inside the function that calls
parse_regular_identifier(p) and then parses the container query, ensure you call
p.expect(T!['(']) before calling parse_any_container_scroll_state_query(p) to
enforce the required '(' and produce a consistent diagnostic on failure.
In
`@crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/container-scroll-state.scss`:
- Around line 1-5: Add an additional invalid test case that uses a wrong
function/identifier before the parentheses to ensure runtime validation catches
non-scroll-state names; specifically add an `@container` rule like "@container
foo(stuck) { }" alongside the existing `@container` scroll-state(...) cases so the
parser/runtime will reject a non-"scroll-state" identifier.
In
`@crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss`:
- Around line 1-9: Add an additional OK fixture that exercises the
named-container parsing path by using a named container before the scroll-state
condition; e.g. include a rule with the pattern "@container my-container
scroll-state(snapped: x)" (or "snapped: y") so the parser sees a named
identifier then scroll-state(...); ensure the new case mirrors the style of
existing fixtures (spacing/curly braces) and uses "snapped" as the property to
cover that variant of scroll-state.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
crates/biome_css_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_formatter/tests/specs/css/scss/at-rule/container-scroll-state.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/at_rule/at_rule_container/at_rule_container_and_query_error.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/at_rule/at_rule_container/at_rule_container_error.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/at_rule/at_rule_container/at_rule_container_or_query_error.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/container-scroll-state.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/at_rule/at_rule_container.css.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (21)
crates/biome_css_formatter/src/css/any/container_query_in_parens.rscrates/biome_css_formatter/src/css/any/container_scroll_state_and_combinable_query.rscrates/biome_css_formatter/src/css/any/container_scroll_state_in_parens.rscrates/biome_css_formatter/src/css/any/container_scroll_state_or_combinable_query.rscrates/biome_css_formatter/src/css/any/container_scroll_state_query.rscrates/biome_css_formatter/src/css/any/mod.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_and_query.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_in_parens.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_not_query.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_or_query.rscrates/biome_css_formatter/src/css/auxiliary/container_scroll_state_query_in_parens.rscrates/biome_css_formatter/src/css/auxiliary/mod.rscrates/biome_css_formatter/src/generated.rscrates/biome_css_formatter/tests/specs/css/scss/at-rule/container-scroll-state.scsscrates/biome_css_parser/src/syntax/at_rule/container/error.rscrates/biome_css_parser/src/syntax/at_rule/container/mod.rscrates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/container-scroll-state.scsscrates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/container-scroll-state.scsscrates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rsxtask/codegen/css.ungramxtask/codegen/src/css_kinds_src.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/biome_css_formatter/src/css/auxiliary/mod.rs
- crates/biome_css_formatter/src/css/any/container_scroll_state_in_parens.rs
- crates/biome_css_formatter/src/css/any/container_scroll_state_or_combinable_query.rs
- crates/biome_css_formatter/src/css/auxiliary/container_scroll_state_in_parens.rs
- crates/biome_css_formatter/src/css/any/container_scroll_state_query.rs
- crates/biome_css_formatter/src/css/any/container_scroll_state_and_combinable_query.rs
- crates/biome_css_formatter/src/css/auxiliary/container_scroll_state_and_query.rs
- crates/biome_css_formatter/src/css/auxiliary/container_scroll_state_not_query.rs
- crates/biome_css_formatter/src/css/auxiliary/container_scroll_state_query_in_parens.rs
- crates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rs
I guess this is a follow-up, because we didn't fully support the new syntax. |
dyc3
left a comment
There was a problem hiding this comment.
I think there should be a changeset because your last example produces a parsing error on main, and this PR fixes it.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.changeset/fix-container-scroll-state-scss.md (1)
1-14: Question: should the filename omit "scss"?The filename includes "scss" but the feature and examples are CSS-based (
@container scroll-state). Unless there's a specific SCSS aspect to this fix, consider renaming tofix-container-scroll-state.mdfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/fix-container-scroll-state-scss.md around lines 1 - 14, The changeset filename contains "scss" but the fix targets CSS `@container scroll-state` parsing; rename the changeset file from ".changeset/fix-container-scroll-state-scss.md" to ".changeset/fix-container-scroll-state.md" (or equivalent) so the filename accurately reflects the CSS feature being fixed and avoid implying a SCSS-specific change; update any references to the old filename in the repository (e.g., changelog generation or CI scripts) to point to the new name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/fix-container-scroll-state-scss.md:
- Around line 1-14: The changeset filename contains "scss" but the fix targets
CSS `@container scroll-state` parsing; rename the changeset file from
".changeset/fix-container-scroll-state-scss.md" to
".changeset/fix-container-scroll-state.md" (or equivalent) so the filename
accurately reflects the CSS feature being fixed and avoid implying a
SCSS-specific change; update any references to the old filename in the
repository (e.g., changelog generation or CI scripts) to point to the new name.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_css_formatter/tests/specs/css/atrule/container.css.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (1)
.changeset/fix-container-scroll-state-scss.md
Summary
Added support for the CSS
@container scroll-state(...)at-rule and associated queries according to the [CSS Conditional 5 specification](https://drafts.csswg.org/css-conditional-5/#scroll-state-container).I haven't added changeset since the changes shouldn't change the formatter or lint rules.
Examples of syntax:
Test Plan
cargo test -p biome_css_formatter
cargo test -p biome_css_parser