From 3bfad8f0d2d9ed0a5c739d8c8bbe59acd5672228 Mon Sep 17 00:00:00 2001 From: Laurenz Stampfl <47084093+LaurenzV@users.noreply.github.com> Date: Thu, 7 Nov 2024 23:01:29 +0100 Subject: [PATCH 1/4] Add support for !important attribute --- crates/usvg/src/parser/svgtree/mod.rs | 6 ++- crates/usvg/src/parser/svgtree/parse.rs | 71 ++++++++++++++----------- crates/usvg/tests/parser.rs | 59 ++++++++++++++++++++ 3 files changed, 104 insertions(+), 32 deletions(-) diff --git a/crates/usvg/src/parser/svgtree/mod.rs b/crates/usvg/src/parser/svgtree/mod.rs index edb36f19..5ebc0276 100644 --- a/crates/usvg/src/parser/svgtree/mod.rs +++ b/crates/usvg/src/parser/svgtree/mod.rs @@ -205,14 +205,16 @@ pub struct Attribute<'input> { pub name: AId, /// Attribute's value. pub value: roxmltree::StringStorage<'input>, + /// Attribute's importance + pub important: bool } impl std::fmt::Debug for Attribute<'_> { fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> { write!( f, - "Attribute {{ name: {:?}, value: {} }}", - self.name, self.value + "Attribute {{ name: {:?}, value: {}, important: {} }}", + self.name, self.value, self.important ) } } diff --git a/crates/usvg/src/parser/svgtree/parse.rs b/crates/usvg/src/parser/svgtree/parse.rs index 0c2328ba..bfaf1fe9 100644 --- a/crates/usvg/src/parser/svgtree/parse.rs +++ b/crates/usvg/src/parser/svgtree/parse.rs @@ -49,8 +49,8 @@ impl<'input> Document<'input> { new_child_id } - fn append_attribute(&mut self, name: AId, value: roxmltree::StringStorage<'input>) { - self.attrs.push(Attribute { name, value }); + fn append_attribute(&mut self, name: AId, value: roxmltree::StringStorage<'input>, important: bool) { + self.attrs.push(Attribute { name, value, important }); } } @@ -251,10 +251,10 @@ pub(crate) fn parse_svg_element<'input>( continue; } - append_attribute(parent_id, tag_name, aid, attr.value_storage().clone(), doc); + append_attribute(parent_id, tag_name, aid, attr.value_storage().clone(), false, doc); } - let mut insert_attribute = |aid, value: &str| { + let mut insert_attribute = |aid, value: &str, important: bool| { // Check that attribute already exists. let idx = doc.attrs[attrs_start_idx..] .iter_mut() @@ -266,15 +266,23 @@ pub(crate) fn parse_svg_element<'input>( tag_name, aid, roxmltree::StringStorage::new_owned(value), + important, doc, ); // Check that attribute was actually added, because it could be skipped. if added { if let Some(idx) = idx { - // Swap the last attribute with an existing one. let last_idx = doc.attrs.len() - 1; - doc.attrs.swap(attrs_start_idx + idx, last_idx); + let existing_idx = attrs_start_idx + idx; + + // Swap the last attribute with an existing one. However, if the currently existing + // attribute is important and the new one not, we don't want to overwrite it, since + // the important one takes precedence. + if !(doc.attrs[existing_idx].important && !doc.attrs[last_idx].important) { + doc.attrs.swap(existing_idx, last_idx); + } + // Remove last. doc.attrs.pop(); } @@ -284,40 +292,40 @@ pub(crate) fn parse_svg_element<'input>( let mut write_declaration = |declaration: &Declaration| { // TODO: perform XML attribute normalization if declaration.name == "marker" { - insert_attribute(AId::MarkerStart, declaration.value); - insert_attribute(AId::MarkerMid, declaration.value); - insert_attribute(AId::MarkerEnd, declaration.value); + insert_attribute(AId::MarkerStart, declaration.value, declaration.important); + insert_attribute(AId::MarkerMid, declaration.value, declaration.important); + insert_attribute(AId::MarkerEnd, declaration.value, declaration.important); } else if declaration.name == "font" { if let Ok(shorthand) = FontShorthand::from_str(declaration.value) { // First we need to reset all values to their default. - insert_attribute(AId::FontStyle, "normal"); - insert_attribute(AId::FontVariant, "normal"); - insert_attribute(AId::FontWeight, "normal"); - insert_attribute(AId::FontStretch, "normal"); - insert_attribute(AId::LineHeight, "normal"); - insert_attribute(AId::FontSizeAdjust, "none"); - insert_attribute(AId::FontKerning, "auto"); - insert_attribute(AId::FontVariantCaps, "normal"); - insert_attribute(AId::FontVariantLigatures, "normal"); - insert_attribute(AId::FontVariantNumeric, "normal"); - insert_attribute(AId::FontVariantEastAsian, "normal"); - insert_attribute(AId::FontVariantPosition, "normal"); + insert_attribute(AId::FontStyle, "normal", declaration.important); + insert_attribute(AId::FontVariant, "normal", declaration.important); + insert_attribute(AId::FontWeight, "normal", declaration.important); + insert_attribute(AId::FontStretch, "normal", declaration.important); + insert_attribute(AId::LineHeight, "normal", declaration.important); + insert_attribute(AId::FontSizeAdjust, "none", declaration.important); + insert_attribute(AId::FontKerning, "auto", declaration.important); + insert_attribute(AId::FontVariantCaps, "normal", declaration.important); + insert_attribute(AId::FontVariantLigatures, "normal", declaration.important); + insert_attribute(AId::FontVariantNumeric, "normal", declaration.important); + insert_attribute(AId::FontVariantEastAsian, "normal", declaration.important); + insert_attribute(AId::FontVariantPosition, "normal", declaration.important); // Then, we set the properties that have been declared. shorthand .font_stretch - .map(|s| insert_attribute(AId::FontStretch, s)); + .map(|s| insert_attribute(AId::FontStretch, s, declaration.important)); shorthand .font_weight - .map(|s| insert_attribute(AId::FontWeight, s)); + .map(|s| insert_attribute(AId::FontWeight, s, declaration.important)); shorthand .font_variant - .map(|s| insert_attribute(AId::FontVariant, s)); + .map(|s| insert_attribute(AId::FontVariant, s, declaration.important)); shorthand .font_style - .map(|s| insert_attribute(AId::FontStyle, s)); - insert_attribute(AId::FontSize, shorthand.font_size); - insert_attribute(AId::FontFamily, shorthand.font_family); + .map(|s| insert_attribute(AId::FontStyle, s, declaration.important)); + insert_attribute(AId::FontSize, shorthand.font_size, declaration.important); + insert_attribute(AId::FontFamily, shorthand.font_family, declaration.important); } else { log::warn!( "Failed to parse {} value: '{}'", @@ -328,7 +336,7 @@ pub(crate) fn parse_svg_element<'input>( } else if let Some(aid) = AId::from_str(declaration.name) { // Parse only the presentation attributes. if aid.is_presentation() { - insert_attribute(aid, declaration.value); + insert_attribute(aid, declaration.value, declaration.important); } } }; @@ -369,6 +377,7 @@ fn append_attribute<'input>( tag_name: EId, aid: AId, value: roxmltree::StringStorage<'input>, + important: bool, doc: &mut Document<'input>, ) -> bool { match aid { @@ -389,7 +398,7 @@ fn append_attribute<'input>( return resolve_inherit(parent_id, aid, doc); } - doc.append_attribute(aid, value); + doc.append_attribute(aid, value, important); true } @@ -412,6 +421,7 @@ fn resolve_inherit(parent_id: NodeId, aid: AId, doc: &mut Document) -> bool { doc.attrs.push(Attribute { name: aid, value: attr.value, + important: attr.important }); return true; @@ -429,6 +439,7 @@ fn resolve_inherit(parent_id: NodeId, aid: AId, doc: &mut Document) -> bool { doc.attrs.push(Attribute { name: aid, value: attr.value, + important: attr.important }); return true; @@ -483,7 +494,7 @@ fn resolve_inherit(parent_id: NodeId, aid: AId, doc: &mut Document) -> bool { _ => return false, }; - doc.append_attribute(aid, roxmltree::StringStorage::Borrowed(value)); + doc.append_attribute(aid, roxmltree::StringStorage::Borrowed(value), false); true } diff --git a/crates/usvg/tests/parser.rs b/crates/usvg/tests/parser.rs index ea81afdb..f29e2861 100644 --- a/crates/usvg/tests/parser.rs +++ b/crates/usvg/tests/parser.rs @@ -75,6 +75,65 @@ fn stylesheet_injection() { ); } +#[test] +fn stylesheet_injection_with_important() { + let svg = " + + + + + + +"; + + let stylesheet = "rect { fill: red !important }".to_string(); + + let options = usvg::Options { + style_sheet: Some(stylesheet), + ..usvg::Options::default() + }; + + let tree = usvg::Tree::from_str(&svg, &options).unwrap(); + + let usvg::Node::Path(ref first) = &tree.root().children()[0] else { + unreachable!() + }; + + // All rects should be overriden, since we use `important`. + assert_eq!( + first.fill().unwrap().paint(), + &usvg::Paint::Color(Color::new_rgb(255, 0, 0)) + ); + + let usvg::Node::Path(ref second) = &tree.root().children()[1] else { + unreachable!() + }; + assert_eq!( + second.fill().unwrap().paint(), + &usvg::Paint::Color(Color::new_rgb(255, 0, 0)) + ); + + let usvg::Node::Path(ref third) = &tree.root().children()[2] else { + unreachable!() + }; + assert_eq!( + third.fill().unwrap().paint(), + &usvg::Paint::Color(Color::new_rgb(255, 0, 0)) + ); + + let usvg::Node::Path(ref third) = &tree.root().children()[3] else { + unreachable!() + }; + assert_eq!( + third.fill().unwrap().paint(), + &usvg::Paint::Color(Color::new_rgb(255, 0, 0)) + ); +} + #[test] fn simplify_paths() { let svg = " From 63137a7d8cd33fe8dd52b47f1ca37d825b26feb9 Mon Sep 17 00:00:00 2001 From: Laurenz Stampfl <47084093+LaurenzV@users.noreply.github.com> Date: Thu, 7 Nov 2024 23:06:29 +0100 Subject: [PATCH 2/4] Update reference image --- .../tests/tests/structure/style/important.png | Bin 348 -> 355 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/crates/resvg/tests/tests/structure/style/important.png b/crates/resvg/tests/tests/structure/style/important.png index c0e50934c0cd8aa6a4d8884e3ec48b86a5cb1406..4ca11c8174c232cc2f080efd0d1fa5515f18426b 100644 GIT binary patch literal 355 zcmeAS@N?(olHy`uVBq!ia0y~yVAKI&7G|JGckpC4ASDst6XFV_(Ln=9;Q7ThJAf2h zNswQ#1A~Ble!-dz|7HV4zIeJghE&{od(M%O(LjXZ0{@Ol3@Qz5Tpkbi+!? z_lHLnD=%1v?R3%%`@Z7bqAhB(U#;1?1qs->dB6Us9yN$S=!Hv2psIb^wYNcYvooJ^ eExqB|y3C*Z&|}v<>sPaaLdMh8&t;ucLK6T( Date: Fri, 8 Nov 2024 21:44:31 +0100 Subject: [PATCH 3/4] Use short-hand variable --- crates/usvg/src/parser/svgtree/parse.rs | 49 +++++++++++++------------ 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/crates/usvg/src/parser/svgtree/parse.rs b/crates/usvg/src/parser/svgtree/parse.rs index bfaf1fe9..e20761ef 100644 --- a/crates/usvg/src/parser/svgtree/parse.rs +++ b/crates/usvg/src/parser/svgtree/parse.rs @@ -291,41 +291,44 @@ pub(crate) fn parse_svg_element<'input>( let mut write_declaration = |declaration: &Declaration| { // TODO: perform XML attribute normalization + let imp = declaration.important; + let val = declaration.value; + if declaration.name == "marker" { - insert_attribute(AId::MarkerStart, declaration.value, declaration.important); - insert_attribute(AId::MarkerMid, declaration.value, declaration.important); - insert_attribute(AId::MarkerEnd, declaration.value, declaration.important); + insert_attribute(AId::MarkerStart, val, imp); + insert_attribute(AId::MarkerMid, val, imp); + insert_attribute(AId::MarkerEnd, val, imp); } else if declaration.name == "font" { - if let Ok(shorthand) = FontShorthand::from_str(declaration.value) { + if let Ok(shorthand) = FontShorthand::from_str(val) { // First we need to reset all values to their default. - insert_attribute(AId::FontStyle, "normal", declaration.important); - insert_attribute(AId::FontVariant, "normal", declaration.important); - insert_attribute(AId::FontWeight, "normal", declaration.important); - insert_attribute(AId::FontStretch, "normal", declaration.important); - insert_attribute(AId::LineHeight, "normal", declaration.important); - insert_attribute(AId::FontSizeAdjust, "none", declaration.important); - insert_attribute(AId::FontKerning, "auto", declaration.important); - insert_attribute(AId::FontVariantCaps, "normal", declaration.important); - insert_attribute(AId::FontVariantLigatures, "normal", declaration.important); - insert_attribute(AId::FontVariantNumeric, "normal", declaration.important); - insert_attribute(AId::FontVariantEastAsian, "normal", declaration.important); - insert_attribute(AId::FontVariantPosition, "normal", declaration.important); + insert_attribute(AId::FontStyle, "normal", imp); + insert_attribute(AId::FontVariant, "normal", imp); + insert_attribute(AId::FontWeight, "normal", imp); + insert_attribute(AId::FontStretch, "normal", imp); + insert_attribute(AId::LineHeight, "normal", imp); + insert_attribute(AId::FontSizeAdjust, "none", imp); + insert_attribute(AId::FontKerning, "auto", imp); + insert_attribute(AId::FontVariantCaps, "normal", imp); + insert_attribute(AId::FontVariantLigatures, "normal", imp); + insert_attribute(AId::FontVariantNumeric, "normal", imp); + insert_attribute(AId::FontVariantEastAsian, "normal", imp); + insert_attribute(AId::FontVariantPosition, "normal", imp); // Then, we set the properties that have been declared. shorthand .font_stretch - .map(|s| insert_attribute(AId::FontStretch, s, declaration.important)); + .map(|s| insert_attribute(AId::FontStretch, s, imp)); shorthand .font_weight - .map(|s| insert_attribute(AId::FontWeight, s, declaration.important)); + .map(|s| insert_attribute(AId::FontWeight, s, imp)); shorthand .font_variant - .map(|s| insert_attribute(AId::FontVariant, s, declaration.important)); + .map(|s| insert_attribute(AId::FontVariant, s, imp)); shorthand .font_style - .map(|s| insert_attribute(AId::FontStyle, s, declaration.important)); - insert_attribute(AId::FontSize, shorthand.font_size, declaration.important); - insert_attribute(AId::FontFamily, shorthand.font_family, declaration.important); + .map(|s| insert_attribute(AId::FontStyle, s, imp)); + insert_attribute(AId::FontSize, shorthand.font_size, imp); + insert_attribute(AId::FontFamily, shorthand.font_family, imp); } else { log::warn!( "Failed to parse {} value: '{}'", @@ -336,7 +339,7 @@ pub(crate) fn parse_svg_element<'input>( } else if let Some(aid) = AId::from_str(declaration.name) { // Parse only the presentation attributes. if aid.is_presentation() { - insert_attribute(aid, declaration.value, declaration.important); + insert_attribute(aid, val, imp); } } }; From 7470956809686d1d0e21b5485dc018247e31e24f Mon Sep 17 00:00:00 2001 From: Laurenz Stampfl Date: Sun, 10 Nov 2024 14:31:24 +0100 Subject: [PATCH 4/4] Fix edge case where both attributes are important --- crates/usvg/src/parser/svgtree/mod.rs | 2 +- crates/usvg/src/parser/svgtree/parse.rs | 48 ++++++++++++++++++++----- crates/usvg/tests/parser.rs | 18 ++++++++++ 3 files changed, 58 insertions(+), 10 deletions(-) diff --git a/crates/usvg/src/parser/svgtree/mod.rs b/crates/usvg/src/parser/svgtree/mod.rs index 5ebc0276..f22ac96d 100644 --- a/crates/usvg/src/parser/svgtree/mod.rs +++ b/crates/usvg/src/parser/svgtree/mod.rs @@ -206,7 +206,7 @@ pub struct Attribute<'input> { /// Attribute's value. pub value: roxmltree::StringStorage<'input>, /// Attribute's importance - pub important: bool + pub important: bool, } impl std::fmt::Debug for Attribute<'_> { diff --git a/crates/usvg/src/parser/svgtree/parse.rs b/crates/usvg/src/parser/svgtree/parse.rs index e20761ef..40efb8a7 100644 --- a/crates/usvg/src/parser/svgtree/parse.rs +++ b/crates/usvg/src/parser/svgtree/parse.rs @@ -49,8 +49,17 @@ impl<'input> Document<'input> { new_child_id } - fn append_attribute(&mut self, name: AId, value: roxmltree::StringStorage<'input>, important: bool) { - self.attrs.push(Attribute { name, value, important }); + fn append_attribute( + &mut self, + name: AId, + value: roxmltree::StringStorage<'input>, + important: bool, + ) { + self.attrs.push(Attribute { + name, + value, + important, + }); } } @@ -251,7 +260,14 @@ pub(crate) fn parse_svg_element<'input>( continue; } - append_attribute(parent_id, tag_name, aid, attr.value_storage().clone(), false, doc); + append_attribute( + parent_id, + tag_name, + aid, + attr.value_storage().clone(), + false, + doc, + ); } let mut insert_attribute = |aid, value: &str, important: bool| { @@ -276,10 +292,24 @@ pub(crate) fn parse_svg_element<'input>( let last_idx = doc.attrs.len() - 1; let existing_idx = attrs_start_idx + idx; - // Swap the last attribute with an existing one. However, if the currently existing - // attribute is important and the new one not, we don't want to overwrite it, since - // the important one takes precedence. - if !(doc.attrs[existing_idx].important && !doc.attrs[last_idx].important) { + // See https://developer.mozilla.org/en-US/docs/Web/CSS/important + // When a declaration is important, the order of precedence is reversed. + // Declarations marked as important in the user-agent style sheets override + // all important declarations in the user style sheets. Similarly, all important + // declarations in the user style sheets override all important declarations in the + // author's style sheets. Finally, all important declarations take precedence over + // all animations. + // + // Which means: + // 1) Existing is not important, new is not important -> swap + // 2) Existing is important, new is not important -> don't swap + // 3) Existing is not important, new is important -> swap + // 4) Existing is important, new is important -> don't swap (since the order + // is reversed, so existing important attributes take precedence over new + // important attributes) + let has_precedence = !doc.attrs[existing_idx].important; + + if has_precedence { doc.attrs.swap(existing_idx, last_idx); } @@ -424,7 +454,7 @@ fn resolve_inherit(parent_id: NodeId, aid: AId, doc: &mut Document) -> bool { doc.attrs.push(Attribute { name: aid, value: attr.value, - important: attr.important + important: attr.important, }); return true; @@ -442,7 +472,7 @@ fn resolve_inherit(parent_id: NodeId, aid: AId, doc: &mut Document) -> bool { doc.attrs.push(Attribute { name: aid, value: attr.value, - important: attr.important + important: attr.important, }); return true; diff --git a/crates/usvg/tests/parser.rs b/crates/usvg/tests/parser.rs index f29e2861..94b2f026 100644 --- a/crates/usvg/tests/parser.rs +++ b/crates/usvg/tests/parser.rs @@ -28,6 +28,7 @@ fn stylesheet_injection() { + "; @@ -73,6 +74,14 @@ fn stylesheet_injection() { third.fill().unwrap().paint(), &usvg::Paint::Color(Color::new_rgb(0, 128, 0)) ); + + let usvg::Node::Path(ref third) = &tree.root().children()[3] else { + unreachable!() + }; + assert_eq!( + third.fill().unwrap().paint(), + &usvg::Paint::Color(Color::new_rgb(0, 128, 0)) + ); } #[test] @@ -87,6 +96,7 @@ fn stylesheet_injection_with_important() { + "; @@ -132,6 +142,14 @@ fn stylesheet_injection_with_important() { third.fill().unwrap().paint(), &usvg::Paint::Color(Color::new_rgb(255, 0, 0)) ); + + let usvg::Node::Path(ref third) = &tree.root().children()[4] else { + unreachable!() + }; + assert_eq!( + third.fill().unwrap().paint(), + &usvg::Paint::Color(Color::new_rgb(255, 0, 0)) + ); } #[test]