[librsvg: 3/5] Do not constrain units for x/y/width/height in filters based on objectBoundingBox

[Federico Mena Quintero <federico src gnome org>]

commit c82e9ed640e439d143e1966833b573874786e2b9
Author: Federico Mena Quintero <federico gnome org>
Date:   Fri Apr 9 16:50:17 2021 -0500

    Do not constrain units for x/y/width/height in filters based on objectBoundingBox
    
    I can't find anything in the spec that indicates that using
    objectBoundingBox for filterUnits or primitiveUnits means that
    x/y/width/height can only be numbers or percentages.  I interpret the
    spec to mean that for objectBoundingBox, well, those lengths *are*
    normalized with respect to the bbox of the object being filtered.
    
    This changes the test for filter-effects-region.svg once again.  The
    reference still disagrees with WebKit / Firefox; I'll file a bug about that.

 src/filter.rs                                      |  72 +++------------------
 src/filters/error.rs                               |   6 --
 src/filters/mod.rs                                 |  19 ------
 .../reftests/filter-effects-region-ref.png         | Bin 1669 -> 1698 bytes
 4 files changed, 10 insertions(+), 87 deletions(-)
---
diff --git a/src/filter.rs b/src/filter.rs
index a1568bfc..a6cae1eb 100644
--- a/src/filter.rs
+++ b/src/filter.rs
@@ -1,14 +1,14 @@
 //! The `filter` element.
 
 use cssparser::Parser;
-use markup5ever::{expanded_name, local_name, namespace_url, ns, QualName};
+use markup5ever::{expanded_name, local_name, namespace_url, ns};
 use std::slice::Iter;
 
 use crate::coord_units::CoordUnits;
 use crate::document::{AcquiredNodes, NodeId};
 use crate::drawing_ctx::ViewParams;
 use crate::element::{Draw, Element, ElementResult, SetAttributes};
-use crate::error::{ElementError, ValueErrorKind};
+use crate::error::ValueErrorKind;
 use crate::length::*;
 use crate::node::NodeBorrow;
 use crate::parsers::{Parse, ParseValue};
