From 486ad058ff50c929f8776b15eb4ebc846d2e1efc Mon Sep 17 00:00:00 2001 From: Ashley Hunter Date: Thu, 20 Aug 2026 15:08:26 +0100 Subject: [PATCH 1/3] fix(styles): scope selectors glued directly to a preceding comment scope_simple_selector bailed out of scoping the whole selector string whenever it contained the %COMMENT% placeholder. That's correct when the placeholder stands alone (a comment on its own line), but when a comment formatter/reprinter removes the whitespace between a comment and the selector that follows it, the placeholder ends up glued directly to real selector text (e.g. `%COMMENT%.foo`) and the whole rule silently loses Emulated encapsulation - a rule like `.foo { ... }` ships with no `[_ngcontent-*]` attribute at all, so it matches (and can visually clobber) any other element with that class name anywhere else in the app, with no error. Peel leading/trailing comment placeholder runs off before scoping the real selector text, then splice them back - they resolve to blank text later in restore_comments, so their exact position doesn't matter. Falls back to the old (unscoped) behavior only when a placeholder sits inside the selector body itself, which is unusual enough not to risk a generic split. --- .../src/styles/encapsulation.rs | 33 ++++++- .../comment_glued_selector_regression_test.rs | 81 +++++++++++++++++ .../tests/shadow_css_test.rs | 88 +++++++++++++++++++ 3 files changed, 200 insertions(+), 2 deletions(-) create mode 100644 crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs diff --git a/crates/oxc_angular_compiler/src/styles/encapsulation.rs b/crates/oxc_angular_compiler/src/styles/encapsulation.rs index 251abfaff..a75e685c4 100644 --- a/crates/oxc_angular_compiler/src/styles/encapsulation.rs +++ b/crates/oxc_angular_compiler/src/styles/encapsulation.rs @@ -2243,9 +2243,38 @@ fn scope_simple_selector(selector: &str, content_attr: &str) -> String { return String::new(); } - // Don't scope comment placeholders + // A comment can end up glued directly to the selector that follows it - + // e.g. when CSS has been reprinted by an upstream formatter (PostCSS) + // that strips the blank line normally separating `/* comment */` from + // the next rule, `extract_comments` leaves behind `%COMMENT%.foo` with + // no separating whitespace. Bailing out unscoped for the *whole* string + // here would silently drop Emulated encapsulation for `.foo` too - so + // peel any leading/trailing placeholder(s) off, scope the real selector + // text, then splice them back. `restore_comments` later turns every + // placeholder into blank text, so their exact position doesn't matter. if selector.contains(COMMENT_PLACEHOLDER) { - return selector.to_string(); + let mut rest = selector; + let mut leading = String::new(); + while let Some(stripped) = rest.strip_prefix(COMMENT_PLACEHOLDER) { + leading.push_str(COMMENT_PLACEHOLDER); + rest = stripped; + } + let mut trailing = String::new(); + while let Some(stripped) = rest.strip_suffix(COMMENT_PLACEHOLDER) { + trailing.push_str(COMMENT_PLACEHOLDER); + rest = stripped; + } + if rest.is_empty() { + // Nothing but comment placeholder(s) - no real selector to scope. + return selector.to_string(); + } + if rest.contains(COMMENT_PLACEHOLDER) { + // A placeholder sits inside the selector body itself (e.g. a + // comment between a class name and a combinator) - too unusual + // to safely split; leave unscoped rather than risk corruption. + return selector.to_string(); + } + return format!("{leading}{}{trailing}", scope_simple_selector(rest, content_attr)); } // Already has the content attribute diff --git a/crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs b/crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs new file mode 100644 index 000000000..d544585a3 --- /dev/null +++ b/crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs @@ -0,0 +1,81 @@ +//! Regression test for a real-world encapsulation bug: a component's CSS +//! rule shipped with no `[_ngcontent-*]` scoping attribute at all, so it +//! matched (and visually clobbered) any element with the same class name +//! anywhere else in the app, with no console error. +//! +//! Root cause: when a Vite consumer preprocesses a component's `styleUrl` +//! file through PostCSS (e.g. for Tailwind) before handing it to this +//! crate's encapsulation pass, PostCSS's AST-based reprint collapses the +//! blank line that normally separates a `/* comment */` from the rule that +//! follows it. A hand-formatted stylesheet like: +//! +//! ```css +//! /* explains why this rule exists */ +//! .widget { +//! width: 9px; +//! } +//! ``` +//! +//! reaches `shim_css_text` as `*/.widget { width: 9px; }` - the comment +//! glued directly to the selector with no separating whitespace. +//! `scope_simple_selector` used to bail out of scoping the *entire* selector +//! string whenever it contained the comment placeholder, so `.widget` +//! shipped unscoped: a plain, globally-matching rule any other component's +//! same-named class could collide with. +//! +//! Fixed in `scope_simple_selector` (`src/styles/encapsulation.rs`): leading +//! and trailing comment placeholders are now peeled off before scoping and +//! spliced back afterwards, instead of aborting scoping altogether. +//! +//! See `tests/shadow_css_test.rs` for the minimal unit-level regression +//! tests (`test_scope_selector_glued_directly_to_*`); this file exercises +//! the same bug against a full stylesheet shape with several unrelated, +//! comment-documented rules - the shape that originally surfaced it. + +use oxc_angular_compiler::styles::finalize_component_style; + +/// A demo stylesheet shaped like the one that surfaced this bug: several +/// unrelated rules, each documented with a `/* comment */` immediately +/// before it, and reproduced here exactly as PostCSS reprints it - with the +/// blank line that would normally separate the comment from the next rule +/// removed. +const CSS_WITH_COMMENTS_GLUED_TO_SELECTORS: &str = "\ +:host { position: relative; overflow: hidden; }\ +/* Contributes width but no height. */\ +.probe { display: grid; height: 0; }\ +:host:has(.widget) { padding-left: 21px; }\ +.wrapper { overflow: auto; position: relative; }\ +/* This rule positions the widget precisely. */\ +.widget { width: 9px; padding: 0 4px; height: 100%; position: absolute; cursor: col-resize; }\ +.widget:hover { background-color: red; }\ +/* Sticky variant used when scrolling. */\ +.container.sticky-header { display: flex; flex-direction: column; }\ +"; + +#[test] +fn comment_glued_selectors_all_stay_scoped() { + let result = finalize_component_style( + CSS_WITH_COMMENTS_GLUED_TO_SELECTORS, + true, + "_ngcontent-%COMP%", + "_nghost-%COMP%", + false, + ); + + for selector in [".probe", ".wrapper", ".widget", ".container.sticky-header"] { + let unscoped_rule_start = format!("{selector} {{"); + assert!( + !result.contains(&unscoped_rule_start), + "`{selector}` lost its Angular content-attribute scoping - a \ + comment glued directly to it (no separating whitespace, as \ + PostCSS reprints it) made `scope_simple_selector` bail out of \ + scoping the whole rule. Full shimmed output:\n{result}" + ); + + let scoped_rule = format!("{selector}[_ngcontent-%COMP%]"); + assert!( + result.contains(&scoped_rule), + "expected `{scoped_rule}` in output, got:\n{result}" + ); + } +} diff --git a/crates/oxc_angular_compiler/tests/shadow_css_test.rs b/crates/oxc_angular_compiler/tests/shadow_css_test.rs index 53b050d47..92af597c1 100644 --- a/crates/oxc_angular_compiler/tests/shadow_css_test.rs +++ b/crates/oxc_angular_compiler/tests/shadow_css_test.rs @@ -1000,6 +1000,94 @@ fn test_scope_first_selector_after_multiple_comments() { assert_css_eq!(shim(css, "contenta"), expected); } +#[test] +fn test_scope_selector_glued_directly_to_preceding_comment() { + // Regression: a comment with NO separating whitespace before the next + // selector - e.g. `*/.foo`. This is not how anyone hand-writes CSS, but + // it's exactly what PostCSS's AST-based reprint produces when it + // processes a `styleUrl` file (Vite's `preprocessCSS`, used for Tailwind, + // runs before this CSS ever reaches `shim_css_text`): it strips the + // blank line that normally separates `/* comment */` from the following + // rule. `.foo` must still get its content attribute; a whole rule + // silently losing Emulated encapsulation - because the selector string + // handed to `scope_simple_selector` was `%COMMENT%.foo`, which the old + // code bailed out of scoping entirely - is how a component's own styles + // leak globally onto any other element sharing that class name, with no + // console error. + let css = "/* comment */.foo { color: red; }"; + let expected = ".foo[contenta] { color: red; }"; + assert_css_eq!(shim(css, "contenta"), expected); +} + +#[test] +fn test_scope_selector_glued_directly_to_multiple_preceding_comments() { + // Same as above, but with two adjacent comments (also produced by + // PostCSS reprinting, e.g. an SCSS partial's license header merged with + // a rule doc-comment) both glued with no whitespace before the selector. + let css = "/* one *//* two */.foo { color: red; }"; + let expected = ".foo[contenta] { color: red; }"; + assert_css_eq!(shim(css, "contenta"), expected); +} + +#[test] +fn test_scope_selector_glued_directly_to_trailing_comment() { + // A comment glued to the END of a selector, immediately before `{`. + let css = ".foo/* comment */{ color: red; }"; + let expected = ".foo[contenta] { color: red; }"; + assert_css_eq!(shim(css, "contenta"), expected); +} + +#[test] +fn test_scope_multiple_rules_after_comment_glued_selectors() { + // The real-world shape that surfaced this bug: several unrelated rules + // in the same stylesheet, each with its explanatory comment glued + // directly to the following selector by PostCSS's reprint, must all + // still scope correctly rather than only the first one. + let css = "/* first */.foo { width: 1px; }/* second */.bar { height: 2px; }"; + let expected = ".foo[contenta] { width: 1px; }.bar[contenta] { height: 2px; }"; + assert_css_eq!(shim(css, "contenta"), expected); +} + +#[test] +fn test_scope_comment_glued_selectors_in_descendant_chain_and_comma_list() { + // The comment only glues to the *first* compound selector in a + // multi-part selector - every part still needs its own attribute. + let descendant = "/* c */.container .tabs-group { color: red; }"; + assert_css_eq!( + shim(descendant, "contenta"), + ".container[contenta] .tabs-group[contenta] { color: red; }" + ); + + let comma_list = "/* c */.a, .b { color: red; }"; + assert_css_eq!(shim(comma_list, "contenta"), ".a[contenta], .b[contenta] { color: red; }"); +} + +#[test] +fn test_scope_comment_glued_to_host_and_host_context() { + let host = "/* c */:host { color: red; }"; + assert_css_eq!(shim_with_host(host, "contenta", "hosta"), "[hosta] { color: red; }"); + + let host_context = "/* c */:host-context(.dark) { color: red; }"; + assert_css_eq!( + shim_with_host(host_context, "contenta", "hosta"), + ".dark[hosta], .dark [hosta] { color: red; }" + ); +} + +#[test] +fn test_interior_comment_placeholder_falls_back_to_unscoped() { + // Documented, intentional limitation: a comment placeholder *inside* a + // selector body (not glued to its start or end, e.g. between a class + // name and a pseudo-class) is too unusual to safely split - the fix + // deliberately leaves this case unscoped rather than risk corrupting the + // selector. This isn't a shape PostCSS's normal reprint produces (which + // is what the fix actually targets); it just must not panic or garble + // the selector. + let css = ".foo/* c */:hover { color: red; }"; + let expected = ".foo:hover { color: red; }"; + assert_css_eq!(shim(css, "contenta"), expected); +} + #[test] fn test_newline_as_descendant_combinator() { // Newline between selectors is a valid CSS descendant combinator From 7a26ece714ac3b7ec805b6081db3a147cfde4190 Mon Sep 17 00:00:00 2001 From: Ashley Hunter Date: Thu, 20 Aug 2026 16:49:15 +0100 Subject: [PATCH 2/3] fix(styles): scope selectors with interior comments, index comment placeholders Extends the glued-comment fix to cover two more ways comment placeholders corrupt encapsulated CSS. A placeholder sitting *inside* a compound selector (`.foo/* c */:hover`, `.foo/* c */.bar`) was left unscoped, which is the same global-leak bug as the leading-comment case: the rule ships with no `[_ngcontent-*]` attribute and clobbers any other component's `.foo`. Angular scopes these, and it takes less code to handle than to special-case, so the interior fallback is gone: strip every placeholder, scope the real selector text, re-emit them in front. This also stops a multi-line interior comment's newline from landing mid-selector, where it silently became a descendant combinator. Separately, placeholders were unindexed, so `restore_comments` mapped them to comments positionally. That only works if every placeholder survives exactly once and in order, and two passes break it: `:host-context()` duplicates its trailing selector text once per generated permutation, and `polyfill-next-selector` discards text. Each duplicate consumed the *next* comment, so a following `/*# sourceMappingURL=... */` was teleported into the middle of a selector and a literal `%COMMENT%` shipped in the output CSS. Placeholders now carry their comment index, making restoration independent of both count and order; an unresolvable index restores to nothing rather than being left in the CSS. The standalone regression test file is folded into shadow_css_test.rs, where it now also exercises the minifying entry point, alongside a stress corpus covering every pass that duplicates or drops selector text. --- .../src/styles/encapsulation.rs | 133 ++++++--- .../comment_glued_selector_regression_test.rs | 81 ----- .../tests/shadow_css_test.rs | 282 ++++++++++++++---- 3 files changed, 318 insertions(+), 178 deletions(-) delete mode 100644 crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs diff --git a/crates/oxc_angular_compiler/src/styles/encapsulation.rs b/crates/oxc_angular_compiler/src/styles/encapsulation.rs index a75e685c4..99e225c58 100644 --- a/crates/oxc_angular_compiler/src/styles/encapsulation.rs +++ b/crates/oxc_angular_compiler/src/styles/encapsulation.rs @@ -26,8 +26,60 @@ //! - `::ng-deep` → removed (deprecated but still supported) //! - Media queries, keyframes, etc. → preserved -/// Placeholder for comments during processing. -const COMMENT_PLACEHOLDER: &str = "%COMMENT%"; +use std::ops::Range; + +/// Comments are replaced by `%COMMENT%` placeholders during processing, +/// where `` is the comment's index in the extracted list. +/// +/// Angular uses a bare `%COMMENT%` here, but that only restores correctly when +/// every placeholder survives exactly once and in order. Neither holds: the +/// `:host-context()` pass duplicates selector text (and any placeholder in it) +/// once per generated permutation, and `polyfill-next-selector` discards text. +/// With an unindexed placeholder each duplicate consumes the *next* comment, +/// so a following `/*# sourceMappingURL=... */` gets teleported into the middle +/// of a selector and a literal `%COMMENT%` is left behind in the shipped CSS. +/// Carrying the index makes restoration independent of both count and order. +const COMMENT_PLACEHOLDER_PREFIX: &str = "%COMMENT"; + +/// The placeholder standing in for `comments[index]`. +fn comment_placeholder(index: usize) -> String { + format!("{COMMENT_PLACEHOLDER_PREFIX}{index}%") +} + +/// Find the comment placeholder at or after `from`, returning its byte range +/// and the comment index it carries. +fn find_comment_placeholder(s: &str, from: usize) -> Option<(Range, usize)> { + let mut at = from; + while let Some(rel) = s[at..].find(COMMENT_PLACEHOLDER_PREFIX) { + let start = at + rel; + let digits_at = start + COMMENT_PLACEHOLDER_PREFIX.len(); + let after = &s[digits_at..]; + let digits = after.bytes().take_while(u8::is_ascii_digit).count(); + if digits > 0 + && after.as_bytes().get(digits) == Some(&b'%') + && let Ok(index) = after[..digits].parse() + { + return Some((start..digits_at + digits + 1, index)); + } + at = digits_at; + } + None +} + +/// Remove every comment placeholder from `s`, returning the cleaned text and +/// the removed placeholders concatenated in their original order. +fn strip_comment_placeholders(s: &str) -> (String, String) { + let mut cleaned = String::with_capacity(s.len()); + let mut removed = String::new(); + let mut at = 0; + while let Some((range, _)) = find_comment_placeholder(s, at) { + cleaned.push_str(&s[at..range.start]); + at = range.end; + removed.push_str(&s[range]); + } + cleaned.push_str(&s[at..]); + (cleaned, removed) +} // Polyfill host markers (matching Angular's shadow_css.ts) const POLYFILL_HOST: &str = "-shadowcsshost"; @@ -354,6 +406,7 @@ fn extract_comments(css: &str) -> (String, Vec) { trimmed.starts_with('#') && trimmed[1..].trim_start().starts_with("source") }; + let index = comments.len(); if is_sourcemap { comments.push(comment.to_string()); } else { @@ -368,7 +421,7 @@ fn extract_comments(css: &str) -> (String, Vec) { comments.push(preserved); } - result.push_str(COMMENT_PLACEHOLDER); + result.push_str(&comment_placeholder(index)); } else { i += push_utf8_char(&mut result, css, i); } @@ -378,18 +431,22 @@ fn extract_comments(css: &str) -> (String, Vec) { } /// Restore comments from placeholders. +/// +/// Every placeholder carries the index of the comment it stands for, so a +/// placeholder that got duplicated (`:host-context()` permutations) restores to +/// the same comment as its twin, and one that got dropped shifts nothing. Any +/// placeholder with an out-of-range index resolves to nothing rather than being +/// left in the output - a literal `%COMMENT%` must never reach shipped CSS. fn restore_comments(css: &str, comments: &[String]) -> String { - let mut result = css.to_string(); - let mut idx = 0; + let mut result = String::with_capacity(css.len()); + let mut at = 0; - while result.find(COMMENT_PLACEHOLDER).is_some() { - if idx < comments.len() { - result = result.replacen(COMMENT_PLACEHOLDER, &comments[idx], 1); - idx += 1; - } else { - break; - } + while let Some((range, index)) = find_comment_placeholder(css, at) { + result.push_str(&css[at..range.start]); + result.push_str(comments.get(index).map_or("", String::as_str)); + at = range.end; } + result.push_str(&css[at..]); result } @@ -2243,38 +2300,28 @@ fn scope_simple_selector(selector: &str, content_attr: &str) -> String { return String::new(); } - // A comment can end up glued directly to the selector that follows it - - // e.g. when CSS has been reprinted by an upstream formatter (PostCSS) - // that strips the blank line normally separating `/* comment */` from - // the next rule, `extract_comments` leaves behind `%COMMENT%.foo` with - // no separating whitespace. Bailing out unscoped for the *whole* string - // here would silently drop Emulated encapsulation for `.foo` too - so - // peel any leading/trailing placeholder(s) off, scope the real selector - // text, then splice them back. `restore_comments` later turns every - // placeholder into blank text, so their exact position doesn't matter. - if selector.contains(COMMENT_PLACEHOLDER) { - let mut rest = selector; - let mut leading = String::new(); - while let Some(stripped) = rest.strip_prefix(COMMENT_PLACEHOLDER) { - leading.push_str(COMMENT_PLACEHOLDER); - rest = stripped; - } - let mut trailing = String::new(); - while let Some(stripped) = rest.strip_suffix(COMMENT_PLACEHOLDER) { - trailing.push_str(COMMENT_PLACEHOLDER); - rest = stripped; - } - if rest.is_empty() { - // Nothing but comment placeholder(s) - no real selector to scope. - return selector.to_string(); - } - if rest.contains(COMMENT_PLACEHOLDER) { - // A placeholder sits inside the selector body itself (e.g. a - // comment between a class name and a combinator) - too unusual - // to safely split; leave unscoped rather than risk corruption. - return selector.to_string(); + // A comment can end up glued directly to the selector text: PostCSS and + // other formatters reprint `/* why */\n.foo` as `/* why */.foo`, so + // `extract_comments` leaves behind `%COMMENT0%.foo` with nothing separating + // them. Bailing out here would ship `.foo` with no content attribute at + // all - a global rule that clobbers every other component's `.foo`, with no + // error. So strip the placeholders out, scope the real selector text, and + // re-emit them in front: they restore to blank text (or, for a sourcemap + // comment, to text that was never part of the selector anyway), so only + // their order relative to each other matters. + if selector.contains(COMMENT_PLACEHOLDER_PREFIX) { + // `%COMMENT` without a valid index isn't ours - fall through and scope + // it as ordinary selector text. + let (stripped, placeholders) = strip_comment_placeholders(selector); + if !placeholders.is_empty() { + if stripped.trim().is_empty() { + // Nothing but placeholder(s) - no real selector to scope. Must + // not fall through, or a lone comment would become a bare + // `[content]` matching every element in the component. + return selector.to_string(); + } + return placeholders + &scope_simple_selector(&stripped, content_attr); } - return format!("{leading}{}{trailing}", scope_simple_selector(rest, content_attr)); } // Already has the content attribute diff --git a/crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs b/crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs deleted file mode 100644 index d544585a3..000000000 --- a/crates/oxc_angular_compiler/tests/comment_glued_selector_regression_test.rs +++ /dev/null @@ -1,81 +0,0 @@ -//! Regression test for a real-world encapsulation bug: a component's CSS -//! rule shipped with no `[_ngcontent-*]` scoping attribute at all, so it -//! matched (and visually clobbered) any element with the same class name -//! anywhere else in the app, with no console error. -//! -//! Root cause: when a Vite consumer preprocesses a component's `styleUrl` -//! file through PostCSS (e.g. for Tailwind) before handing it to this -//! crate's encapsulation pass, PostCSS's AST-based reprint collapses the -//! blank line that normally separates a `/* comment */` from the rule that -//! follows it. A hand-formatted stylesheet like: -//! -//! ```css -//! /* explains why this rule exists */ -//! .widget { -//! width: 9px; -//! } -//! ``` -//! -//! reaches `shim_css_text` as `*/.widget { width: 9px; }` - the comment -//! glued directly to the selector with no separating whitespace. -//! `scope_simple_selector` used to bail out of scoping the *entire* selector -//! string whenever it contained the comment placeholder, so `.widget` -//! shipped unscoped: a plain, globally-matching rule any other component's -//! same-named class could collide with. -//! -//! Fixed in `scope_simple_selector` (`src/styles/encapsulation.rs`): leading -//! and trailing comment placeholders are now peeled off before scoping and -//! spliced back afterwards, instead of aborting scoping altogether. -//! -//! See `tests/shadow_css_test.rs` for the minimal unit-level regression -//! tests (`test_scope_selector_glued_directly_to_*`); this file exercises -//! the same bug against a full stylesheet shape with several unrelated, -//! comment-documented rules - the shape that originally surfaced it. - -use oxc_angular_compiler::styles::finalize_component_style; - -/// A demo stylesheet shaped like the one that surfaced this bug: several -/// unrelated rules, each documented with a `/* comment */` immediately -/// before it, and reproduced here exactly as PostCSS reprints it - with the -/// blank line that would normally separate the comment from the next rule -/// removed. -const CSS_WITH_COMMENTS_GLUED_TO_SELECTORS: &str = "\ -:host { position: relative; overflow: hidden; }\ -/* Contributes width but no height. */\ -.probe { display: grid; height: 0; }\ -:host:has(.widget) { padding-left: 21px; }\ -.wrapper { overflow: auto; position: relative; }\ -/* This rule positions the widget precisely. */\ -.widget { width: 9px; padding: 0 4px; height: 100%; position: absolute; cursor: col-resize; }\ -.widget:hover { background-color: red; }\ -/* Sticky variant used when scrolling. */\ -.container.sticky-header { display: flex; flex-direction: column; }\ -"; - -#[test] -fn comment_glued_selectors_all_stay_scoped() { - let result = finalize_component_style( - CSS_WITH_COMMENTS_GLUED_TO_SELECTORS, - true, - "_ngcontent-%COMP%", - "_nghost-%COMP%", - false, - ); - - for selector in [".probe", ".wrapper", ".widget", ".container.sticky-header"] { - let unscoped_rule_start = format!("{selector} {{"); - assert!( - !result.contains(&unscoped_rule_start), - "`{selector}` lost its Angular content-attribute scoping - a \ - comment glued directly to it (no separating whitespace, as \ - PostCSS reprints it) made `scope_simple_selector` bail out of \ - scoping the whole rule. Full shimmed output:\n{result}" - ); - - let scoped_rule = format!("{selector}[_ngcontent-%COMP%]"); - assert!( - result.contains(&scoped_rule), - "expected `{scoped_rule}` in output, got:\n{result}" - ); - } -} diff --git a/crates/oxc_angular_compiler/tests/shadow_css_test.rs b/crates/oxc_angular_compiler/tests/shadow_css_test.rs index 92af597c1..631cc3ef8 100644 --- a/crates/oxc_angular_compiler/tests/shadow_css_test.rs +++ b/crates/oxc_angular_compiler/tests/shadow_css_test.rs @@ -9,7 +9,7 @@ //! //! The goal is to have 1:1 compatibility with Angular's ShadowCss implementation. -use oxc_angular_compiler::styles::shim_css_text; +use oxc_angular_compiler::styles::{finalize_component_style, shim_css_text}; /// Normalize CSS for comparison (matches Angular's toEqualCss behavior). /// - Removes leading/trailing whitespace @@ -328,7 +328,6 @@ fn test_multibyte_utf8_preserved_in_css_values() { #[test] fn test_finalize_preserves_unicode() { - use oxc_angular_compiler::styles::finalize_component_style; // Full pipeline with Sass-compiled CSS containing actual bullet character let css = ".test:after { content: \"\u{2022}\"; }"; let result = finalize_component_style(css, true, "_ngcontent-%COMP%", "_nghost-%COMP%", true); @@ -1002,47 +1001,65 @@ fn test_scope_first_selector_after_multiple_comments() { #[test] fn test_scope_selector_glued_directly_to_preceding_comment() { - // Regression: a comment with NO separating whitespace before the next - // selector - e.g. `*/.foo`. This is not how anyone hand-writes CSS, but - // it's exactly what PostCSS's AST-based reprint produces when it - // processes a `styleUrl` file (Vite's `preprocessCSS`, used for Tailwind, - // runs before this CSS ever reaches `shim_css_text`): it strips the - // blank line that normally separates `/* comment */` from the following - // rule. `.foo` must still get its content attribute; a whole rule - // silently losing Emulated encapsulation - because the selector string - // handed to `scope_simple_selector` was `%COMMENT%.foo`, which the old - // code bailed out of scoping entirely - is how a component's own styles - // leak globally onto any other element sharing that class name, with no - // console error. - let css = "/* comment */.foo { color: red; }"; - let expected = ".foo[contenta] { color: red; }"; - assert_css_eq!(shim(css, "contenta"), expected); + // A comment with NO whitespace before the next selector. Nobody writes + // this by hand, but it's what PostCSS's AST reprint produces when a Vite + // consumer runs a `styleUrl` file through `preprocessCSS` (e.g. for + // Tailwind) before it reaches `shim_css_text`. Bailing out here shipped + // `.foo` with no content attribute at all - a global rule that clobbers + // any other component's `.foo`, with no console error. + assert_css_eq!( + shim("/* comment */.foo { color: red; }", "contenta"), + ".foo[contenta] { color: red; }" + ); } #[test] fn test_scope_selector_glued_directly_to_multiple_preceding_comments() { - // Same as above, but with two adjacent comments (also produced by - // PostCSS reprinting, e.g. an SCSS partial's license header merged with - // a rule doc-comment) both glued with no whitespace before the selector. - let css = "/* one *//* two */.foo { color: red; }"; - let expected = ".foo[contenta] { color: red; }"; - assert_css_eq!(shim(css, "contenta"), expected); + assert_css_eq!( + shim("/* one *//* two */.foo { color: red; }", "contenta"), + ".foo[contenta] { color: red; }" + ); } #[test] fn test_scope_selector_glued_directly_to_trailing_comment() { - // A comment glued to the END of a selector, immediately before `{`. - let css = ".foo/* comment */{ color: red; }"; - let expected = ".foo[contenta] { color: red; }"; - assert_css_eq!(shim(css, "contenta"), expected); + // Comment glued to the END of a selector, immediately before `{`. + assert_css_eq!( + shim(".foo/* comment */{ color: red; }", "contenta"), + ".foo[contenta]{ color: red; }" + ); +} + +#[test] +fn test_scope_selector_with_interior_glued_comment() { + // Placeholder *inside* the compound selector rather than at either end. + // These must scope too - an unscoped rule leaks globally either way. + assert_css_eq!( + shim(".foo/* c */:hover { color: red; }", "contenta"), + ".foo[contenta]:hover { color: red; }" + ); + assert_css_eq!( + shim(".foo/* c */.bar { color: red; }", "contenta"), + ".foo.bar[contenta] { color: red; }" + ); + assert_css_eq!( + shim("/* a */.foo/* b */.bar/* c */ { color: red; }", "contenta"), + ".foo.bar[contenta] { color: red; }" + ); +} + +#[test] +fn test_comment_only_selector_is_not_scoped() { + // A lone comment where a selector would go must stay a no-op - scoping it + // would emit a bare `[contenta]` matching every element in the component. + assert!(!shim("/* c */ { color: red; }", "contenta").contains("contenta")); + assert!(!shim("/* a */ /* b */ { color: red; }", "contenta").contains("contenta")); } #[test] fn test_scope_multiple_rules_after_comment_glued_selectors() { - // The real-world shape that surfaced this bug: several unrelated rules - // in the same stylesheet, each with its explanatory comment glued - // directly to the following selector by PostCSS's reprint, must all - // still scope correctly rather than only the first one. + // Several rules in one stylesheet, each with its comment glued on by + // PostCSS - all must scope, not just the first. let css = "/* first */.foo { width: 1px; }/* second */.bar { height: 2px; }"; let expected = ".foo[contenta] { width: 1px; }.bar[contenta] { height: 2px; }"; assert_css_eq!(shim(css, "contenta"), expected); @@ -1050,42 +1067,199 @@ fn test_scope_multiple_rules_after_comment_glued_selectors() { #[test] fn test_scope_comment_glued_selectors_in_descendant_chain_and_comma_list() { - // The comment only glues to the *first* compound selector in a - // multi-part selector - every part still needs its own attribute. - let descendant = "/* c */.container .tabs-group { color: red; }"; + // The comment glues only to the *first* compound selector - every other + // part still needs its own attribute. assert_css_eq!( - shim(descendant, "contenta"), + shim("/* c */.container .tabs-group { color: red; }", "contenta"), ".container[contenta] .tabs-group[contenta] { color: red; }" ); - - let comma_list = "/* c */.a, .b { color: red; }"; - assert_css_eq!(shim(comma_list, "contenta"), ".a[contenta], .b[contenta] { color: red; }"); + assert_css_eq!( + shim("/* c */.a, .b { color: red; }", "contenta"), + ".a[contenta], .b[contenta] { color: red; }" + ); } #[test] fn test_scope_comment_glued_to_host_and_host_context() { - let host = "/* c */:host { color: red; }"; - assert_css_eq!(shim_with_host(host, "contenta", "hosta"), "[hosta] { color: red; }"); - - let host_context = "/* c */:host-context(.dark) { color: red; }"; assert_css_eq!( - shim_with_host(host_context, "contenta", "hosta"), + shim_with_host("/* c */:host { color: red; }", "contenta", "hosta"), + "[hosta] { color: red; }" + ); + assert_css_eq!( + shim_with_host("/* c */:host-context(.dark) { color: red; }", "contenta", "hosta"), ".dark[hosta], .dark [hosta] { color: red; }" ); + assert_css_eq!( + shim_with_host(":host/* c */.a { color: red; }", "contenta", "hosta"), + ".a[hosta] { color: red; }" + ); } #[test] -fn test_interior_comment_placeholder_falls_back_to_unscoped() { - // Documented, intentional limitation: a comment placeholder *inside* a - // selector body (not glued to its start or end, e.g. between a class - // name and a pseudo-class) is too unusual to safely split - the fix - // deliberately leaves this case unscoped rather than risk corrupting the - // selector. This isn't a shape PostCSS's normal reprint produces (which - // is what the fix actually targets); it just must not panic or garble - // the selector. - let css = ".foo/* c */:hover { color: red; }"; - let expected = ".foo:hover { color: red; }"; - assert_css_eq!(shim(css, "contenta"), expected); +fn test_comment_glued_keyframe_selectors_stay_unscoped() { + // `%COMMENT0%from` must still be recognised as a keyframe selector after + // the placeholders are stripped - scoping it would break the animation. + assert_css_eq!( + shim("@keyframes k {/* c */from { opacity: 0; }/* d */50% { opacity: 1; } }", "contenta"), + "@keyframes contenta_k {from { opacity: 0; }50% { opacity: 1; }}" + ); +} + +#[test] +fn test_comment_glued_selector_preserves_comment_newlines() { + // Comments restore as the newlines they contained, so the shimmed output + // keeps its line count and component sourcemaps don't shift. Stripping + // placeholders out of a selector must not drop any of them. Their exact + // position within the selector doesn't matter (they're blank text) - they + // come back out in front of it. + assert_eq!( + shim("/* multi\nline */.foo { color: red; }", "contenta"), + "\n.foo[contenta] { color: red; }" + ); + assert_eq!( + shim(".foo/* multi\nline */{ color: red; }", "contenta"), + "\n.foo[contenta]{ color: red; }" + ); + // An interior comment's newline must not land mid-compound-selector, where + // it would silently become a descendant combinator (`.foo :hover`). + assert_eq!( + shim(".foo/* multi\nline */:hover { color: red; }", "contenta"), + "\n.foo[contenta]:hover { color: red; }" + ); +} + +#[test] +fn test_comment_glued_stylesheet_stays_scoped_through_finalize() { + // End-to-end shape that surfaced this: a whole component stylesheet as + // PostCSS reprints it, with every documenting comment glued to the rule + // it documents. Run through the minifying entry point too, since that + // re-parses the shimmed output. + let css = "\ +:host { position: relative; }\ +/* Contributes width but no height. */\ +.probe { display: grid; height: 0; }\ +:host:has(.widget) { padding-left: 21px; }\ +/* This rule positions the widget precisely. */\ +.widget { width: 9px; position: absolute; }\ +.widget:hover { background-color: red; }\ +/* Sticky variant used when scrolling. */\ +.container.sticky-header { display: flex; }\ +"; + + for minify in [false, true] { + let result = + finalize_component_style(css, true, "_ngcontent-%COMP%", "_nghost-%COMP%", minify); + assert!(!result.contains("%COMMENT"), "placeholder leaked (minify={minify}):\n{result}"); + for selector in [".probe", ".widget", ".container.sticky-header"] { + assert!( + result.contains(&format!("{selector}[_ngcontent-%COMP%]")), + "`{selector}` lost its content attribute (minify={minify}):\n{result}" + ); + } + } +} + +// ============================================================================ +// Regression: comment placeholders must never survive into the shipped CSS +// ============================================================================ + +/// Inputs that exercise every pass which duplicates, drops or reorders the +/// selector text a comment placeholder can be embedded in. +const PLACEHOLDER_STRESS_CASES: &[&str] = &[ + // `:host-context()` emits its trailing selector text once per permutation, + // duplicating any placeholder inside it. + ":host-context(.d) /* c */ .e { color: red; }", + ":host-context(.d)/* c */ .e { color: red; }", + ":host-context(.d) .e/* c */ { color: red; }", + ":host-context(/* c */.d) .e { color: red; }", + ":host-context(.a, .b) /* c */ .e { color: red; }", + ":host-context(.a):host-context(.b) /* c */ .e { color: red; }", + ":where(:host-context(.d)) /* c */ .e { color: red; }", + "/* pre */:host-context(.d) /* c */ .e { color: red; }", + ":host-context(.a) /* c */ :host { color: red; }", + // Passes that discard selector text, dropping placeholders. + "polyfill-next-selector { content: ':host .a'; }/* c */::content .b { color: red; }", + "polyfill-rule { content: ':host .a'; color: red; }/* c */.z { color: blue; }", + "polyfill-unscoped-rule { content: '.a'; color: red; }/* c */.z { color: blue; }", + ".a ::ng-deep /* c */ .b { color: red; }", + // Ordinary shapes, glued and unglued. + "/* c */.foo/* d */.bar/* e */ { color: red; }", + "@media screen { /* c */.foo { color: red; } }", + "@keyframes k {/* c */from { opacity: 0; } }", + "/* c */ { color: red; }", +]; + +#[test] +fn test_comment_placeholder_never_leaks_into_output() { + // A literal `%COMMENT%` in shipped CSS is a corrupt selector. Before + // placeholders carried their comment index, every `:host-context()` case + // below leaked one: each duplicated placeholder consumed the *next* + // comment, so the last one ran out and was left in the output verbatim. + for css in PLACEHOLDER_STRESS_CASES { + let result = shim_with_host(css, "contenta", "hosta"); + assert!(!result.contains("%COMMENT"), "placeholder leaked for {css:?}:\n{result}"); + } +} + +#[test] +fn test_sourcemap_comment_survives_placeholder_duplication() { + // Sourcemap comments are the one kind restored verbatim, so a mis-mapped + // placeholder is directly visible: before the fix the duplicated + // `:host-context()` placeholder consumed this comment and teleported it + // into the middle of a selector, leaving `%COMMENT%` at the real position. + for css in PLACEHOLDER_STRESS_CASES { + let with_map = format!("{css}\n/*# sourceMappingURL=x.map */"); + let result = shim_with_host(&with_map, "contenta", "hosta"); + // It must still occupy its own line - the teleport spliced it into the + // middle of a selector. (`polyfill-unscoped-rule` legitimately appends + // its rule after this line, so don't require it to be last.) + assert!( + result.lines().any(|line| line.trim() == "/*# sourceMappingURL=x.map */"), + "sourcemap comment moved for {css:?}:\n{result}" + ); + assert_eq!( + result.matches("sourceMappingURL").count(), + 1, + "sourcemap comment duplicated for {css:?}:\n{result}" + ); + } +} + +#[test] +fn test_placeholder_shaped_text_in_source_css_never_leaks() { + // Backstop: a placeholder whose index doesn't resolve must not survive into + // the output. Reachable today only by writing the placeholder shape into + // the source CSS, but it's what keeps "no `%COMMENT%` in shipped CSS" + // true for any future pass that duplicates or fabricates one. + let result = shim(".foo%COMMENT99%.bar { color: red; }", "contenta"); + assert!(!result.contains("%COMMENT"), "{result}"); + assert!(result.contains("[contenta]"), "{result}"); +} + +#[test] +fn test_host_context_permutations_share_one_comment() { + // Every permutation gets a copy of the placeholder; each must restore to + // the same (blank) comment rather than eating a later one. + assert_css_eq!( + shim_with_host(":host-context(.a, .b) /* c */ .e { color: red; }", "contenta", "hosta"), + ".a[hosta] .e[contenta], .a [hosta] .e[contenta], \ + .b[hosta] .e[contenta], .b [hosta] .e[contenta] { color: red; }" + ); + assert_css_eq!( + shim_with_host(":host-context(/* c */.d) .e { color: red; }", "contenta", "hosta"), + ".d[hosta] .e[contenta], .d [hosta] .e[contenta] { color: red; }" + ); +} + +#[test] +fn test_comment_placeholder_indices_are_independent_of_position() { + // Restoration is by index, so a dropped placeholder earlier in the file + // must not shift the comments that follow it onto the wrong placeholders. + let css = "polyfill-next-selector { content: ':host .a'; }/* dropped */::content .b { color: red; }\n\ + /*# sourceMappingURL=x.map */"; + let result = shim_with_host(css, "contenta", "hosta"); + assert!(result.contains("/*# sourceMappingURL=x.map */"), "{result}"); + assert!(!result.contains("%COMMENT"), "{result}"); } #[test] From a08fc26771c40170cdf0cf93b57e150d6945c999 Mon Sep 17 00:00:00 2001 From: Ashley Hunter Date: Fri, 21 Aug 2026 09:03:53 +0100 Subject: [PATCH 3/3] fix(styles): detach comment placeholders before pseudo-function scoping Addresses two review findings. A comment glued onto an otherwise pure `:where()`/`:is()` part stopped it looking like a pure pseudo-function, so `/* c */:where(.one)` scoped to `[contenta]:where(.one)` rather than `:where(.one[contenta])`. `:where()` contributes no specificity, so that quietly moves the rule up the cascade. The placeholder detach moves from `scope_simple_selector` up to `scope_selector_part_with_context`, which sits above the pseudo-function dispatch, the host-marker check and both `scope_simple_selector` calls - so every check downstream now sees the real selector text. The guard is gone from `scope_simple_selector`, whose callers are all downstream of the new one. `restore_comments` also no longer deletes source text that merely looks like a placeholder. Every placeholder this crate generates resolves, since `extract_comments` only emits in-range indices and the duplicating passes copy the index along with the text - so an index that doesn't resolve came from the author (`content: "%COMMENT7%"`) and is now left exactly as written. --- .../src/styles/encapsulation.rs | 62 +++++++++++-------- .../tests/shadow_css_test.rs | 45 +++++++++++--- 2 files changed, 72 insertions(+), 35 deletions(-) diff --git a/crates/oxc_angular_compiler/src/styles/encapsulation.rs b/crates/oxc_angular_compiler/src/styles/encapsulation.rs index 99e225c58..64fece20f 100644 --- a/crates/oxc_angular_compiler/src/styles/encapsulation.rs +++ b/crates/oxc_angular_compiler/src/styles/encapsulation.rs @@ -434,16 +434,20 @@ fn extract_comments(css: &str) -> (String, Vec) { /// /// Every placeholder carries the index of the comment it stands for, so a /// placeholder that got duplicated (`:host-context()` permutations) restores to -/// the same comment as its twin, and one that got dropped shifts nothing. Any -/// placeholder with an out-of-range index resolves to nothing rather than being -/// left in the output - a literal `%COMMENT%` must never reach shipped CSS. +/// the same comment as its twin, and one that got dropped shifts nothing. +/// +/// `extract_comments` only ever emits in-range indices, and the passes that +/// copy selector text copy the index along with it, so every placeholder *we* +/// generated resolves. An index that doesn't is therefore source CSS that +/// merely looks like a placeholder (`content: "%COMMENT7%"`), and is left +/// exactly as the author wrote it. fn restore_comments(css: &str, comments: &[String]) -> String { let mut result = String::with_capacity(css.len()); let mut at = 0; while let Some((range, index)) = find_comment_placeholder(css, at) { result.push_str(&css[at..range.start]); - result.push_str(comments.get(index).map_or("", String::as_str)); + result.push_str(comments.get(index).map_or(&css[range.start..range.end], String::as_str)); at = range.end; } result.push_str(&css[at..]); @@ -2105,6 +2109,30 @@ fn scope_selector_part_with_context( return String::new(); } + // Detach any comment placeholders before anything inspects the selector. + // A comment can be glued straight onto the selector text (PostCSS reprints + // `/* why */\n.foo` as `/* why */.foo`), and every check below is a + // whole-string match: with `%COMMENT0%` still attached, `:where(.one)` + // stops looking like a pure pseudo-function and gets scoped as + // `[content]:where(.one)` instead of `:where(.one[content])` - which is a + // real cascade change, since `:where()` contributes no specificity. The + // placeholders restore to blank text, so re-emitting them in front is + // enough. + if selector.contains(COMMENT_PLACEHOLDER_PREFIX) { + // `%COMMENT` without a valid index isn't ours - fall through and treat + // it as ordinary selector text. + let (stripped, placeholders) = strip_comment_placeholders(selector); + if !placeholders.is_empty() { + if stripped.trim().is_empty() { + // Nothing but placeholder(s) - no real selector to scope. Must + // not fall through, or a lone comment would become a bare + // `[content]` matching every element in the component. + return selector.to_string(); + } + return placeholders + &scope_selector_part_with_context(&stripped, ctx, part_has_host); + } + } + // If this part IS the host marker, don't add content attr if !ctx.host_marker.is_empty() && selector.trim() == ctx.host_marker { return selector.to_string(); @@ -2300,29 +2328,9 @@ fn scope_simple_selector(selector: &str, content_attr: &str) -> String { return String::new(); } - // A comment can end up glued directly to the selector text: PostCSS and - // other formatters reprint `/* why */\n.foo` as `/* why */.foo`, so - // `extract_comments` leaves behind `%COMMENT0%.foo` with nothing separating - // them. Bailing out here would ship `.foo` with no content attribute at - // all - a global rule that clobbers every other component's `.foo`, with no - // error. So strip the placeholders out, scope the real selector text, and - // re-emit them in front: they restore to blank text (or, for a sourcemap - // comment, to text that was never part of the selector anyway), so only - // their order relative to each other matters. - if selector.contains(COMMENT_PLACEHOLDER_PREFIX) { - // `%COMMENT` without a valid index isn't ours - fall through and scope - // it as ordinary selector text. - let (stripped, placeholders) = strip_comment_placeholders(selector); - if !placeholders.is_empty() { - if stripped.trim().is_empty() { - // Nothing but placeholder(s) - no real selector to scope. Must - // not fall through, or a lone comment would become a bare - // `[content]` matching every element in the component. - return selector.to_string(); - } - return placeholders + &scope_simple_selector(&stripped, content_attr); - } - } + // Comment placeholders are detached in `scope_selector_part_with_context`, + // which is upstream of every caller of this function, so the selector text + // here is already placeholder-free. // Already has the content attribute let attr = format!("[{}]", content_attr); diff --git a/crates/oxc_angular_compiler/tests/shadow_css_test.rs b/crates/oxc_angular_compiler/tests/shadow_css_test.rs index 631cc3ef8..b88b71176 100644 --- a/crates/oxc_angular_compiler/tests/shadow_css_test.rs +++ b/crates/oxc_angular_compiler/tests/shadow_css_test.rs @@ -1226,14 +1226,43 @@ fn test_sourcemap_comment_survives_placeholder_duplication() { } #[test] -fn test_placeholder_shaped_text_in_source_css_never_leaks() { - // Backstop: a placeholder whose index doesn't resolve must not survive into - // the output. Reachable today only by writing the placeholder shape into - // the source CSS, but it's what keeps "no `%COMMENT%` in shipped CSS" - // true for any future pass that duplicates or fabricates one. - let result = shim(".foo%COMMENT99%.bar { color: red; }", "contenta"); - assert!(!result.contains("%COMMENT"), "{result}"); - assert!(result.contains("[contenta]"), "{result}"); +fn test_placeholder_shaped_text_in_source_css_is_left_alone() { + // Author-written text that merely looks like a placeholder must survive. + // Every placeholder we generate resolves (`extract_comments` only emits + // in-range indices and the duplicating passes copy the index with the + // text), so an index that doesn't resolve is the author's, not ours. + for css in [ + ".label { content: \"%COMMENT0%\"; }", + ".label { content: \"%COMMENT99%\"; }", + "/* c */.label { content: \"%COMMENT7%\"; }", + ] { + let result = shim(css, "contenta"); + assert!(result.contains("content: \"%COMMENT"), "declaration text was eaten: {result}"); + assert!(result.contains(".label[contenta]"), "{result}"); + } +} + +#[test] +fn test_comment_glued_to_pure_pseudo_function_scopes_inside_it() { + // `:where()` contributes no specificity, so scoping to `[contenta]:where(.one)` + // instead of `:where(.one[contenta])` silently changes the cascade. A glued + // comment must not make the part stop looking like a pure pseudo-function. + assert_css_eq!( + shim("/* c */:where(.one) { color: red; }", "contenta"), + ":where(.one[contenta]) { color: red; }" + ); + assert_css_eq!( + shim(":where(.one)/* c */ { color: red; }", "contenta"), + ":where(.one[contenta]) { color: red; }" + ); + assert_css_eq!( + shim("/* c */:is(.a, .b) { color: red; }", "contenta"), + ":is(.a[contenta], .b[contenta]) { color: red; }" + ); + assert_css_eq!( + shim("/* c */:where(.a):is(.b) { color: red; }", "contenta"), + ":where(.a[contenta]):is(.b[contenta]) { color: red; }" + ); } #[test]