-
-
Notifications
You must be signed in to change notification settings - Fork 792
fix(format/html): preserve whitespace between a HtmlSingleTextExpression/HtmlElement and HtmlContent #8595
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
Conversation
🦋 Changeset detectedLatest commit: 72a6d17 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 |
WalkthroughPreserves trailing whitespace/newlines after embedded HTML content and expressions. Implements trailing-trivia inspection in Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@dyc3 This should also apply to regular HTML elements. Apart from parity with Prettier, this quite literally isn't the same code, it renders differently in the browser.
|
ematipico
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should add a test for interpolation too {{ }}
|
@Carbophile Good catch, thanks. |
…ion/HtmlElement and HtmlContent
b994a24 to
cdecc99
Compare
crates/biome_html_formatter/tests/specs/prettier/html/whitespace/inline-nodes.html.snap
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/biome_html_formatter/src/utils/metadata.rs (1)
812-826: Logic looks solid; consider extracting the name extraction pattern.The function correctly follows the existing pattern and handles errors gracefully. However, the name extraction logic (matching on
AnyHtmlElementvariants and extracting the tag name) is duplicated fromis_element_whitespace_sensitive_from_element(lines 778-794).If this pattern is used elsewhere or might be extended, consider extracting a helper like
extract_tag_name(element: &AnyHtmlElement) -> Option<HtmlTagName>to reduce duplication.🔎 Suggested refactor to reduce duplication
+/// Extracts the tag name from an HTML element. +fn extract_tag_name(element: &AnyHtmlElement) -> Option<HtmlTagName> { + match element { + AnyHtmlElement::HtmlElement(element) => { + element.opening_element().and_then(|element| element.name()).ok() + } + AnyHtmlElement::HtmlSelfClosingElement(element) => element.name().ok(), + _ => None, + } +} + pub(crate) fn is_element_whitespace_sensitive_from_element( f: &HtmlFormatter, element: &AnyHtmlElement, ) -> bool { - let name = match element { - AnyHtmlElement::HtmlElement(element) => { - element.opening_element().and_then(|element| element.name()) - } - AnyHtmlElement::HtmlSelfClosingElement(element) => element.name(), - _ => return false, - }; - let Ok(name) = name else { + let Some(name) = extract_tag_name(element) else { return false; }; is_element_whitespace_sensitive(f, &name) } pub(crate) fn is_inline_element_from_element(element: &AnyHtmlElement) -> bool { - let name = match element { - AnyHtmlElement::HtmlElement(element) => { - element.opening_element().and_then(|element| element.name()) - } - AnyHtmlElement::HtmlSelfClosingElement(element) => element.name(), - _ => return false, - }; - let Ok(name) = name else { + let Some(name) = extract_tag_name(element) else { return false; }; is_inline_element(&name) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
crates/biome_html_formatter/tests/specs/html/svelte/each_nested.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/html/whitespace/preserve-space-after-element.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/aurelia/basic.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/openging-at-end.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/tags/seach.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/tags/tags.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/whitespace/break-tags.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/whitespace/inline-leading-trailing-spaces.html.snapis excluded by!**/*.snapand included by**crates/biome_html_formatter/tests/specs/prettier/html/whitespace/inline-nodes.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/evil-fans-live.mdcrates/biome_html_formatter/src/html/lists/element_list.rscrates/biome_html_formatter/src/utils/children.rscrates/biome_html_formatter/src/utils/metadata.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/evil-fans-live.md
- crates/biome_html_formatter/src/utils/children.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use inline rustdoc documentation for rules, assists, and their options
Use thedbg!()macro for debugging output in Rust tests and code
Use doc tests (doctest) format with code blocks in rustdoc comments; ensure assertions pass in tests
Files:
crates/biome_html_formatter/src/utils/metadata.rscrates/biome_html_formatter/src/html/lists/element_list.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: dyc3
Repo: biomejs/biome PR: 8291
File: crates/biome_html_formatter/tests/specs/prettier/vue/html-vue/elastic-header.html:10-10
Timestamp: 2025-12-04T13:29:49.287Z
Learning: Files under `crates/biome_html_formatter/tests/specs/prettier` are test fixtures synced from Prettier and should not receive detailed code quality reviews (e.g., HTTP vs HTTPS, formatting suggestions, etc.). These files are test data meant to validate formatter behavior and should be preserved as-is.
📚 Learning: 2025-11-24T18:06:03.545Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:03.545Z
Learning: Applies to crates/biome_parser/**/src/**/*.rs : Use `ParseSeparatedList` and `ParseNodeList` for parsing lists with error recovery to avoid infinite loops
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Define `FormatHtmlSyntaxNode` struct in a `cst.rs` file implementing `FormatRule<HtmlSyntaxNode>`, `AsFormat<HtmlFormatContext>`, and `IntoFormat<HtmlFormatContext>` traits using the provided boilerplate code
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
📚 Learning: 2025-11-24T18:05:27.810Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.810Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Import the `FormatNode` trait and implement it for your Node when creating formatters in biome_js_formatter
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Applies to crates/biome_formatter/**/biome_*_formatter/src/context.rs : Define `<Language>FormatContext` struct in a `context.rs` file containing `comments` and `source_map` fields, implementing `FormatContext` and `CstFormatContext` traits
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Applies to crates/biome_formatter/**/biome_*_formatter/src/lib.rs : Expose a public `format_node` function that accepts formatting options and a root syntax node, returning a `FormatResult<Formatted<Context>>` with appropriate documentation
Applied to files:
crates/biome_html_formatter/src/html/lists/element_list.rs
🧬 Code graph analysis (1)
crates/biome_html_formatter/src/html/lists/element_list.rs (2)
crates/biome_html_formatter/src/utils/metadata.rs (2)
is_element_whitespace_sensitive_from_element(778-794)is_inline_element_from_element(813-826)crates/biome_formatter/src/format_extensions.rs (1)
memoized(59-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: End-to-end tests
- GitHub Check: Test Node.js API
- GitHub Check: autofix
🔇 Additional comments (1)
crates/biome_html_formatter/src/html/lists/element_list.rs (1)
13-13: Excellent fix for inline element handling!The changes correctly distinguish inline elements from block elements to preserve whitespace and prevent unnecessary line breaks. The rationale is well-documented in the comments, and the logic is consistent across both call sites (force_multiline and children_meta).
This should resolve the issue where inline elements like
<a>or<span>were incorrectly forcing multiline layout and potentially removing rendered spaces.Note: The
HTML_INLINE_TAGSarray inmetadata.rshas a TODO indicating it's incomplete (line 19). Whilst the current list covers common inline elements (a,span,b,i,strong, etc.), you might want to verify that all critical inline elements for your use cases are included.Also applies to: 508-515, 570-575

Summary
I threw opus at the problem and somehow, it actually did it. Usually AI can't reason about the formatter too well.
fixes #8584
Test Plan
snapshots
Docs