fix(format/html): format class and style attributes#10159
Draft
dyc3 wants to merge 2 commits into
Draft
Conversation
🦋 Changeset detectedLatest commit: 5fb8241 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 |
github-actions
Bot
added
A-Core
Contributor
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
Contributor
WalkthroughAdds special handling for HTML Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/wise-pigs-tell.md:
- Line 5: Update the changeset entry text to use the bug-fix format and include
an issue link like the other changeset in this PR: replace the current sentence
about HTML style attributes with a line starting with "Fixed
[#<ISSUE_NUMBER>](<ISSUE_URL>):" followed by a short description (e.g., "style
attributes now treated as embedded CSS so noUnknownProperty works"). Ensure the
link uses the same markdown style as the other changeset in the PR and keep the
rest of the file unchanged.
In
`@crates/biome_html_formatter/src/html/auxiliary/attribute_initializer_clause.rs`:
- Around line 196-303: The hand-rolled splitter in the (_, Some("style")) branch
(the declarations variable, format_declaration closure, and
JoinStyleDeclarations) corrupts valid CSS (e.g., using trimmed lengths to slice
properties and splitting on raw ';' / ':'); replace this manual parsing logic by
invoking the existing embedded-CSS parser/formatter used elsewhere in the
project instead of splitting html_string.inner_string_text(), and format the
attribute by passing the original value_token (or its source range) into that
parser/formatter so token ranges are preserved rather than deriving slices from
trimmed lengths; remove the declarations/map/split logic and the
format_declaration closure and ensure the output is written using the same
formatting primitives (group, token("\""), fmt_eq_token,
format_removed(&value_token), etc.) while preserving multiline handling via the
embedded CSS formatter.
In `@crates/biome_js_analyze/src/lint/nursery/use_sorted_classes/sort.rs`:
- Around line 218-220: The code consumes the only class token by calling next()
then next_back() on class_iter, so single-class names yield last_class_len == 0;
fix by collecting the split_whitespace iterator into a Vec (e.g., let parts:
Vec<_> = class_name.text().split_whitespace().collect();) and compute
first_class_len = parts.first().map_or(0, |s| s.len()) as u32 and last_class_len
= parts.last().map_or(0, |s| s.len()) as u32; apply the same change to the other
occurrence around lines 225-226 (replace use of class_iter, first_class_len,
last_class_len with parts.first()/parts.last()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 514687a7-b192-4b30-a588-1a75653d36a6
⛔ Files ignored due to path filters (12)
crates/biome_css_analyze/tests/specs/correctness/noUnknownProperty/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/html/attributes/style.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/html/class-attr.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/html/vue/v-bind-style.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/attributes/attributes.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/attributes/class-bem1.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/attributes/class-names.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/attributes/style.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/doctype_declarations/xhtml1.1.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/tags/marquee.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/whitespace/fill.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/whitespace/nested-inline-without-whitespace.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (17)
.changeset/purple-phones-film.md.changeset/wise-pigs-tell.mdcrates/biome_css_analyze/tests/spec_tests.rscrates/biome_css_analyze/tests/specs/correctness/noUnknownProperty/invalid.htmlcrates/biome_html_formatter/src/html/auxiliary/attribute_initializer_clause.rscrates/biome_html_formatter/tests/specs/html/attributes/style.htmlcrates/biome_html_formatter/tests/specs/html/class-attr.htmlcrates/biome_html_formatter/tests/specs/html/vue/v-bind-style.vuecrates/biome_js_analyze/src/lint/nursery/use_sorted_classes/sort.rscrates/biome_rowan/src/token_text.rscrates/biome_service/src/embed/detector.rscrates/biome_service/src/embed/registry.rscrates/biome_service/src/embed/types.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/html/parse_embedded_nodes.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_service/src/workspace/document/mod.rs
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| In HTML, `style` attributes are now considered to contain embedded CSS in their values. Rules like [`noUnknownProperty`](https://biomejs.dev/linter/rules/no-unknown-property/) will now work in these areas. |
Contributor
There was a problem hiding this comment.
Please link the related bug in this changeset entry.
Use the bug-fix format with an issue link (same style as the other changeset in this PR) to keep release notes consistent.
Suggested edit
-In HTML, `style` attributes are now considered to contain embedded CSS in their values. Rules like [`noUnknownProperty`](https://biomejs.dev/linter/rules/no-unknown-property/) will now work in these areas.
+Fixed [`#5242`](https://github.com/biomejs/biome/issues/5242): In HTML, `style` attributes are now considered to contain embedded CSS in their values. Rules like [`noUnknownProperty`](https://biomejs.dev/linter/rules/no-unknown-property/) now work in these areas.As per coding guidelines, “For changeset descriptions of bug fixes, reference the issue with a link (e.g., ‘Fixed #4444’).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| In HTML, `style` attributes are now considered to contain embedded CSS in their values. Rules like [`noUnknownProperty`](https://biomejs.dev/linter/rules/no-unknown-property/) will now work in these areas. | |
| Fixed [`#5242`](https://github.com/biomejs/biome/issues/5242): In HTML, `style` attributes are now considered to contain embedded CSS in their values. Rules like [`noUnknownProperty`](https://biomejs.dev/linter/rules/no-unknown-property/) now work in these areas. |
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: The verb ‘considered’ is used with the gerund form.
Context: ...-- In HTML, style attributes are now considered to contain embedded CSS in their values. Rules lik...
(ADMIT_ENJOY_VB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/wise-pigs-tell.md at line 5, Update the changeset entry text to
use the bug-fix format and include an issue link like the other changeset in
this PR: replace the current sentence about HTML style attributes with a line
starting with "Fixed [#<ISSUE_NUMBER>](<ISSUE_URL>):" followed by a short
description (e.g., "style attributes now treated as embedded CSS so
noUnknownProperty works"). Ensure the link uses the same markdown style as the
other changeset in the PR and keep the rest of the file unchanged.
Comment on lines
+196
to
+303
| (_, Some("style")) => { | ||
| let content = html_string.inner_string_text()?; | ||
| let value_token = html_string.value_token()?; | ||
| let declarations = content | ||
| .split(';') | ||
| .map(TokenText::trim_token) | ||
| .filter(|declaration| !declaration.is_empty()) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| if declarations.iter().any(|declaration| { | ||
| !declaration.text().contains(':') | ||
| || declaration.text().contains(['{', '}']) | ||
| }) { | ||
| return write!(f, [fmt_eq_token, value.format()]); | ||
| } | ||
|
|
||
| struct JoinStyleDeclarations { | ||
| multiline: bool, | ||
| } | ||
|
|
||
| impl Format<HtmlFormatContext> for JoinStyleDeclarations { | ||
| fn fmt(&self, f: &mut HtmlFormatter) -> FormatResult<()> { | ||
| if self.multiline { | ||
| write!(f, [token(";"), hard_line_break()]) | ||
| } else { | ||
| write!(f, [token(";"), space()]) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let format_declaration = |declaration: TokenText| { | ||
| let value_token = value_token.clone(); | ||
| format_with(move |f| { | ||
| let Some((property, _)) = declaration.text().split_once(':') | ||
| else { | ||
| return Ok(()); | ||
| }; | ||
| let property = declaration | ||
| .clone() | ||
| .slice(TextRange::new( | ||
| TextSize::from(0), | ||
| TextSize::try_from(property.len()).unwrap_or_default(), | ||
| )) | ||
| .trim_token(); | ||
| let value_start = property.text_len() + TextSize::from(1); | ||
| let value = declaration | ||
| .clone() | ||
| .slice(TextRange::new(value_start, declaration.text_len())) | ||
| .trim_token(); | ||
| write!( | ||
| f, | ||
| [ | ||
| located_token_text( | ||
| &value_token, | ||
| property.source_range(value_token.text_range()) | ||
| ), | ||
| token(":"), | ||
| space(), | ||
| located_token_text( | ||
| &value_token, | ||
| value.source_range(value_token.text_range()) | ||
| ) | ||
| ] | ||
| ) | ||
| }) | ||
| }; | ||
|
|
||
| let multiline = declarations.len() > 2; | ||
|
|
||
| write!( | ||
| f, | ||
| [ | ||
| fmt_eq_token, | ||
| format_removed(&value_token), | ||
| token("\""), | ||
| group(&format_with(|f| { | ||
| if multiline { | ||
| write!( | ||
| f, | ||
| [block_indent(&format_with(|f| { | ||
| f.join_with(JoinStyleDeclarations { | ||
| multiline, | ||
| }) | ||
| .entries( | ||
| declarations | ||
| .iter() | ||
| .cloned() | ||
| .map(format_declaration), | ||
| ) | ||
| .finish()?; | ||
| write!(f, [token(";")]) | ||
| }))] | ||
| ) | ||
| } else { | ||
| f.join_with(JoinStyleDeclarations { multiline }) | ||
| .entries( | ||
| declarations | ||
| .iter() | ||
| .cloned() | ||
| .map(format_declaration), | ||
| ) | ||
| .finish() | ||
| } | ||
| })), | ||
| token("\"") | ||
| ] | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
This hand-rolled CSS splitter will corrupt valid style values.
Line 240 derives the value slice from the trimmed property length, so style="color : red" is reformatted as color: : red. More broadly, splitting on raw ; / : will also mis-handle quoted strings and similar valid CSS. Since this PR already wires style attributes through embedded CSS parsing elsewhere, this formatter path needs to reuse that parser/formatter instead of token slicing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/biome_html_formatter/src/html/auxiliary/attribute_initializer_clause.rs`
around lines 196 - 303, The hand-rolled splitter in the (_, Some("style"))
branch (the declarations variable, format_declaration closure, and
JoinStyleDeclarations) corrupts valid CSS (e.g., using trimmed lengths to slice
properties and splitting on raw ';' / ':'); replace this manual parsing logic by
invoking the existing embedded-CSS parser/formatter used elsewhere in the
project instead of splitting html_string.inner_string_text(), and format the
attribute by passing the original value_token (or its source range) into that
parser/formatter so token ranges are preserved rather than deriving slices from
trimmed lengths; remove the declarations/map/split logic and the
format_declaration closure and ensure the output is written using the same
formatting primitives (group, token("\""), fmt_eq_token,
format_removed(&value_token), etc.) while preserving multiline handling via the
embedded CSS formatter.
Comment on lines
+218
to
220
| let mut class_iter = class_name.text().split_whitespace(); | ||
| let first_class_len = class_iter.next().map_or(0, |s| s.len()) as u32; | ||
| let last_class_len = class_iter.next_back().map_or(0, |s| s.len()) as u32; |
Contributor
There was a problem hiding this comment.
Single-class postfix case is miscomputed.
Line 219 consumes the only item before Line 220 runs next_back(). So for foo${bar} (ignore_postfix == true), last_class_len becomes 0, and the sortable range is wrong.
Suggested fix
- let mut class_iter = class_name.text().split_whitespace();
- let first_class_len = class_iter.next().map_or(0, |s| s.len()) as u32;
- let last_class_len = class_iter.next_back().map_or(0, |s| s.len()) as u32;
+ let first_class_len = class_name
+ .text()
+ .split_whitespace()
+ .next()
+ .map_or(0, |s| s.len()) as u32;
+ let last_class_len = class_name
+ .text()
+ .split_whitespace()
+ .next_back()
+ .map_or(0, |s| s.len()) as u32;Also applies to: 225-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_js_analyze/src/lint/nursery/use_sorted_classes/sort.rs` around
lines 218 - 220, The code consumes the only class token by calling next() then
next_back() on class_iter, so single-class names yield last_class_len == 0; fix
by collecting the split_whitespace iterator into a Vec (e.g., let parts: Vec<_>
= class_name.text().split_whitespace().collect();) and compute first_class_len =
parts.first().map_or(0, |s| s.len()) as u32 and last_class_len =
parts.last().map_or(0, |s| s.len()) as u32; apply the same change to the other
occurrence around lines 225-226 (replace use of class_iter, first_class_len,
last_class_len with parts.first()/parts.last()).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_css_analyze/tests/spec_tests.rs (1)
59-104: Consider keeping action-validation parity for HTML tests.The HTML branch skips the
check_code_actionpath used by non-HTML tests, so mutation integrity for quick-fixes in HTML-embedded CSS is not validated here. Worth aligning later to avoid blind spots as more fixable rules support HTML inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_analyze/tests/spec_tests.rs` around lines 59 - 104, The HTML branch currently only calls analyze_with_workspace and skips the check_code_action pathway exercised by the non-HTML branch; update the HTML branch so it runs the same action-validation path as non-HTML tests — either invoke the existing check_code_action helper or call analyze_and_snap with the same parameters (including CheckActionType::Lint and parser options) for the HTML input (and for each script returned by scripts_from_json if applicable) so mutation/quick-fix validation is performed for embedded CSS as well; adjust calls to use analyze_with_workspace, analyze_and_snap, CheckActionType::Lint, and any parser_options/source_type logic consistently with the non-HTML branch.
🤖 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_analyze/tests/spec_tests.rs`:
- Around line 59-104: The HTML branch currently only calls
analyze_with_workspace and skips the check_code_action pathway exercised by the
non-HTML branch; update the HTML branch so it runs the same action-validation
path as non-HTML tests — either invoke the existing check_code_action helper or
call analyze_and_snap with the same parameters (including CheckActionType::Lint
and parser options) for the HTML input (and for each script returned by
scripts_from_json if applicable) so mutation/quick-fix validation is performed
for embedded CSS as well; adjust calls to use analyze_with_workspace,
analyze_and_snap, CheckActionType::Lint, and any parser_options/source_type
logic consistently with the non-HTML branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e89a4160-36fa-4089-a726-3b37aada2d68
📒 Files selected for processing (1)
crates/biome_css_analyze/tests/spec_tests.rs
Merging this PR will degrade performance by 6.68%
Performance Changes
Comparing 
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR does 2 things:
classattributes to match prettier by splitting by whitespace and re-printing them with a single space between themstyleattribute values from just a string value to being considered embedded CSS.I had gpt 5.5 do the plumbing to make the style attribute value an embedded language, but I wrote the class attribute formatting stuff myself.
I'm not sure if I like having the specific
EmbedDetector::StyleAttributevariant instead of just a more genericAttribute, but I don't think we have any other use cases like this. So its probably fine as is.fixes #5242
Test Plan
snapshots
Docs