From d0d608580f3c6ea7f677d8f420beb2bfb0486cc3 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Wed, 2 Sep 2026 14:18:08 +0800 Subject: [PATCH] fix(cssom): preserve stylesheet EOF-open var values --- moli-css-parse/src/declaration.rs | 137 ++++++++++++++++++ moli-css-parse/src/lib.rs | 13 +- moli-css-parse/src/value.rs | 18 +++ moli-renderer-v8/src/live_stylesheet/mod.rs | 92 +++++++++++- .../src/live_stylesheet/mutation.rs | 4 + .../src/live_stylesheet/registry.rs | 3 + moli-renderer-v8/src/live_stylesheet/tests.rs | 61 ++++++++ .../styles/declaration/parse/pdb_compat.rs | 3 + .../cssom/extracted/cssom_interfaces.rs | 40 ++++- 9 files changed, 363 insertions(+), 8 deletions(-) diff --git a/moli-css-parse/src/declaration.rs b/moli-css-parse/src/declaration.rs index e45af0bcae..7fbb3ffed6 100644 --- a/moli-css-parse/src/declaration.rs +++ b/moli-css-parse/src/declaration.rs @@ -40,6 +40,112 @@ pub fn parse_declaration_list( .collect() } +/// Recover the declaration-list serialization for a stylesheet that ended in +/// one valid, unclosed `var()` reference. +/// +/// Stylo accepts the EOF-recovered rule but currently appends the missing `)` +/// to its stored value. This helper only removes that character when the raw +/// final declaration and Stylo's serialized final declaration otherwise match +/// exactly. The caller is responsible for associating the result with the +/// rightmost native style rule. +pub fn recover_stylesheet_eof_open_var_declaration_text( + stylesheet_text: &str, + serialized_declaration_text: &str, +) -> Option { + let options = DeclarationParseOptions { + canonicalize_property_name: false, + unescape_value_semicolons: true, + preserve_empty_values: false, + }; + let serialized_declaration = parse_declaration_list(serialized_declaration_text, options) + .into_iter() + .last()?; + + for block_start in eof_open_curly_block_starts(stylesheet_text) { + let Some(source_declaration) = + parse_declaration_list(&stylesheet_text[block_start + 1..], options) + .into_iter() + .last() + else { + continue; + }; + if !crate::value::css_value_is_eof_open_var_function(&source_declaration.value) + || crate::canonical_style_property_name(&source_declaration.name) + != crate::canonical_style_property_name(&serialized_declaration.name) + || source_declaration.important != serialized_declaration.important + || serialized_declaration.value != format!("{})", source_declaration.value) + { + continue; + } + + let value_start = serialized_declaration_text.rfind(&serialized_declaration.value)?; + let closing_parenthesis = value_start + serialized_declaration.value.len() - 1; + if serialized_declaration_text.as_bytes()[closing_parenthesis] != b')' + || !serialized_declaration_suffix_matches( + &serialized_declaration_text[closing_parenthesis + 1..], + serialized_declaration.important, + ) + { + continue; + } + + let mut recovered = String::with_capacity(serialized_declaration_text.len() - 1); + recovered.push_str(&serialized_declaration_text[..closing_parenthesis]); + recovered.push_str(&serialized_declaration_text[closing_parenthesis + 1..]); + return Some(recovered); + } + None +} + +fn serialized_declaration_suffix_matches(suffix: &str, important: bool) -> bool { + let suffix = suffix.trim(); + if important { + suffix.eq_ignore_ascii_case("!important;") + } else { + suffix == ";" + } +} + +fn eof_open_curly_block_starts(css_text: &str) -> Vec { + let mut open_blocks = Vec::new(); + let mut input = ParserInput::new(css_text); + let mut input = Parser::new(&mut input); + collect_eof_open_curly_block_starts(&mut input, &mut open_blocks); + open_blocks +} + +fn collect_eof_open_curly_block_starts<'i, 't>( + input: &mut Parser<'i, 't>, + open_blocks: &mut Vec, +) { + loop { + let token_start = input.position(); + let Ok(token) = input.next_including_whitespace_and_comments().cloned() else { + break; + }; + let is_curly = matches!(token, cssparser::Token::CurlyBracketBlock); + if !matches!( + token, + cssparser::Token::Function(_) + | cssparser::Token::ParenthesisBlock + | cssparser::Token::SquareBracketBlock + | cssparser::Token::CurlyBracketBlock + ) { + continue; + } + + let mut nested_end = None; + let nested: Result<(), ParseError<'i, ()>> = input.parse_nested_block(|input| { + collect_eof_open_curly_block_starts(input, open_blocks); + nested_end = Some(input.position()); + Ok(()) + }); + if nested.is_ok() && is_curly && nested_end == Some(input.position()) { + open_blocks.push(token_start.byte_index()); + } + } +} + struct DeclarationListParser { options: DeclarationParseOptions, } @@ -178,6 +284,37 @@ mod tests { assert_eq!(entries[0].value, "var(--prop"); } + #[test] + fn stylesheet_eof_open_var_recovery_removes_only_stylos_inserted_parenthesis() { + let source = r#" + .ignored::before { content: "}"; } + /* ignored { */ + @media all { .target { color: red; width: var(--prop + "#; + assert_eq!( + super::recover_stylesheet_eof_open_var_declaration_text( + source, + "color: red; width: var(--prop);", + ) + .as_deref(), + Some("color: red; width: var(--prop;") + ); + assert_eq!( + super::recover_stylesheet_eof_open_var_declaration_text( + ".target { width: var(--prop)", + "width: var(--prop);", + ), + None + ); + assert_eq!( + super::recover_stylesheet_eof_open_var_declaration_text( + ".target { width: var(--prop", + "width: var(--other);", + ), + None + ); + } + #[test] fn declaration_parser_canonicalizes_when_requested() { let entries = parse_declaration_list( diff --git a/moli-css-parse/src/lib.rs b/moli-css-parse/src/lib.rs index bb8f207118..69de126f45 100644 --- a/moli-css-parse/src/lib.rs +++ b/moli-css-parse/src/lib.rs @@ -18,7 +18,10 @@ pub use color::{ css_system_color_srgb, parse_css_color_scheme_flags, parse_css_color_to_opaque_srgb_hex, parse_css_color_to_srgb_bytes, }; -pub use declaration::{CssDeclaration, DeclarationParseOptions, parse_declaration_list}; +pub use declaration::{ + CssDeclaration, DeclarationParseOptions, parse_declaration_list, + recover_stylesheet_eof_open_var_declaration_text, +}; pub use font_face::{ CssFontFace, font_load_query_contains_css_wide_keyword, font_load_query_family, font_load_query_is_valid, normalize_font_face_src, parse_font_faces, @@ -76,10 +79,10 @@ pub use util::{ unescape_top_level_semicolons, unquote_css_string, }; pub use value::{ - css_declaration_value_has_valid_env_functions, css_value_may_contain_env_function, - css_value_may_contain_var_function, normalize_css_variable_specified_value, - normalize_cssom_component_value_serialization, normalize_custom_property_specified_value, - serialize_component_values_single_line, + css_declaration_value_has_valid_env_functions, css_value_is_eof_open_var_function, + css_value_may_contain_env_function, css_value_may_contain_var_function, + normalize_css_variable_specified_value, normalize_cssom_component_value_serialization, + normalize_custom_property_specified_value, serialize_component_values_single_line, }; /// Returns the non-custom CSS properties that the pinned Stylo world currently diff --git a/moli-css-parse/src/value.rs b/moli-css-parse/src/value.rs index f1fb031834..666ef28305 100644 --- a/moli-css-parse/src/value.rs +++ b/moli-css-parse/src/value.rs @@ -33,6 +33,15 @@ pub fn css_value_may_contain_var_function(value: &str) -> bool { .any(|window| matches!(window, [b'v' | b'V', b'a' | b'A', b'r' | b'R', b'('])) } +/// Whether `value` is a valid standalone `var()` reference whose final +/// parenthesis was omitted at the end of a CSS input fragment. +/// +/// CSSOM preserves that omission in the specified value even though Stylo's +/// declaration storage currently appends the missing parenthesis internally. +pub fn css_value_is_eof_open_var_function(value: &str) -> bool { + recover_left_open_css_var_function(value).is_some() +} + /// This validates var() token structure before substitution, but does not decide /// ordinary property grammar. pub(crate) fn css_declaration_value_has_valid_var_functions(value: &str) -> bool { @@ -559,6 +568,15 @@ mod tests { ); } + #[test] + fn eof_open_var_function_detection_is_narrow() { + assert!(super::css_value_is_eof_open_var_function("var(--x")); + assert!(super::css_value_is_eof_open_var_function("VAR(--x;")); + assert!(!super::css_value_is_eof_open_var_function("var(--x)")); + assert!(!super::css_value_is_eof_open_var_function("1px var(--x")); + assert!(!super::css_value_is_eof_open_var_function("var(-x")); + } + #[test] fn custom_property_specified_value_normalizer_preserves_ident_var_names() { assert_eq!( diff --git a/moli-renderer-v8/src/live_stylesheet/mod.rs b/moli-renderer-v8/src/live_stylesheet/mod.rs index 92b691e8fb..8ea2e5b2bc 100644 --- a/moli-renderer-v8/src/live_stylesheet/mod.rs +++ b/moli-renderer-v8/src/live_stylesheet/mod.rs @@ -496,11 +496,20 @@ pub(crate) struct StylesheetRuleWrapperBinding { path: Vec, rule: NativeStylesheetRule, shared_lock: SharedRwLock, + source_declaration_override: LiveStylesheetSourceDeclarationOverrideState, + detached_source_declaration_override: Option, } impl StylesheetRuleWrapperBinding { fn snapshot(&self) -> CssRuleSnapshot { - self.rule.snapshot(&self.shared_lock) + let mut snapshot = self.rule.snapshot(&self.shared_lock); + if let Some(declaration_text) = self.source_declaration_override() { + snapshot.declaration_text = Some(declaration_text.clone()); + if let Some(selector_text) = snapshot.selector_text.as_deref() { + snapshot.css_text = format!("{selector_text} {{ {declaration_text} }}"); + } + } + snapshot } pub(crate) fn rule_type(&self) -> CssRuleType { @@ -508,9 +517,25 @@ impl StylesheetRuleWrapperBinding { } pub(crate) fn css_text(&self) -> String { + if let Some(declaration_text) = self.source_declaration_override() + && let Some(selector_text) = self.rule.style_selector_text(&self.shared_lock) + { + return format!("{selector_text} {{ {declaration_text} }}"); + } self.rule.css_text(&self.shared_lock) } + fn source_declaration_override(&self) -> Option { + if self.stylesheet_id.is_none() { + return self.detached_source_declaration_override.clone(); + } + let source_override = self.source_declaration_override.borrow(); + source_override + .as_ref() + .filter(|source_override| source_override.rule_path == self.path) + .map(|source_override| source_override.declaration_text.clone()) + } + pub(crate) fn style_selector_text(&self) -> Option { self.rule.style_selector_text(&self.shared_lock) } @@ -568,6 +593,7 @@ impl StylesheetRuleWrapperBinding { } fn mark_detached(&mut self) { + self.detached_source_declaration_override = self.source_declaration_override(); self.stylesheet_id = None; self.path.clear(); } @@ -967,6 +993,15 @@ struct StylesheetContentsCacheKey { allow_import_rules: bool, } +#[derive(Clone, Debug, Eq, PartialEq)] +struct LiveStylesheetSourceDeclarationOverride { + rule_path: Vec, + declaration_text: String, +} + +type LiveStylesheetSourceDeclarationOverrideState = + Rc>>; + impl StylesheetContentsCacheKey { fn new( css_text: &str, @@ -993,6 +1028,7 @@ pub(crate) struct SharedStylesheetContents { base_url: url::Url, quirks_mode: QuirksMode, allow_import_rules: AllowImportRules, + source_declaration_override: Option, } /// Renderer-owned identity for one authoritative parsed stylesheet. @@ -1026,6 +1062,7 @@ pub(crate) struct LiveStylesheet { next_font_face_rule_identity: Cell, font_face_rule_identities: RefCell>, shared_initial_contents: RefCell>>, + source_declaration_override: LiveStylesheetSourceDeclarationOverrideState, rule_wrapper_leases: RefCell< HashMap< StylesheetRuleWrapperLeaseId, @@ -1226,6 +1263,8 @@ impl LiveStylesheet { quirks_mode, allow_import_rules, )); + let source_declaration_override = + stylesheet_eof_open_var_declaration_override(&stylesheet, css_text); Self::from_stylesheet( id, stylesheet, @@ -1234,6 +1273,7 @@ impl LiveStylesheet { allow_import_rules, runtime_state_kind, None, + source_declaration_override, ) } @@ -1242,6 +1282,7 @@ impl LiveStylesheet { shared_lock: SharedRwLock, shared_contents: StdArc, ) -> Self { + let source_declaration_override = shared_contents.source_declaration_override.clone(); let media = ServoArc::new(shared_lock.wrap(MediaList::empty())); let stylesheet = ServoArc::new(Stylesheet { contents: shared_lock.wrap(shared_contents.contents.clone()), @@ -1257,6 +1298,7 @@ impl LiveStylesheet { shared_contents.allow_import_rules, LiveStylesheetRuntimeStateKind::Cascade, Some(shared_contents), + source_declaration_override, ) } @@ -1268,6 +1310,7 @@ impl LiveStylesheet { allow_import_rules: AllowImportRules, runtime_state_kind: LiveStylesheetRuntimeStateKind, shared_initial_contents: Option>, + source_declaration_override: Option, ) -> Self { let cssom_runtime_state = match runtime_state_kind { LiveStylesheetRuntimeStateKind::Cascade => LiveStylesheetCssomRuntimeState::Cascade, @@ -1298,6 +1341,7 @@ impl LiveStylesheet { next_font_face_rule_identity: Cell::new(0), font_face_rule_identities: RefCell::new(Vec::new()), shared_initial_contents: RefCell::new(shared_initial_contents), + source_declaration_override: Rc::new(RefCell::new(source_declaration_override)), rule_wrapper_leases: RefCell::new(HashMap::new()), }; stylesheet.reconcile_import_edges(); @@ -1323,6 +1367,7 @@ impl LiveStylesheet { base_url: self.base_url.clone(), quirks_mode: self.quirks_mode, allow_import_rules: self.allow_import_rules, + source_declaration_override: self.source_declaration_override.borrow().clone(), }); *self.shared_initial_contents.borrow_mut() = Some(StdArc::clone(&shared_contents)); shared_contents @@ -1500,9 +1545,16 @@ impl LiveStylesheet { Some(match native_rule { NativeStylesheetRule::Css(CssRule::Style(rule)) => { let rule = rule.read_with(&guard); + let declaration_text = self + .source_declaration_override + .borrow() + .as_ref() + .filter(|source_override| source_override.rule_path == path) + .map(|source_override| source_override.declaration_text.clone()) + .unwrap_or_else(|| serialize_declaration_block(&rule.block, &guard)); LiveStylesheetRuleWrapperSeed::Style { selector_text: cssparser::ToCss::to_css_string(&rule.selectors), - declaration_text: serialize_declaration_block(&rule.block, &guard), + declaration_text, } } NativeStylesheetRule::Keyframe(rule) => { @@ -1934,6 +1986,42 @@ impl LiveStylesheet { } } +fn stylesheet_eof_open_var_declaration_override( + stylesheet: &ServoArc, + css_text: &str, +) -> Option { + let guard = stylesheet.shared_lock.read(); + let contents = stylesheet.contents.read_with(&guard); + let rules = contents.rules.read_with(&guard); + let mut rule_path = vec![rules.0.len().checked_sub(1)?]; + let mut rule = rules.0.last()?.clone(); + + loop { + let children = rule.children(&guard); + let Some(child_index) = children.len().checked_sub(1) else { + break; + }; + rule_path.push(child_index); + rule = children[child_index].clone(); + } + + let CssRule::Style(rule) = rule else { + return None; + }; + let serialized_declaration_text = { + let rule = rule.read_with(&guard); + serialize_declaration_block(&rule.block, &guard) + }; + let declaration_text = moli_css_parse::recover_stylesheet_eof_open_var_declaration_text( + css_text, + &serialized_declaration_text, + )?; + Some(LiveStylesheetSourceDeclarationOverride { + rule_path, + declaration_text, + }) +} + mod import_graph; mod mutation; mod registry; diff --git a/moli-renderer-v8/src/live_stylesheet/mutation.rs b/moli-renderer-v8/src/live_stylesheet/mutation.rs index b84dff0652..37b477822d 100644 --- a/moli-renderer-v8/src/live_stylesheet/mutation.rs +++ b/moli-renderer-v8/src/live_stylesheet/mutation.rs @@ -1189,6 +1189,8 @@ impl LiveStylesheet { self.quirks_mode, self.allow_import_rules, )); + let source_declaration_override = + stylesheet_eof_open_var_declaration_override(&replacement, css_text); let replacement_contents = { let guard = replacement.shared_lock.read(); replacement.contents.read_with(&guard).clone() @@ -1205,9 +1207,11 @@ impl LiveStylesheet { self.reconcile_import_edges(); self.font_face_rule_identities.borrow_mut().clear(); self.note_contents_mutation(); + *self.source_declaration_override.borrow_mut() = source_declaration_override; } pub(crate) fn note_contents_mutation(&self) { + self.source_declaration_override.borrow_mut().take(); self.contents_revision .set(self.contents_revision.get().saturating_add(1)); self.note_cascade_mutation(); diff --git a/moli-renderer-v8/src/live_stylesheet/registry.rs b/moli-renderer-v8/src/live_stylesheet/registry.rs index 9dda572136..c1419aa7a9 100644 --- a/moli-renderer-v8/src/live_stylesheet/registry.rs +++ b/moli-renderer-v8/src/live_stylesheet/registry.rs @@ -192,6 +192,7 @@ impl LiveStylesheetRegistry { allow_import_rules, LiveStylesheetRuntimeStateKind::Cascade, None, + None, )); self.entries .borrow_mut() @@ -525,6 +526,8 @@ impl LiveStylesheetRegistry { path, rule, shared_lock: stylesheet.stylesheet.shared_lock.clone(), + source_declaration_override: Rc::clone(&stylesheet.source_declaration_override), + detached_source_declaration_override: None, }; if let Some(id) = existing_id { diff --git a/moli-renderer-v8/src/live_stylesheet/tests.rs b/moli-renderer-v8/src/live_stylesheet/tests.rs index 5244b739ed..ba4b02e88e 100644 --- a/moli-renderer-v8/src/live_stylesheet/tests.rs +++ b/moli-renderer-v8/src/live_stylesheet/tests.rs @@ -19,6 +19,67 @@ fn serialized_stylesheet_text(stylesheet: &LiveStylesheetRef) -> StdArc { .serialized_css_text() } +fn style_rule_seed_declaration_text(stylesheet: &LiveStylesheet, path: &[usize]) -> String { + let Some(LiveStylesheetRuleWrapperSeed::Style { + declaration_text, .. + }) = stylesheet.rule_wrapper_seed_at_path(path) + else { + panic!("style rule seed should resolve at {path:?}"); + }; + declaration_text +} + +#[test] +fn eof_open_var_cssom_override_is_shared_and_cleared_by_native_mutation() { + let registry = LiveStylesheetRegistry::default(); + let shared_lock = SharedRwLock::new(); + let base_url = url::Url::parse("https://example.test/styles/open-var.css").unwrap(); + let source = ".target { color: red; width: var(--prop"; + let first = registry.create_inline_with_shared_initial_contents( + source, + base_url.clone(), + QuirksMode::NoQuirks, + shared_lock.clone(), + ); + let second = registry.create_inline_with_shared_initial_contents( + source, + base_url, + QuirksMode::NoQuirks, + shared_lock, + ); + + assert_eq!( + style_rule_seed_declaration_text(&first, &[0]), + "color: red; width: var(--prop;" + ); + assert_eq!( + style_rule_seed_declaration_text(&second, &[0]), + "color: red; width: var(--prop;" + ); + assert_eq!( + native_rule_css_text(&first, &[0]), + ".target { color: red; width: var(--prop); }" + ); + + first + .set_style_rule_declarations(&[0], "width: var(--updated);") + .unwrap(); + assert_eq!( + style_rule_seed_declaration_text(&first, &[0]), + "width: var(--updated);" + ); + assert_eq!( + style_rule_seed_declaration_text(&second, &[0]), + "color: red; width: var(--prop;" + ); + + first.replace_from_text(".replacement { height: var(--replacement"); + assert_eq!( + style_rule_seed_declaration_text(&first, &[0]), + "height: var(--replacement;" + ); +} + #[test] fn native_import_edges_own_distinct_children_and_propagate_cascade_changes() { let registry = LiveStylesheetRegistry::default(); diff --git a/moli-renderer-v8/src/native_bridge/element/styles/declaration/parse/pdb_compat.rs b/moli-renderer-v8/src/native_bridge/element/styles/declaration/parse/pdb_compat.rs index c526e4841f..6a5520deb6 100644 --- a/moli-renderer-v8/src/native_bridge/element/styles/declaration/parse/pdb_compat.rs +++ b/moli-renderer-v8/src/native_bridge/element/styles/declaration/parse/pdb_compat.rs @@ -206,6 +206,9 @@ pub(super) fn style_entries_affecting_property( } pub(super) fn cssom_style_property_write_uses_pdb(name: &str, value: &str) -> bool { + if moli_css_parse::css_value_is_eof_open_var_function(value) { + return false; + } let name = canonical_style_property_name(name); if crate::detached_css_style::css_style_declaration_is_shell_property(&name) { return false; diff --git a/moli-renderer-v8/src/script_vm/tests/dom_xhr/cssom/extracted/cssom_interfaces.rs b/moli-renderer-v8/src/script_vm/tests/dom_xhr/cssom/extracted/cssom_interfaces.rs index 5b6079ae83..a266264bc3 100644 --- a/moli-renderer-v8/src/script_vm/tests/dom_xhr/cssom/extracted/cssom_interfaces.rs +++ b/moli-renderer-v8/src/script_vm/tests/dom_xhr/cssom/extracted/cssom_interfaces.rs @@ -66,7 +66,7 @@ fn css_variable_specified_values_preserve_cssom_shorthand_boundaries() { assert_eq!( result, - "margin: var(--prop);,var(--prop),|margin-right: ; margin-bottom: ; margin-left: ; margin-top: 10px;,,,10px|var(--prop),|,,var(--prop) /* keep */ var(--prop),color: var(--prop) /* keep */ var(--prop);|var(--open)|3px,1px,1px,1px" + "margin: var(--prop);,var(--prop),|margin-right: ; margin-bottom: ; margin-left: ; margin-top: 10px;,,,10px|var(--prop),|,,var(--prop) /* keep */ var(--prop),color: var(--prop) /* keep */ var(--prop);|var(--open|3px,1px,1px,1px" ); } #[test] @@ -3189,3 +3189,41 @@ fn webkit_text_stroke_shorthand_uses_pdb_backing_across_cssom_surfaces() { assert_eq!(result, "PASS"); } + +#[test] +fn stylesheet_eof_open_var_preserves_cssom_text_views() { + let mut vm = new_storage_test_vm("https://css-var-stylesheet-eof.test/"); + + let result = vm + .eval( + r#" +(() => { + const style = document.createElement('style'); + style.textContent = 'div { width: var(--open'; + (document.head || document.documentElement || document).appendChild(style); + const rule = style.sheet.cssRules[0]; + + const constructed = new CSSStyleSheet(); + constructed.replaceSync('span { height: var(--retained'); + const retained = constructed.cssRules[0]; + constructed.replaceSync('span { height: 1px; }'); + constructed.cssRules[0].style.width = 'var(--written'; + + return [ + rule.style.getPropertyValue('width'), + rule.style.cssText, + rule.cssText, + retained.style.getPropertyValue('height'), + retained.cssText, + constructed.cssRules[0].style.width + ].join('|'); +})() +"#, + ) + .expect("stylesheet EOF-open var CSSOM views should evaluate"); + + assert_eq!( + result, + "var(--open|width: var(--open;|div { width: var(--open; }|var(--retained|span { height: var(--retained; }|var(--written" + ); +}