From c17bb0141eb9d9c52999bb790b4e40d740be5d01 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Mon, 24 Aug 2026 12:31:56 +0800 Subject: [PATCH] fix(svg): cascade presentation attributes --- .../runtime/page_vm/tests/inline_svg_paint.rs | 44 ++++- .../src/style_engine/mutation_effect.rs | 4 +- .../src/style_engine/tests/lifecycle.rs | 7 + moli-selector/src/lib.rs | 2 +- moli-selector/src/stylo.rs | 1 + moli-selector/src/stylo/presentation.rs | 179 ++++++++++++++++-- moli-selector/src/stylo/query/element.rs | 3 + moli-selector/src/stylo/style_traversal.rs | 3 + 8 files changed, 223 insertions(+), 20 deletions(-) diff --git a/moli-renderer-v8/src/runtime/page_vm/tests/inline_svg_paint.rs b/moli-renderer-v8/src/runtime/page_vm/tests/inline_svg_paint.rs index 5008d3626c..48887e903b 100644 --- a/moli-renderer-v8/src/runtime/page_vm/tests/inline_svg_paint.rs +++ b/moli-renderer-v8/src/runtime/page_vm/tests/inline_svg_paint.rs @@ -17,10 +17,14 @@ html,body{margin:0;background:white} #run{position:absolute;left:0;top:0;width:80px;height:40px;margin:0;padding:0;border:0;background:rgb(174,67,28);color:white} .icon{position:absolute;left:30px;top:10px;display:block;width:20px;height:20px;fill:currentcolor} #stroke{position:absolute;left:100px;top:10px;display:block;width:20px;height:20px;color:rgb(0,128,0);fill:none;stroke:currentcolor;stroke-width:4px;stroke-linecap:butt} +#presentation{position:absolute;left:140px;top:10px;display:block;color:rgb(51,51,51)} `; document.body.innerHTML = ` - -`; + + + + +`; 'installed' "#, )?; @@ -28,15 +32,15 @@ document.body.innerHTML = ` assert_eq!( page_vm.vm_mut().eval( - "[getComputedStyle(document.getElementById('run-icon')).fill,getComputedStyle(document.getElementById('stroke')).stroke].join('|')", + "[getComputedStyle(document.getElementById('run-icon')).fill,getComputedStyle(document.getElementById('stroke')).stroke,getComputedStyle(document.getElementById('presentation')).fill,getComputedStyle(document.getElementById('presentation-shape')).stroke,getComputedStyle(document.getElementById('presentation-shape')).strokeWidth].join('|')", )?, - "rgb(255, 255, 255)|rgb(0, 128, 0)", - "Stylo must resolve the external SVG paint rules before the paint bridge snapshots them", + "rgb(255, 255, 255)|rgb(0, 128, 0)|none|rgb(51, 51, 51)|2px", + "presentation attributes must enter Stylo below author CSS, retain SVG unitless lengths, and inherit currentColor normally", ); let snapshot = page_vm .vm_mut() - .screenshot_layout_snapshot(moli_layout::PaintViewport::new(140, 50, 1.0))? + .screenshot_layout_snapshot(moli_layout::PaintViewport::new(170, 50, 1.0))? .expect("inline SVG computed-paint fixture must retain a layout root"); let raster = moli_paint::raster_snapshot(&snapshot)?; let pixel = |x: u32, y: u32| { @@ -48,6 +52,34 @@ document.body.innerHTML = ` assert_eq!(pixel(15, 20), [174, 67, 28, 255]); assert_eq!(pixel(110, 20), [0, 128, 0, 255]); assert_eq!(pixel(90, 20), [255, 255, 255, 255]); + assert_eq!(pixel(142, 20), [51, 51, 51, 255]); + assert_eq!( + pixel(150, 20), + [255, 255, 255, 255], + "fill=none must not be replaced by SVG's initial black fill" + ); + + page_vm + .vm_mut() + .eval("document.getElementById('presentation').setAttribute('fill','blue')")?; + assert_eq!( + page_vm + .vm_mut() + .eval("getComputedStyle(document.getElementById('presentation')).fill")?, + "rgb(0, 0, 255)", + "changing a presentation attribute must invalidate its computed style", + ); + let mutated = page_vm + .vm_mut() + .screenshot_layout_snapshot(moli_layout::PaintViewport::new(170, 50, 1.0))? + .expect("mutated SVG presentation fixture must retain a layout root"); + let mutated_raster = moli_paint::raster_snapshot(&mutated)?; + let center = ((20 * mutated_raster.width + 150) * 4) as usize; + assert_eq!( + &mutated_raster.rgba[center..center + 4], + [0, 0, 255, 255], + "the invalidated computed fill must reach the inline SVG paint bridge" + ); Ok::<_, anyhow::Error>(()) }) .await diff --git a/moli-renderer-v8/src/style_engine/mutation_effect.rs b/moli-renderer-v8/src/style_engine/mutation_effect.rs index a308e7a915..d7fdbc7ed9 100644 --- a/moli-renderer-v8/src/style_engine/mutation_effect.rs +++ b/moli-renderer-v8/src/style_engine/mutation_effect.rs @@ -163,12 +163,14 @@ pub(crate) enum StyleAttributeImpact { impl StyleAttributeImpact { pub(crate) fn for_attribute_name(name: &str) -> Self { - match name.to_ascii_lowercase().as_str() { + let name = name.to_ascii_lowercase(); + match name.as_str() { "style" | "class" | "id" => Self::ComputedStyle, "hidden" | "width" | "height" | "cols" | "rows" | "size" | "value" | "border" | "slot" | "align" => Self::LayoutMetric, "href" | "rel" | "media" | "blocking" | "disabled" => Self::StylesheetLinkage, "type" => Self::LayoutMetricAndStylesheetLinkage, + _ if moli_selector::is_svg_presentation_attribute_name(&name) => Self::LayoutMetric, _ => Self::None, } } diff --git a/moli-renderer-v8/src/style_engine/tests/lifecycle.rs b/moli-renderer-v8/src/style_engine/tests/lifecycle.rs index a007fc80de..bf9701b0f7 100644 --- a/moli-renderer-v8/src/style_engine/tests/lifecycle.rs +++ b/moli-renderer-v8/src/style_engine/tests/lifecycle.rs @@ -2540,6 +2540,13 @@ fn style_attribute_impact_classifies_dom_and_stylesheet_inputs() { assert!(!StyleAttributeImpact::for_attribute_name("width").changes_computed_style()); assert!(!StyleAttributeImpact::for_attribute_name("width").changes_stylesheet_linkage()); + for attribute in ["fill", "stroke", "stroke-width", "paint-order"] { + assert!( + StyleAttributeImpact::for_attribute_name(attribute).affects_layout_metric(), + "SVG presentation attribute {attribute} must invalidate computed paint" + ); + } + assert!(!StyleAttributeImpact::for_attribute_name("href").affects_layout_metric()); assert!(StyleAttributeImpact::for_attribute_name("href").changes_stylesheet_linkage()); diff --git a/moli-selector/src/lib.rs b/moli-selector/src/lib.rs index 3766b5d738..ad4b835655 100644 --- a/moli-selector/src/lib.rs +++ b/moli-selector/src/lib.rs @@ -56,7 +56,7 @@ pub use stylo::{ StyloSourceStyleInvalidationTargetResultRecord, StyloStateInvalidationRoot, StyloStyleInvalidationQuery, StyloStyleInvalidationSnapshot, StyloStyleInvalidationSnapshotAttribute, StyloStyleSourceScope, - StyloStylesheetSourceScopeFallbackInput, + StyloStylesheetSourceScopeFallbackInput, is_svg_presentation_attribute_name, stylo_attribute_change_can_skip_fallback_without_dependency, stylo_attribute_change_can_use_retained_invalidator, stylo_element_dependency_snapshot, stylo_fallback_roots_plan, stylo_focus_change_invalidation_roots, diff --git a/moli-selector/src/stylo.rs b/moli-selector/src/stylo.rs index 4e8acc11af..4b681dec41 100644 --- a/moli-selector/src/stylo.rs +++ b/moli-selector/src/stylo.rs @@ -75,6 +75,7 @@ pub use invalidation::{ stylo_state_change_can_use_retained_invalidator, stylo_stylesheet_owner_is_in_source_scope, stylo_stylesheet_source_scope_fallback_roots, }; +pub use presentation::is_svg_presentation_attribute_name; pub(crate) use query::html_directionality; use query::{QueryDocument, QueryElement, QueryNode}; #[cfg(test)] diff --git a/moli-selector/src/stylo/presentation.rs b/moli-selector/src/stylo/presentation.rs index 39540997dc..962ebafae4 100644 --- a/moli-selector/src/stylo/presentation.rs +++ b/moli-selector/src/stylo/presentation.rs @@ -8,30 +8,128 @@ use selectors::sink::Push; use style::{ applicable_declarations::ApplicableDeclarationBlock, - properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock}, + context::QuirksMode, + properties::{ + Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId, + SourcePropertyDeclaration, parse_one_declaration_into, + }, rule_tree::{CascadeLevel, CascadeOrigin}, servo_arc::Arc, shared_lock::SharedRwLock, - stylesheets::layer_rule::LayerOrder, + stylesheets::{CssRuleType, Origin, UrlExtraData, layer_rule::LayerOrder}, values::specified::{LengthPercentage, NoCalcLength, NoCalcPercentage}, }; use style_traits::ParsingMode; -use crate::dom::native::Element; +use crate::dom::{ + NodeId, + native::{DomHost, Element}, +}; const SVG_NAMESPACE: &str = "http://www.w3.org/2000/svg"; +// Mirrors Blink's CSSPropertyIdForSVGAttributeName allowlist. These attributes +// participate in the author cascade as presentation hints: author rules and an +// inline style override them, while inheritance observes their parsed CSS +// value. Geometry attributes such as the root SVG width/height are handled +// separately below because they have element-specific SVG parsing rules. +const SVG_STYLE_PRESENTATION_ATTRIBUTES: &[&str] = &[ + "alignment-baseline", + "baseline-shift", + "buffered-rendering", + "clip", + "clip-path", + "clip-rule", + "color", + "color-interpolation", + "color-interpolation-filters", + "color-rendering", + "cursor", + "direction", + "display", + "dominant-baseline", + "fill", + "fill-opacity", + "fill-rule", + "filter", + "flood-color", + "flood-opacity", + "font-family", + "font-size", + "font-stretch", + "font-style", + "font-variant", + "font-weight", + "image-rendering", + "letter-spacing", + "lighting-color", + "marker-end", + "marker-mid", + "marker-start", + "mask", + "mask-type", + "opacity", + "overflow", + "paint-order", + "pointer-events", + "shape-rendering", + "stop-color", + "stop-opacity", + "stroke", + "stroke-dasharray", + "stroke-dashoffset", + "stroke-linecap", + "stroke-linejoin", + "stroke-miterlimit", + "stroke-opacity", + "stroke-width", + "text-anchor", + "text-decoration", + "text-rendering", + "transform-origin", + "unicode-bidi", + "vector-effect", + "visibility", + "word-spacing", + "writing-mode", +]; + +/// Whether changing an attribute can change an SVG element's computed style +/// without any selector dependency on that attribute. +pub fn is_svg_presentation_attribute_name(name: &str) -> bool { + matches!(name, "width" | "height") || SVG_STYLE_PRESENTATION_ATTRIBUTES.contains(&name) +} + pub(super) fn synthesize_svg_presentational_hints( + host: &DomHost, + handle: NodeId, element: &Element, + quirks_mode: QuirksMode, shared_lock: &SharedRwLock, hints: &mut V, ) where V: Push, { - if element.namespace() != SVG_NAMESPACE || element.local_name() != "svg" { + if element.namespace() != SVG_NAMESPACE { return; } + let mut block = PropertyDeclarationBlock::new(); + if element.local_name() == "svg" { + append_root_svg_size_declarations(element, &mut block); + } + append_svg_style_presentation_declarations(host, handle, element, quirks_mode, &mut block); + + if !block.is_empty() { + hints.push(ApplicableDeclarationBlock::from_declarations( + Arc::new(shared_lock.wrap(block)), + CascadeLevel::new(CascadeOrigin::PresHints), + LayerOrder::root(), + )); + } +} + +fn append_root_svg_size_declarations(element: &Element, block: &mut PropertyDeclarationBlock) { for (attribute, is_width) in [("width", true), ("height", false)] { let Some(value) = element.attribute(attribute) else { continue; @@ -46,14 +144,50 @@ pub(super) fn synthesize_svg_presentational_hints( } else { PropertyDeclaration::Height(size) }; - hints.push(ApplicableDeclarationBlock::from_declarations( - Arc::new(shared_lock.wrap(PropertyDeclarationBlock::with_one( - declaration, - Importance::Normal, - ))), - CascadeLevel::new(CascadeOrigin::PresHints), - LayerOrder::root(), - )); + block.push(declaration, Importance::Normal); + } +} + +fn append_svg_style_presentation_declarations( + host: &DomHost, + handle: NodeId, + element: &Element, + quirks_mode: QuirksMode, + block: &mut PropertyDeclarationBlock, +) { + let Some(base_url) = host + .owner_document_handle(handle) + .and_then(|document| host.document_base_url_for_handle(document)) + else { + return; + }; + let url_data = UrlExtraData::from(base_url); + + for attribute in element.attributes() { + if !attribute.namespace().is_empty() + || !SVG_STYLE_PRESENTATION_ATTRIBUTES.contains(&attribute.local_name()) + { + continue; + } + let Ok(property) = PropertyId::parse_enabled_for_all_content(attribute.local_name()) else { + continue; + }; + let mut declarations = SourcePropertyDeclaration::default(); + if parse_one_declaration_into( + &mut declarations, + property, + attribute.value(), + Origin::Author, + &url_data, + None, + ParsingMode::ALLOW_UNITLESS_LENGTH | ParsingMode::ALLOW_ALL_NUMERIC_VALUES, + quirks_mode, + CssRuleType::Style, + ) + .is_ok() + { + block.extend(declarations.drain(), Importance::Normal); + } } } @@ -104,4 +238,25 @@ mod tests { assert!(parse_svg_size_attribute("auto").is_none()); assert!(parse_svg_size_attribute("-1em").is_none()); } + + #[test] + fn svg_paint_attributes_are_classified_as_presentational() { + for name in [ + "fill", + "fill-opacity", + "stroke", + "stroke-width", + "paint-order", + "shape-rendering", + "width", + "height", + ] { + assert!( + is_svg_presentation_attribute_name(name), + "{name} must invalidate presentation hints when mutated" + ); + } + assert!(!is_svg_presentation_attribute_name("viewBox")); + assert!(!is_svg_presentation_attribute_name("d")); + } } diff --git a/moli-selector/src/stylo/query/element.rs b/moli-selector/src/stylo/query/element.rs index 5a00bf7f99..a5e812e16e 100644 --- a/moli-selector/src/stylo/query/element.rs +++ b/moli-selector/src/stylo/query/element.rs @@ -402,7 +402,10 @@ impl<'a> TElement for QueryElement<'a> { V: selectors::sink::Push, { super::super::presentation::synthesize_svg_presentational_hints( + self.host, + self.handle, self.element(), + self.read_quirks_mode(), self.shared_lock, hints, ); diff --git a/moli-selector/src/stylo/style_traversal.rs b/moli-selector/src/stylo/style_traversal.rs index fbe3eb9977..9e8beb364d 100644 --- a/moli-selector/src/stylo/style_traversal.rs +++ b/moli-selector/src/stylo/style_traversal.rs @@ -2009,7 +2009,10 @@ impl<'a> TElement for StyleElement<'a> { V: selectors::sink::Push, { super::presentation::synthesize_svg_presentational_hints( + self.host(), + self.handle(), self.element(), + self.as_query().read_quirks_mode(), &self.style_state().shared_lock, hints, );