@@ -57,23 +57,10 @@ impl Filter {
     }
 
     pub fn resolve(&self, values: &ComputedValues, params: &ViewParams) -> ResolvedFilter {
-        // With filterunits == ObjectBoundingBox, lengths represent fractions or percentages of the
-        // referencing node. No units are allowed (it's checked during attribute parsing).
-        let (x, y, w, h) = if self.filter_units == CoordUnits::ObjectBoundingBox {
-            (
-                self.x.length,
-                self.y.length,
-                self.width.length,
-                self.height.length,
-            )
-        } else {
-            (
-                self.x.normalize(values, &params),
-                self.y.normalize(values, &params),
-                self.width.normalize(values, &params),
-                self.height.normalize(values, &params),
-            )
-        };
+        let x = self.x.normalize(values, &params);
+        let y = self.y.normalize(values, &params);
+        let w = self.width.normalize(values, &params);
+        let h = self.height.normalize(values, &params);
 
         let rect = Rect::new(x, y, x + w, y + h);
 
@@ -96,32 +83,13 @@ impl SetAttributes for Filter {
             self.filter_units = filter_units
         }
 
-        // With ObjectBoundingBox, only fractions and percents are allowed.
-        let units_allowed = self.filter_units != CoordUnits::ObjectBoundingBox;
-
         // Parse the rest of the attributes.
         for (attr, value) in attrs.iter() {
             match attr.expanded() {
-                expanded_name!("", "x") => {
-                    self.x = attr
-                        .parse(value)
-                        .and_then(|v| check_units(v, units_allowed, attr))?
-                }
-                expanded_name!("", "y") => {
-                    self.y = attr
-                        .parse(value)
-                        .and_then(|v| check_units(v, units_allowed, attr))?
-                }
-                expanded_name!("", "width") => {
-                    self.width = attr
-                        .parse(value)
-                        .and_then(|v| check_units(v, units_allowed, attr))?
-                }
-                expanded_name!("", "height") => {
-                    self.height = attr
-                        .parse(value)
-                        .and_then(|v| check_units(v, units_allowed, attr))?
-                }
+                expanded_name!("", "x") => self.x = attr.parse(value)?,
+                expanded_name!("", "y") => self.y = attr.parse(value)?,
+                expanded_name!("", "width") => self.width = attr.parse(value)?,
+                expanded_name!("", "height") => self.height = attr.parse(value)?,
                 expanded_name!("", "primitiveUnits") => self.primitive_units = attr.parse(value)?,
                 _ => (),
             }
@@ -131,26 +99,6 @@ impl SetAttributes for Filter {
     }
 }
 
-fn check_units<N: Normalize, V: Validate>(
-    length: CssLength<N, V>,
-    units_allowed: bool,
-    attr: QualName,
-) -> Result<CssLength<N, V>, ElementError> {
-    if units_allowed {
-        return Ok(length);
-    }
-
-    match length.unit {
-        LengthUnit::Px | LengthUnit::Percent => Ok(length),
-        _ => Err(ElementError {
-            attr,
-            err: ValueErrorKind::parse_error(
-                "unit identifiers are not allowed with filterUnits set to objectBoundingBox",
-            ),
-        }),
-    }
-}
-
 impl Draw for Filter {}
 
 #[derive(Debug, Clone, PartialEq)]
diff --git a/src/filters/error.rs b/src/filters/error.rs
index 2e30ce1a..3ca64bb1 100644
--- a/src/filters/error.rs
+++ b/src/filters/error.rs
@@ -5,8 +5,6 @@ use crate::error::RenderingError;
 /// An enumeration of errors that can occur during filter primitive rendering.
 #[derive(Debug, Clone)]
 pub enum FilterError {
-    /// The units on the filter bounds are invalid
-    InvalidUnits,
     /// The filter was passed invalid input (the `in` attribute).
     InvalidInput,
     /// The filter was passed an invalid parameter.
@@ -31,10 +29,6 @@ pub enum FilterError {
 impl fmt::Display for FilterError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match *self {
-            FilterError::InvalidUnits => write!(
-                f,
-                "unit identifiers are not allowed with primitiveUnits set to objectBoundingBox"
-            ),
             FilterError::InvalidInput => write!(f, "invalid value of the `in` attribute"),
             FilterError::InvalidParameter(ref s) => write!(f, "invalid parameter value: {}", s),
             FilterError::BadInputSurfaceStatus(ref status) => {
diff --git a/src/filters/mod.rs b/src/filters/mod.rs
index 1ffd59a9..44614969 100644
--- a/src/filters/mod.rs
+++ b/src/filters/mod.rs
@@ -5,7 +5,6 @@ use markup5ever::{expanded_name, local_name, namespace_url, ns};
 use std::time::Instant;
 
 use crate::bbox::BoundingBox;
-use crate::coord_units::CoordUnits;
 use crate::document::AcquiredNodes;
 use crate::drawing_ctx::DrawingCtx;
 use crate::element::{Draw, ElementResult, SetAttributes};
@@ -139,14 +138,6 @@ impl Primitive {
         values: &ComputedValues,
         draw_ctx: &DrawingCtx,
     ) -> Result<ResolvedPrimitive, FilterError> {
-        // With ObjectBoundingBox, only fractions and percents are allowed.
-        if ctx.primitive_units() == CoordUnits::ObjectBoundingBox {
-            check_px_or_percent_units(self.x)?;
-            check_px_or_percent_units(self.y)?;
-            check_px_or_percent_units(self.width)?;
-            check_px_or_percent_units(self.height)?;
-        }
-
         let params = draw_ctx.push_coord_units(ctx.primitive_units());
 
         let x = self.x.map(|l| l.normalize(values, &params));
@@ -172,16 +163,6 @@ impl ResolvedPrimitive {
     }
 }
 
-fn check_px_or_percent_units<N: Normalize, V: Validate>(
-    length: Option<CssLength<N, V>>,
-) -> Result<(), FilterError> {
-    match length {
-        Some(l) if l.unit == LengthUnit::Px || l.unit == LengthUnit::Percent => Ok(()),
-        Some(_) => Err(FilterError::InvalidUnits),
-        None => Ok(()),
-    }
-}
-
 impl Primitive {
     fn parse_standard_attributes(
         &mut self,
diff --git a/tests/fixtures/reftests/filter-effects-region-ref.png 
b/tests/fixtures/reftests/filter-effects-region-ref.png
index bf3086cd..79559741 100644
Binary files a/tests/fixtures/reftests/filter-effects-region-ref.png and 
b/tests/fixtures/reftests/filter-effects-region-ref.png differ