[librsvg: 2/3] (#568): Support href and xlink:href for URL references everywhere

[Federico Mena Quintero <federico src gnome org>]

commit c70bc8363ce3fb62dacfb9fcf3bf353f722fed4c
Author: Federico Mena Quintero <federico gnome org>
Date:   Tue Aug 18 19:56:28 2020 -0500

    (#568): Support href and xlink:href for URL references everywhere
    
    SVG2 favors a plain "href" attribute instead of the namespaced
    "xlink:href" for things like <use href="#some_element">.  This commit
    adds support for that everywhere...
    
    ... except in the <tref> element, which got removed in SVG2.  That one
    still expects xlink:href as per SVG 1.1.
    
    Fixes https://gitlab.gnome.org/GNOME/librsvg/-/issues/568

 librsvg_crate/tests/bugs.rs         | 52 +++++++++++++++++++++++++++++++++++++
 rsvg_internals/src/filters/image.rs |  7 ++---
 rsvg_internals/src/gradient.rs      |  9 +++++--
 rsvg_internals/src/href.rs          |  5 ++--
 rsvg_internals/src/image.rs         |  9 ++++---
 rsvg_internals/src/pattern.rs       |  9 +++++--
 rsvg_internals/src/structure.rs     | 12 ++++++---
 rsvg_internals/src/text.rs          |  3 +++
 8 files changed, 88 insertions(+), 18 deletions(-)
---
diff --git a/librsvg_crate/tests/bugs.rs b/librsvg_crate/tests/bugs.rs
index 0743ec951..33aa96602 100644
--- a/librsvg_crate/tests/bugs.rs
+++ b/librsvg_crate/tests/bugs.rs
@@ -75,3 +75,55 @@ fn nonexistent_image_shouldnt_cancel_rendering() {
         "nonexistent_image_shouldnt_cancel_rendering",
     );
 }
+
+// https://gitlab.gnome.org/GNOME/librsvg/-/issues/568
+#[test]
+fn href_attribute_overrides_xlink_href() {
+    let svg = load_svg(
+        br##"<?xml version="1.0" encoding="UTF-8"?>
+<svg xmlns="http://www.w3.org/2000/svg";
+     xmlns:xlink="http://www.w3.org/1999/xlink";
+     width="500" height="500">
+  <defs>
+    <rect id="one" x="100" y="100" width="100" height="100" fill="red"/>
+    <rect id="two" x="100" y="100" width="100" height="100" fill="lime"/>
+  </defs>
+
+  <!-- Per https://svgwg.org/svg2-draft/linking.html#XLinkRefAttrs a plain
+       href attribute overrides an xlink:href one in SVG2 -->
+  <use xlink:href="#one" href="#two"/>
+</svg>
+"##,
+    );
+
+    let output_surf = render_document(
+        &svg,
+        SurfaceSize(500, 500),
+        |_| (),
+        cairo::Rectangle {
+            x: 0.0,
+            y: 0.0,
+            width: 500.0,
+            height: 500.0,
+        },
+    )
+    .unwrap();
+
+    let reference_surf = cairo::ImageSurface::create(cairo::Format::ARgb32, 500, 500).unwrap();
+
+    {
+        let cr = cairo::Context::new(&reference_surf);
+
+        cr.rectangle(100.0, 100.0, 100.0, 100.0);
+        cr.set_source_rgba(0.0, 1.0, 0.0, 1.0);
+        cr.fill();
+    }
+
+    let reference_surf = SharedImageSurface::wrap(reference_surf, SurfaceType::SRgb).unwrap();
+
+    compare_to_surface(
+        &output_surf,
+        &reference_surf,
+        "href_attribute_overrides_xlink_href",
+    );
+}
diff --git a/rsvg_internals/src/filters/image.rs b/rsvg_internals/src/filters/image.rs
index f588d5acf..9bfe04d0e 100644
--- a/rsvg_internals/src/filters/image.rs
+++ b/rsvg_internals/src/filters/image.rs
@@ -6,6 +6,7 @@ use crate::document::AcquiredNodes;
 use crate::drawing_ctx::DrawingCtx;
 use crate::element::{ElementResult, SetAttributes};
 use crate::error::*;
+use crate::href::{is_href, set_href};
 use crate::node::{CascadedValues, Node};
 use crate::parsers::ParseValue;
 use crate::property_bag::PropertyBag;
@@ -118,12 +119,12 @@ impl SetAttributes for FeImage {
                 expanded_name!("", "preserveAspectRatio") => self.aspect = attr.parse(value)?,
 
                 // "path" is used by some older Adobe Illustrator versions
-                expanded_name!(xlink "href") | expanded_name!("", "path") => {
+                ref a if is_href(a) || *a == expanded_name!("", "path") => {
                     let href = Href::parse(value)
                         .map_err(ValueErrorKind::from)
-                        .attribute(attr)?;
+                        .attribute(attr.clone())?;
 
-                    self.href = Some(href);
+                    set_href(a, &mut self.href, href);
                 }
 
                 _ => (),
diff --git a/rsvg_internals/src/gradient.rs b/rsvg_internals/src/gradient.rs
index 29790301e..9e6fe03c4 100644
--- a/rsvg_internals/src/gradient.rs
+++ b/rsvg_internals/src/gradient.rs
@@ -14,6 +14,7 @@ use crate::document::{AcquiredNodes, NodeStack};
 use crate::drawing_ctx::{DrawingCtx, ViewParams};
 use crate::element::{Draw, Element, ElementResult, SetAttributes};
 use crate::error::*;
+use crate::href::{is_href, set_href};
 use crate::length::*;
 use crate::node::{CascadedValues, Node, NodeBorrow};
 use crate::paint_server::{AsPaintSource, PaintSource};
@@ -598,8 +599,12 @@ impl SetAttributes for Common {
                     self.transform = Some(attr.parse(value)?)
                 }
                 expanded_name!("", "spreadMethod") => self.spread = Some(attr.parse(value)?),
-                expanded_name!(xlink "href") => {
-                    self.fallback = Some(Fragment::parse(value).attribute(attr)?)
+                ref a if is_href(a) => {
+                    set_href(
+                        a,
+                        &mut self.fallback,
+                        Fragment::parse(value).attribute(attr.clone())?,
+                    );
                 }
                 _ => (),
             }
diff --git a/rsvg_internals/src/href.rs b/rsvg_internals/src/href.rs
index c5bf466e4..1ad294bdf 100644
--- a/rsvg_internals/src/href.rs
+++ b/rsvg_internals/src/href.rs
@@ -7,7 +7,7 @@
 //! If an element has both `xlink:href` and `href` attributes, the `href` overrides the
 //! other.  We implement that logic in this module.
 
-use markup5ever::{ExpandedName, expanded_name, local_name, namespace_url, ns};
+use markup5ever::{expanded_name, local_name, namespace_url, ns, ExpandedName};
 
 /// Returns whether the attribute is either of `xlink:href` or `href`.
 ///
@@ -33,8 +33,7 @@ use markup5ever::{ExpandedName, expanded_name, local_name, namespace_url, ns};
 /// ```
 pub fn is_href(name: &ExpandedName) -> bool {
     match *name {
-        expanded_name!(xlink "href")
-            | expanded_name!("", "href") => true,
+        expanded_name!(xlink "href") | expanded_name!("", "href") => true,
         _ => false,
     }
 }
diff --git a/rsvg_internals/src/image.rs b/rsvg_internals/src/image.rs
index dd5616c98..f17f94674 100644
--- a/rsvg_internals/src/image.rs
+++ b/rsvg_internals/src/image.rs
@@ -9,6 +9,7 @@ use crate::document::AcquiredNodes;
 use crate::drawing_ctx::{ClipMode, DrawingCtx, ViewParams};
 use crate::element::{Draw, ElementResult, SetAttributes};
 use crate::error::*;
+use crate::href::{is_href, set_href};
 use crate::length::*;
 use crate::node::{CascadedValues, Node};
 use crate::parsers::ParseValue;
@@ -42,12 +43,12 @@ impl SetAttributes for Image {
                 expanded_name!("", "preserveAspectRatio") => self.aspect = attr.parse(value)?,
 
                 // "path" is used by some older Adobe Illustrator versions
-                expanded_name!(xlink "href") | expanded_name!("", "path") => {
+                ref a if is_href(a) || *a == expanded_name!("", "path") => {
                     let href = Href::parse(value)
                         .map_err(|_| ValueErrorKind::parse_error("could not parse href"))
-                        .attribute(attr)?;
+                        .attribute(attr.clone())?;
 
-                    self.href = Some(href);
+                    set_href(a, &mut self.href, href);
                 }
 
                 _ => (),
@@ -81,7 +82,7 @@ impl Draw for Image {
             Href::PlainUrl(ref url) => url,
             Href::WithFragment(_) => {
                 rsvg_log!(
-                    "not rendering {} because its xlink:href cannot contain a fragment identifier",
+                    "not rendering {} because its href cannot contain a fragment identifier",
                     node
                 );
 
diff --git a/rsvg_internals/src/pattern.rs b/rsvg_internals/src/pattern.rs
index 9ba314c9b..9ce96233e 100644
--- a/rsvg_internals/src/pattern.rs
+++ b/rsvg_internals/src/pattern.rs
@@ -13,6 +13,7 @@ use crate::drawing_ctx::{DrawingCtx, ViewParams};
 use crate::element::{Draw, Element, ElementResult, SetAttributes};
 use crate::error::*;
 use crate::float_eq_cairo::ApproxEqCairo;
+use crate::href::{is_href, set_href};
 use crate::length::*;
 use crate::node::{CascadedValues, Node, NodeBorrow, NodeDraw, WeakNode};
 use crate::paint_server::{AsPaintSource, PaintSource};
@@ -133,8 +134,12 @@ impl SetAttributes for Pattern {
                 expanded_name!("", "patternTransform") => {
                     self.common.affine = Some(attr.parse(value)?)
                 }
-                expanded_name!(xlink "href") => {
-                    self.fallback = Some(Fragment::parse(value).attribute(attr)?);
+                ref a if is_href(a) => {
+                    set_href(
+                        a,
+                        &mut self.fallback,
+                        Fragment::parse(value).attribute(attr.clone())?,
+                    );
                 }
                 expanded_name!("", "x") => self.common.x = Some(attr.parse(value)?),
                 expanded_name!("", "y") => self.common.y = Some(attr.parse(value)?),
diff --git a/rsvg_internals/src/structure.rs b/rsvg_internals/src/structure.rs
index 2ec8342fe..9533476dc 100644
--- a/rsvg_internals/src/structure.rs
+++ b/rsvg_internals/src/structure.rs
@@ -11,6 +11,7 @@ use crate::dpi::Dpi;
 use crate::drawing_ctx::{ClipMode, DrawingCtx, ViewParams};
 use crate::element::{Draw, ElementResult, SetAttributes};
 use crate::error::*;
+use crate::href::{is_href, set_href};
 use crate::length::*;
 use crate::node::{CascadedValues, Node, NodeBorrow, NodeDraw};
 use crate::parsers::{Parse, ParseValue};
@@ -309,9 +310,12 @@ impl SetAttributes for Use {
     fn set_attributes(&mut self, pbag: &PropertyBag<'_>) -> ElementResult {
         for (attr, value) in pbag.iter() {
             match attr.expanded() {
-                expanded_name!(xlink "href") => {
-                    self.link = Some(Fragment::parse(value).attribute(attr)?)
-                }
+                ref a if is_href(a) => set_href(
+                    a,
+                    &mut self.link,
+                    Fragment::parse(value).attribute(attr.clone())?,
+                ),
+
                 expanded_name!("", "x") => self.x = attr.parse(value)?,
                 expanded_name!("", "y") => self.y = attr.parse(value)?,
                 expanded_name!("", "width") => {
@@ -489,7 +493,7 @@ impl SetAttributes for Link {
     fn set_attributes(&mut self, pbag: &PropertyBag<'_>) -> ElementResult {
         for (attr, value) in pbag.iter() {
             match attr.expanded() {
-                expanded_name!(xlink "href") => self.link = Some(value.to_owned()),
+                ref a if is_href(a) => set_href(a, &mut self.link, value.to_owned()),
                 _ => (),
             }
         }
diff --git a/rsvg_internals/src/text.rs b/rsvg_internals/src/text.rs
index 1888d044d..cca06ddd8 100644
--- a/rsvg_internals/src/text.rs
+++ b/rsvg_internals/src/text.rs
@@ -721,6 +721,9 @@ impl SetAttributes for TRef {
     fn set_attributes(&mut self, pbag: &PropertyBag<'_>) -> ElementResult {
         for (attr, value) in pbag.iter() {
             match attr.expanded() {
+                // Unlike other elements which use `href` in SVG2 versus `xlink:href` in SVG1.1,
+                // the <tref> element got removed in SVG2.  So, here we still use a match
+                // against the full namespaced version of the attribute.
                 expanded_name!(xlink "href") => {
                     self.link = Some(Fragment::parse(value).attribute(attr)?)
                 }