Bug 1218257 - Cleanup and fix interpolation of SVG lengths. r=boris
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Mar 2019 23:17:12 +0100
changeset 520049 0c76d78aca4851617a655ff34df969893342d801
parent 520048 06f0c5e35c3ab64490bcdfcb25f7ac6ff531ba5e
child 520050 0456bc2c98e2749e932d5d1b20e526a96797e4ca
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersboris
bugs1218257
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1218257 - Cleanup and fix interpolation of SVG lengths. r=boris Instead of storing them as LengthPercentage | Number, always store as LengthPercentage, and use the unitless length quirk to parse numbers instead. Further cleanups to use the rust representation can happen as a followup, which will also get rid of the boolean argument (since we can poke at the rust length itself). That's why I didn't bother to convert it to an enum class yet. Differential Revision: https://phabricator.services.mozilla.com/D21804
dom/svg/SVGContentUtils.cpp
dom/svg/SVGContentUtils.h
layout/svg/SVGTextFrame.cpp
layout/svg/nsSVGUtils.cpp
servo/components/style/properties/gecko.mako.rs
servo/components/style/values/animated/mod.rs
servo/components/style/values/animated/svg.rs
servo/components/style/values/computed/svg.rs
servo/components/style/values/generics/svg.rs
servo/components/style/values/specified/mod.rs
servo/components/style/values/specified/svg.rs
--- a/dom/svg/SVGContentUtils.cpp
+++ b/dom/svg/SVGContentUtils.cpp
@@ -193,17 +193,18 @@ static DashState GetStrokeDashData(
       }
     }
     Float* dashPattern = aStrokeOptions->InitDashPattern(dashArrayLength);
     if (!dashPattern) {
       return eContinuousStroke;
     }
     for (uint32_t i = 0; i < dashArrayLength; i++) {
       Float dashLength =
-          SVGContentUtils::CoordToFloat(aElement, dasharray[i]) * pathScale;
+          SVGContentUtils::CoordToFloat(aElement, dasharray[i], true) *
+          pathScale;
       if (dashLength < 0.0) {
         return eContinuousStroke;  // invalid
       }
       dashPattern[i] = dashLength;
       (i % 2 ? totalLengthOfGaps : totalLengthOfDashes) += dashLength;
     }
   }
 
@@ -237,17 +238,18 @@ static DashState GetStrokeDashData(
       aStyleSVG->mStrokeLinecap == NS_STYLE_STROKE_LINECAP_BUTT) {
     return eNoStroke;
   }
 
   if (aContextPaint && aStyleSVG->StrokeDashoffsetFromObject()) {
     aStrokeOptions->mDashOffset = Float(aContextPaint->GetStrokeDashOffset());
   } else {
     aStrokeOptions->mDashOffset =
-        SVGContentUtils::CoordToFloat(aElement, aStyleSVG->mStrokeDashoffset) *
+        SVGContentUtils::CoordToFloat(aElement, aStyleSVG->mStrokeDashoffset,
+                                      false) *
         pathScale;
   }
 
   return eDashedStroke;
 }
 
 void SVGContentUtils::GetStrokeOptions(AutoStrokeOptions* aStrokeOptions,
                                        SVGElement* aElement,
@@ -338,17 +340,17 @@ Float SVGContentUtils::GetStrokeWidth(SV
   }
 
   const nsStyleSVG* styleSVG = computedStyle->StyleSVG();
 
   if (aContextPaint && styleSVG->StrokeWidthFromObject()) {
     return aContextPaint->GetStrokeWidth();
   }
 
-  return SVGContentUtils::CoordToFloat(aElement, styleSVG->mStrokeWidth);
+  return SVGContentUtils::CoordToFloat(aElement, styleSVG->mStrokeWidth, true);
 }
 
 float SVGContentUtils::GetFontSize(Element* aElement) {
   if (!aElement) {
     return 1.0f;
   }
 
   nsPresContext* pc = nsContentUtils::GetContextForContent(aElement);
@@ -759,32 +761,44 @@ bool SVGContentUtils::ParseInteger(Range
 bool SVGContentUtils::ParseInteger(const nsAString& aString, int32_t& aValue) {
   RangedPtr<const char16_t> iter = GetStartRangedPtr(aString);
   const RangedPtr<const char16_t> end = GetEndRangedPtr(aString);
 
   return ParseInteger(iter, end, aValue) && iter == end;
 }
 
 float SVGContentUtils::CoordToFloat(SVGElement* aContent,
-                                    const nsStyleCoord& aCoord) {
+                                    const nsStyleCoord& aCoord,
+                                    bool aClampNegativeCalc) {
   switch (aCoord.GetUnit()) {
-    case eStyleUnit_Factor:
-      // user units
-      return aCoord.GetFactorValue();
-
     case eStyleUnit_Coord:
       return nsPresContext::AppUnitsToFloatCSSPixels(aCoord.GetCoordValue());
 
     case eStyleUnit_Percent: {
       SVGViewportElement* ctx = aContent->GetCtx();
-      return ctx ? aCoord.GetPercentValue() *
-                       ctx->GetLength(SVGContentUtils::XY)
-                 : 0.0f;
+      if (!ctx) {
+        return 0.0f;
+      }
+      return aCoord.GetPercentValue() * ctx->GetLength(SVGContentUtils::XY);
+    }
+
+    case eStyleUnit_Calc: {
+      auto* calc = aCoord.GetCalcValue();
+      float result = nsPresContext::AppUnitsToFloatCSSPixels(calc->mLength);
+      if (calc->mHasPercent) {
+        SVGViewportElement* ctx = aContent->GetCtx();
+        if (!ctx) {
+          return 0.0f;
+        }
+        result += calc->mPercent * ctx->GetLength(SVGContentUtils::XY);
+      }
+      return aClampNegativeCalc ? std::max(result, 0.0f) : result;
     }
     default:
+      MOZ_ASSERT_UNREACHABLE("Unknown unit for SVG length");
       return 0.0f;
   }
 }
 
 already_AddRefed<gfx::Path> SVGContentUtils::GetPath(
     const nsAString& aPathString) {
   SVGPathData pathData;
   SVGPathDataParser parser(aPathString, &pathData);
--- a/dom/svg/SVGContentUtils.h
+++ b/dom/svg/SVGContentUtils.h
@@ -313,22 +313,22 @@ class SVGContentUtils {
    * Parse an integer of the form:
    * integer ::= [+-]? [0-9]+
    * The returned number is clamped to an int32_t if outside that range.
    * Parsing fails if there is anything left over after the number.
    */
   static bool ParseInteger(const nsAString& aString, int32_t& aValue);
 
   /**
-   * Converts an nsStyleCoord into a userspace value.  Handles units
-   * Factor (straight userspace), Coord (dimensioned), and Percent (of
-   * aContent's SVG viewport)
+   * Converts an nsStyleCoord into a userspace value, resolving percentage
+   * values relative to aContent's SVG viewport.
    */
   static float CoordToFloat(dom::SVGElement* aContent,
-                            const nsStyleCoord& aCoord);
+                            const nsStyleCoord& aCoord,
+                            bool aClampNegativeCalc);
   /**
    * Parse the SVG path string
    * Returns a path
    * string formatted as an SVG path
    */
   static already_AddRefed<mozilla::gfx::Path> GetPath(
       const nsAString& aPathString);
 
--- a/layout/svg/SVGTextFrame.cpp
+++ b/layout/svg/SVGTextFrame.cpp
@@ -2366,18 +2366,18 @@ bool CharIterator::IsOriginalCharTrimmed
     // Since we do a lot of trim checking, we cache the trimmed offsets and
     // lengths while we are in the same frame.
     mFrameForTrimCheck = TextFrame();
     uint32_t offset = mFrameForTrimCheck->GetContentOffset();
     uint32_t length = mFrameForTrimCheck->GetContentLength();
     nsIContent* content = mFrameForTrimCheck->GetContent();
     nsTextFrame::TrimmedOffsets trim = mFrameForTrimCheck->GetTrimmedOffsets(
         content->GetText(),
-            (mPostReflow ? nsTextFrame::TrimmedOffsetFlags::kDefaultTrimFlags :
-                           nsTextFrame::TrimmedOffsetFlags::kNotPostReflow));
+        (mPostReflow ? nsTextFrame::TrimmedOffsetFlags::kDefaultTrimFlags
+                     : nsTextFrame::TrimmedOffsetFlags::kNotPostReflow));
     TrimOffsets(offset, length, trim);
     mTrimmedOffset = offset;
     mTrimmedLength = length;
   }
 
   // A character is trimmed if it is outside the mTrimmedOffset/mTrimmedLength
   // range and it is not a significant newline character.
   uint32_t index = mSkipCharsIterator.GetOriginalOffset();
@@ -3817,19 +3817,19 @@ nsresult SVGTextFrame::GetSubStringLengt
 
     // Offset into frame's nsTextNode:
     const uint32_t untrimmedOffset = frame->GetContentOffset();
     const uint32_t untrimmedLength = frame->GetContentEnd() - untrimmedOffset;
 
     // Trim the offset/length to remove any leading/trailing white space.
     uint32_t trimmedOffset = untrimmedOffset;
     uint32_t trimmedLength = untrimmedLength;
-    nsTextFrame::TrimmedOffsets trimmedOffsets =
-        frame->GetTrimmedOffsets(frame->GetContent()->GetText(),
-            nsTextFrame::TrimmedOffsetFlags::kNotPostReflow);
+    nsTextFrame::TrimmedOffsets trimmedOffsets = frame->GetTrimmedOffsets(
+        frame->GetContent()->GetText(),
+        nsTextFrame::TrimmedOffsetFlags::kNotPostReflow);
     TrimOffsets(trimmedOffset, trimmedLength, trimmedOffsets);
 
     textElementCharIndex += trimmedOffset - untrimmedOffset;
 
     if (textElementCharIndex >= charnum + nchars) {
       break;  // we're past the end of the substring
     }
 
@@ -5001,17 +5001,17 @@ bool SVGTextFrame::ShouldRenderAsPath(ns
         (style->mFill.Type() == eStyleSVGPaintType_Color &&
          style->mFillOpacity == 1))) {
     return true;
   }
 
   // Text has a stroke.
   if (style->HasStroke() &&
       SVGContentUtils::CoordToFloat(static_cast<SVGElement*>(GetContent()),
-                                    style->mStrokeWidth) > 0) {
+                                    style->mStrokeWidth, true) > 0) {
     return true;
   }
 
   return false;
 }
 
 void SVGTextFrame::ScheduleReflowSVG() {
   if (mState & NS_FRAME_IS_NONDISPLAY) {
--- a/layout/svg/nsSVGUtils.cpp
+++ b/layout/svg/nsSVGUtils.cpp
@@ -692,17 +692,18 @@ void nsSVGUtils::PaintFrameWithEffects(n
 
     // maskFrame can be nullptr even if maskUsage.shouldGenerateMaskLayer is
     // true. That happens when a user gives an unresolvable mask-id, such as
     //   mask:url()
     //   mask:url(#id-which-does-not-exist)
     // Since we only uses nsSVGUtils with SVG elements, not like mask on an
     // HTML element, we should treat an unresolvable mask as no-mask here.
     if (maskUsage.shouldGenerateMaskLayer && maskFrame) {
-      StyleMaskMode maskMode = aFrame->StyleSVGReset()->mMask.mLayers[0].mMaskMode;
+      StyleMaskMode maskMode =
+          aFrame->StyleSVGReset()->mMask.mLayers[0].mMaskMode;
       nsSVGMaskFrame::MaskParams params(&aContext, aFrame, aTransform,
                                         maskUsage.opacity, &maskTransform,
                                         maskMode, aImgParams);
       maskSurface = maskFrame->GetMaskForMaskedFrame(params);
 
       if (!maskSurface) {
         // Either entire surface is clipped out, or gfx buffer allocation
         // failure in nsSVGMaskFrame::GetMaskForMaskedFrame.
@@ -1536,18 +1537,17 @@ float nsSVGUtils::GetStrokeWidth(nsIFram
   }
 
   nsIContent* content = aFrame->GetContent();
   if (content->IsText()) {
     content = content->GetParent();
   }
 
   SVGElement* ctx = static_cast<SVGElement*>(content);
-
-  return SVGContentUtils::CoordToFloat(ctx, style->mStrokeWidth);
+  return SVGContentUtils::CoordToFloat(ctx, style->mStrokeWidth, true);
 }
 
 void nsSVGUtils::SetupStrokeGeometry(nsIFrame* aFrame, gfxContext* aContext,
                                      SVGContextPaint* aContextPaint) {
   SVGContentUtils::AutoStrokeOptions strokeOptions;
   SVGContentUtils::GetStrokeOptions(
       &strokeOptions, static_cast<SVGElement*>(aFrame->GetContent()),
       aFrame->Style(), aContextPaint);
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -458,78 +458,57 @@ def set_gecko_property(ffi_name, expr):
     }
 </%def>
 
 <%def name="impl_svg_length(ident, gecko_ffi_name)">
     // When context-value is used on an SVG length, the corresponding flag is
     // set on mContextFlags, and the length field is set to the initial value.
 
     pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
-        use crate::values::generics::svg::{SVGLength, SvgLengthPercentageOrNumber};
+        use crate::values::generics::svg::SVGLength;
         use crate::gecko_bindings::structs::nsStyleSVG_${ident.upper()}_CONTEXT as CONTEXT_VALUE;
         let length = match v {
-            SVGLength::Length(length) => {
+            SVGLength::LengthPercentage(length) => {
                 self.gecko.mContextFlags &= !CONTEXT_VALUE;
                 length
             }
             SVGLength::ContextValue => {
                 self.gecko.mContextFlags |= CONTEXT_VALUE;
                 match longhands::${ident}::get_initial_value() {
-                    SVGLength::Length(length) => length,
+                    SVGLength::LengthPercentage(length) => length,
                     _ => unreachable!("Initial value should not be context-value"),
                 }
             }
         };
-        match length {
-            SvgLengthPercentageOrNumber::LengthPercentage(lp) =>
-                self.gecko.${gecko_ffi_name}.set(lp),
-            SvgLengthPercentageOrNumber::Number(num) =>
-                self.gecko.${gecko_ffi_name}.set_value(CoordDataValue::Factor(num.into())),
-        }
+        self.gecko.${gecko_ffi_name}.set(length)
     }
 
     pub fn copy_${ident}_from(&mut self, other: &Self) {
         use crate::gecko_bindings::structs::nsStyleSVG_${ident.upper()}_CONTEXT as CONTEXT_VALUE;
         self.gecko.${gecko_ffi_name}.copy_from(&other.gecko.${gecko_ffi_name});
         self.gecko.mContextFlags =
             (self.gecko.mContextFlags & !CONTEXT_VALUE) |
             (other.gecko.mContextFlags & CONTEXT_VALUE);
     }
 
     pub fn reset_${ident}(&mut self, other: &Self) {
         self.copy_${ident}_from(other)
     }
 
     pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
-        use crate::values::generics::svg::{SVGLength, SvgLengthPercentageOrNumber};
+        use crate::values::generics::svg::SVGLength;
         use crate::values::computed::LengthPercentage;
         use crate::gecko_bindings::structs::nsStyleSVG_${ident.upper()}_CONTEXT as CONTEXT_VALUE;
         if (self.gecko.mContextFlags & CONTEXT_VALUE) != 0 {
             return SVGLength::ContextValue;
         }
-        let length = match self.gecko.${gecko_ffi_name}.as_value() {
-            CoordDataValue::Factor(number) => {
-                SvgLengthPercentageOrNumber::Number(number)
-            },
-            CoordDataValue::Coord(coord) => {
-                SvgLengthPercentageOrNumber::LengthPercentage(
-                    LengthPercentage::new(Au(coord).into(), None)
-                )
-            },
-            CoordDataValue::Percent(p) => {
-                SvgLengthPercentageOrNumber::LengthPercentage(
-                    LengthPercentage::new(Au(0).into(), Some(Percentage(p)))
-                )
-            },
-            CoordDataValue::Calc(calc) => {
-                SvgLengthPercentageOrNumber::LengthPercentage(calc.into())
-            },
-            _ => unreachable!("Unexpected coordinate in ${ident}"),
-        };
-        SVGLength::Length(length.into())
+        // TODO(emilio): Use the Rust representation instead.
+        let length =
+            LengthPercentage::from_gecko_style_coord(&self.gecko.${gecko_ffi_name}).unwrap();
+        SVGLength::LengthPercentage(length.into())
     }
 </%def>
 
 <%def name="impl_svg_opacity(ident, gecko_ffi_name)">
     <% source_prefix = ident.split("_")[0].upper() + "_OPACITY_SOURCE" %>
 
     pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
         use crate::gecko_bindings::structs::nsStyleSVG_${source_prefix}_MASK as MASK;
@@ -4713,32 +4692,27 @@ clip-path
 
     pub fn clone_paint_order(&self) -> longhands::paint_order::computed_value::T {
         use crate::properties::longhands::paint_order::computed_value::T;
         T(self.gecko.mPaintOrder)
     }
 
     pub fn set_stroke_dasharray(&mut self, v: longhands::stroke_dasharray::computed_value::T) {
         use crate::gecko_bindings::structs::nsStyleSVG_STROKE_DASHARRAY_CONTEXT as CONTEXT_VALUE;
-        use crate::values::generics::svg::{SVGStrokeDashArray, SvgLengthPercentageOrNumber};
+        use crate::values::generics::svg::SVGStrokeDashArray;
 
         match v {
             SVGStrokeDashArray::Values(v) => {
                 let v = v.into_iter();
                 self.gecko.mContextFlags &= !CONTEXT_VALUE;
                 unsafe {
                     bindings::Gecko_nsStyleSVG_SetDashArrayLength(&mut self.gecko, v.len() as u32);
                 }
                 for (gecko, servo) in self.gecko.mStrokeDasharray.iter_mut().zip(v) {
-                    match servo {
-                        SvgLengthPercentageOrNumber::LengthPercentage(lp) =>
-                            gecko.set(lp),
-                        SvgLengthPercentageOrNumber::Number(num) =>
-                            gecko.set_value(CoordDataValue::Factor(num.into())),
-                    }
+                    gecko.set(servo)
                 }
             }
             SVGStrokeDashArray::ContextValue => {
                 self.gecko.mContextFlags |= CONTEXT_VALUE;
                 unsafe {
                     bindings::Gecko_nsStyleSVG_SetDashArrayLength(&mut self.gecko, 0);
                 }
             }
@@ -4756,40 +4730,27 @@ clip-path
     }
 
     pub fn reset_stroke_dasharray(&mut self, other: &Self) {
         self.copy_stroke_dasharray_from(other)
     }
 
     pub fn clone_stroke_dasharray(&self) -> longhands::stroke_dasharray::computed_value::T {
         use crate::gecko_bindings::structs::nsStyleSVG_STROKE_DASHARRAY_CONTEXT as CONTEXT_VALUE;
-        use crate::values::computed::LengthPercentage;
-        use crate::values::generics::NonNegative;
-        use crate::values::generics::svg::{SVGStrokeDashArray, SvgLengthPercentageOrNumber};
+        use crate::values::computed::NonNegativeLengthPercentage;
+        use crate::values::generics::svg::SVGStrokeDashArray;
 
         if self.gecko.mContextFlags & CONTEXT_VALUE != 0 {
             debug_assert_eq!(self.gecko.mStrokeDasharray.len(), 0);
             return SVGStrokeDashArray::ContextValue;
         }
+        // TODO(emilio): Use the rust representation instead.
         let mut vec = vec![];
         for gecko in self.gecko.mStrokeDasharray.iter() {
-            match gecko.as_value() {
-                CoordDataValue::Factor(number) =>
-                    vec.push(SvgLengthPercentageOrNumber::Number(number.into())),
-                CoordDataValue::Coord(coord) =>
-                    vec.push(SvgLengthPercentageOrNumber::LengthPercentage(
-                        NonNegative(LengthPercentage::new(Au(coord).into(), None).into()))),
-                CoordDataValue::Percent(p) =>
-                    vec.push(SvgLengthPercentageOrNumber::LengthPercentage(
-                        NonNegative(LengthPercentage::new_percent(Percentage(p)).into()))),
-                CoordDataValue::Calc(calc) =>
-                    vec.push(SvgLengthPercentageOrNumber::LengthPercentage(
-                        NonNegative(LengthPercentage::from(calc).clamp_to_non_negative()))),
-                _ => unreachable!(),
-            }
+            vec.push(NonNegativeLengthPercentage::from_gecko_style_coord(gecko).unwrap());
         }
         SVGStrokeDashArray::Values(vec)
     }
 
     #[allow(non_snake_case)]
     pub fn _moz_context_properties_count(&self) -> usize {
         self.gecko.mContextProps.len()
     }
--- a/servo/components/style/values/animated/mod.rs
+++ b/servo/components/style/values/animated/mod.rs
@@ -11,17 +11,17 @@
 use crate::properties::PropertyId;
 use crate::values::computed::length::LengthPercentage;
 use crate::values::computed::url::ComputedUrl;
 use crate::values::computed::Angle as ComputedAngle;
 use crate::values::computed::Image;
 use crate::values::specified::SVGPathData;
 use crate::values::CSSFloat;
 use app_units::Au;
-use euclid::{Point2D, Size2D};
+use euclid::Point2D;
 use smallvec::SmallVec;
 use std::cmp;
 
 pub mod color;
 pub mod effects;
 mod font;
 mod grid;
 mod length;
@@ -236,29 +236,16 @@ where
 
 impl Animate for Au {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
         Ok(Au::new(self.0.animate(&other.0, procedure)?))
     }
 }
 
-impl<T> Animate for Size2D<T>
-where
-    T: Animate,
-{
-    #[inline]
-    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
-        Ok(Size2D::new(
-            self.width.animate(&other.width, procedure)?,
-            self.height.animate(&other.height, procedure)?,
-        ))
-    }
-}
-
 impl<T> Animate for Point2D<T>
 where
     T: Animate,
 {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
         Ok(Point2D::new(
             self.x.animate(&other.x, procedure)?,
@@ -392,26 +379,23 @@ where
     fn to_animated_zero(&self) -> Result<Self, ()> {
         match *self {
             Some(ref value) => Ok(Some(value.to_animated_zero()?)),
             None => Ok(None),
         }
     }
 }
 
-impl<T> ToAnimatedZero for Size2D<T>
+impl<T> ToAnimatedZero for Vec<T>
 where
     T: ToAnimatedZero,
 {
     #[inline]
     fn to_animated_zero(&self) -> Result<Self, ()> {
-        Ok(Size2D::new(
-            self.width.to_animated_zero()?,
-            self.height.to_animated_zero()?,
-        ))
+        self.iter().map(|v| v.to_animated_zero()).collect()
     }
 }
 
 impl<T> ToAnimatedZero for Box<[T]>
 where
     T: ToAnimatedZero,
 {
     #[inline]
--- a/servo/components/style/values/animated/svg.rs
+++ b/servo/components/style/values/animated/svg.rs
@@ -3,95 +3,32 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 //! Animation implementations for various SVG-related types.
 
 use super::{Animate, Procedure, ToAnimatedZero};
 use crate::properties::animated_properties::ListAnimation;
 use crate::values::animated::color::Color as AnimatedColor;
 use crate::values::computed::url::ComputedUrl;
-use crate::values::computed::{LengthPercentage, Number, NumberOrPercentage};
-use crate::values::distance::{ComputeSquaredDistance, SquaredDistance};
-use crate::values::generics::svg::{SVGLength, SVGPaint, SvgLengthPercentageOrNumber};
-use crate::values::generics::svg::{SVGOpacity, SVGStrokeDashArray};
+use crate::values::distance::{SquaredDistance, ComputeSquaredDistance};
+use crate::values::generics::svg::{SVGPaint, SVGStrokeDashArray};
 
 /// Animated SVGPaint.
 pub type IntermediateSVGPaint = SVGPaint<AnimatedColor, ComputedUrl>;
 
 impl ToAnimatedZero for IntermediateSVGPaint {
     #[inline]
     fn to_animated_zero(&self) -> Result<Self, ()> {
         Ok(IntermediateSVGPaint {
             kind: self.kind.to_animated_zero()?,
             fallback: self.fallback.and_then(|v| v.to_animated_zero().ok()),
         })
     }
 }
 
-// FIXME: We need to handle calc here properly, see
-// https://bugzilla.mozilla.org/show_bug.cgi?id=1386967
-fn to_number_or_percentage(
-    value: &SvgLengthPercentageOrNumber<LengthPercentage, Number>,
-) -> Result<NumberOrPercentage, ()> {
-    Ok(match *value {
-        SvgLengthPercentageOrNumber::LengthPercentage(ref l) => match l.specified_percentage() {
-            Some(p) => {
-                if l.unclamped_length().px() != 0. {
-                    return Err(());
-                }
-                NumberOrPercentage::Percentage(p)
-            },
-            None => NumberOrPercentage::Number(l.length().px()),
-        },
-        SvgLengthPercentageOrNumber::Number(ref n) => NumberOrPercentage::Number(*n),
-    })
-}
-
-impl Animate for SvgLengthPercentageOrNumber<LengthPercentage, Number> {
-    #[inline]
-    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
-        let this = to_number_or_percentage(self)?;
-        let other = to_number_or_percentage(other)?;
-
-        match (this, other) {
-            (NumberOrPercentage::Number(ref this), NumberOrPercentage::Number(ref other)) => Ok(
-                SvgLengthPercentageOrNumber::Number(this.animate(other, procedure)?),
-            ),
-            (
-                NumberOrPercentage::Percentage(ref this),
-                NumberOrPercentage::Percentage(ref other),
-            ) => Ok(SvgLengthPercentageOrNumber::LengthPercentage(
-                LengthPercentage::new_percent(this.animate(other, procedure)?),
-            )),
-            _ => Err(()),
-        }
-    }
-}
-
-impl ComputeSquaredDistance for SvgLengthPercentageOrNumber<LengthPercentage, Number> {
-    fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
-        to_number_or_percentage(self)?.compute_squared_distance(&to_number_or_percentage(other)?)
-    }
-}
-
-impl<L> Animate for SVGLength<L>
-where
-    L: Animate + Clone,
-{
-    #[inline]
-    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
-        match (self, other) {
-            (&SVGLength::Length(ref this), &SVGLength::Length(ref other)) => {
-                Ok(SVGLength::Length(this.animate(other, procedure)?))
-            },
-            _ => Err(()),
-        }
-    }
-}
-
 /// <https://www.w3.org/TR/SVG11/painting.html#StrokeDasharrayProperty>
 impl<L> Animate for SVGStrokeDashArray<L>
 where
     L: Clone + Animate,
 {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
         if matches!(procedure, Procedure::Add | Procedure::Accumulate { .. }) {
@@ -116,41 +53,8 @@ where
         match (self, other) {
             (&SVGStrokeDashArray::Values(ref this), &SVGStrokeDashArray::Values(ref other)) => {
                 this.squared_distance_repeatable_list(other)
             },
             _ => Err(()),
         }
     }
 }
-
-impl<L> ToAnimatedZero for SVGStrokeDashArray<L>
-where
-    L: ToAnimatedZero,
-{
-    #[inline]
-    fn to_animated_zero(&self) -> Result<Self, ()> {
-        match *self {
-            SVGStrokeDashArray::Values(ref values) => Ok(SVGStrokeDashArray::Values(
-                values
-                    .iter()
-                    .map(ToAnimatedZero::to_animated_zero)
-                    .collect::<Result<Vec<_>, _>>()?,
-            )),
-            SVGStrokeDashArray::ContextValue => Ok(SVGStrokeDashArray::ContextValue),
-        }
-    }
-}
-
-impl<O> Animate for SVGOpacity<O>
-where
-    O: Animate + Clone,
-{
-    #[inline]
-    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
-        match (self, other) {
-            (&SVGOpacity::Opacity(ref this), &SVGOpacity::Opacity(ref other)) => {
-                Ok(SVGOpacity::Opacity(this.animate(other, procedure)?))
-            },
-            _ => Err(()),
-        }
-    }
-}
--- a/servo/components/style/values/computed/svg.rs
+++ b/servo/components/style/values/computed/svg.rs
@@ -1,18 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 //! Computed types for SVG properties.
 
 use crate::values::computed::color::Color;
 use crate::values::computed::url::ComputedUrl;
-use crate::values::computed::{LengthPercentage, NonNegativeLengthPercentage};
-use crate::values::computed::{NonNegativeNumber, Number, Opacity};
+use crate::values::computed::{LengthPercentage, NonNegativeLengthPercentage, Opacity};
 use crate::values::generics::svg as generic;
 use crate::values::RGBA;
 use crate::Zero;
 
 pub use crate::values::specified::SVGPaintOrder;
 
 pub use crate::values::specified::MozContextProperties;
 
@@ -36,68 +35,41 @@ impl SVGPaint {
         let rgba = RGBA::from_floats(0., 0., 0., 1.).into();
         SVGPaint {
             kind: generic::SVGPaintKind::Color(rgba),
             fallback: None,
         }
     }
 }
 
-/// A value of <length> | <percentage> | <number> for stroke-dashoffset.
-/// <https://www.w3.org/TR/SVG11/painting.html#StrokeProperties>
-pub type SvgLengthPercentageOrNumber =
-    generic::SvgLengthPercentageOrNumber<LengthPercentage, Number>;
-
 /// <length> | <percentage> | <number> | context-value
-pub type SVGLength = generic::SVGLength<SvgLengthPercentageOrNumber>;
+pub type SVGLength = generic::SVGLength<LengthPercentage>;
 
 impl SVGLength {
     /// `0px`
     pub fn zero() -> Self {
-        generic::SVGLength::Length(generic::SvgLengthPercentageOrNumber::LengthPercentage(
-            LengthPercentage::zero(),
-        ))
-    }
-}
-
-/// A value of <length> | <percentage> | <number> for stroke-width/stroke-dasharray.
-/// <https://www.w3.org/TR/SVG11/painting.html#StrokeProperties>
-pub type NonNegativeSvgLengthPercentageOrNumber =
-    generic::SvgLengthPercentageOrNumber<NonNegativeLengthPercentage, NonNegativeNumber>;
-
-// FIXME(emilio): This is really hacky, and can go away with a bit of work on
-// the clone_stroke_width code in gecko.mako.rs.
-impl Into<NonNegativeSvgLengthPercentageOrNumber> for SvgLengthPercentageOrNumber {
-    fn into(self) -> NonNegativeSvgLengthPercentageOrNumber {
-        match self {
-            generic::SvgLengthPercentageOrNumber::LengthPercentage(lop) => {
-                generic::SvgLengthPercentageOrNumber::LengthPercentage(lop.into())
-            },
-            generic::SvgLengthPercentageOrNumber::Number(num) => {
-                generic::SvgLengthPercentageOrNumber::Number(num.into())
-            },
-        }
+        generic::SVGLength::LengthPercentage(LengthPercentage::zero())
     }
 }
 
 /// An non-negative wrapper of SVGLength.
-pub type SVGWidth = generic::SVGLength<NonNegativeSvgLengthPercentageOrNumber>;
+pub type SVGWidth = generic::SVGLength<NonNegativeLengthPercentage>;
 
 impl SVGWidth {
     /// `1px`.
     pub fn one() -> Self {
         use crate::values::generics::NonNegative;
-        generic::SVGLength::Length(generic::SvgLengthPercentageOrNumber::LengthPercentage(
+        generic::SVGLength::LengthPercentage(
             NonNegative(LengthPercentage::one()),
-        ))
+        )
     }
 }
 
 /// [ <length> | <percentage> | <number> ]# | context-value
-pub type SVGStrokeDashArray = generic::SVGStrokeDashArray<NonNegativeSvgLengthPercentageOrNumber>;
+pub type SVGStrokeDashArray = generic::SVGStrokeDashArray<NonNegativeLengthPercentage>;
 
 impl Default for SVGStrokeDashArray {
     fn default() -> Self {
         generic::SVGStrokeDashArray::Values(vec![])
     }
 }
 
 /// <opacity-value> | context-fill-opacity | context-stroke-opacity
--- a/servo/components/style/values/generics/svg.rs
+++ b/servo/components/style/values/generics/svg.rs
@@ -123,111 +123,77 @@ impl<ColorType: Parse, UrlPaintServer: P
                 fallback: None,
             })
         } else {
             Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
         }
     }
 }
 
-/// A value of <length> | <percentage> | <number> for svg which allow unitless length.
-/// <https://www.w3.org/TR/SVG11/painting.html#StrokeProperties>
+/// An SVG length value supports `context-value` in addition to length.
 #[derive(
+    Animate,
     Clone,
+    ComputeSquaredDistance,
     Copy,
     Debug,
     MallocSizeOf,
     PartialEq,
-    Parse,
     SpecifiedValueInfo,
     ToAnimatedValue,
     ToAnimatedZero,
     ToComputedValue,
     ToCss,
 )]
-pub enum SvgLengthPercentageOrNumber<LengthPercentage, Number> {
-    /// <number>
-    ///
-    /// Note that this needs to be before, so it gets parsed before the length,
-    /// to handle `0` correctly as a number instead of a `<length>`.
-    Number(Number),
-    /// <length> | <percentage>
-    LengthPercentage(LengthPercentage),
-}
-
-/// Whether the `context-value` value is enabled.
-#[cfg(feature = "gecko")]
-pub fn is_context_value_enabled(_: &ParserContext) -> bool {
-    use crate::gecko_bindings::structs::mozilla;
-    unsafe { mozilla::StaticPrefs_sVarCache_gfx_font_rendering_opentype_svg_enabled }
-}
-
-/// Whether the `context-value` value is enabled.
-#[cfg(not(feature = "gecko"))]
-pub fn is_context_value_enabled(_: &ParserContext) -> bool {
-    false
-}
-
-/// An SVG length value supports `context-value` in addition to length.
-#[derive(
-    Clone,
-    ComputeSquaredDistance,
-    Copy,
-    Debug,
-    MallocSizeOf,
-    PartialEq,
-    Parse,
-    SpecifiedValueInfo,
-    ToAnimatedValue,
-    ToAnimatedZero,
-    ToComputedValue,
-    ToCss,
-)]
-pub enum SVGLength<LengthType> {
+pub enum SVGLength<L> {
     /// `<length> | <percentage> | <number>`
-    Length(LengthType),
+    LengthPercentage(L),
     /// `context-value`
-    #[parse(condition = "is_context_value_enabled")]
+    #[animation(error)]
     ContextValue,
 }
 
 /// Generic value for stroke-dasharray.
 #[derive(
     Clone,
     Debug,
     MallocSizeOf,
     PartialEq,
     SpecifiedValueInfo,
     ToAnimatedValue,
+    ToAnimatedZero,
     ToComputedValue,
     ToCss,
 )]
-pub enum SVGStrokeDashArray<LengthType> {
+pub enum SVGStrokeDashArray<L> {
     /// `[ <length> | <percentage> | <number> ]#`
     #[css(comma)]
-    Values(#[css(if_empty = "none", iterable)] Vec<LengthType>),
+    Values(#[css(if_empty = "none", iterable)] Vec<L>),
     /// `context-value`
     ContextValue,
 }
 
 /// An SVG opacity value accepts `context-{fill,stroke}-opacity` in
 /// addition to opacity value.
 #[derive(
+    Animate,
     Clone,
     ComputeSquaredDistance,
     Copy,
     Debug,
     MallocSizeOf,
     PartialEq,
     Parse,
     SpecifiedValueInfo,
     ToAnimatedZero,
     ToComputedValue,
     ToCss,
 )]
 pub enum SVGOpacity<OpacityType> {
     /// `<opacity-value>`
     Opacity(OpacityType),
     /// `context-fill-opacity`
+    #[animation(error)]
     ContextFillOpacity,
     /// `context-stroke-opacity`
+    #[animation(error)]
     ContextStrokeOpacity,
 }
--- a/servo/components/style/values/specified/mod.rs
+++ b/servo/components/style/values/specified/mod.rs
@@ -777,26 +777,32 @@ impl ClipRectOrAuto {
             Auto::parse(context, input).map(Either::Second)
         }
     }
 }
 
 /// Whether quirks are allowed in this context.
 #[derive(Clone, Copy, PartialEq)]
 pub enum AllowQuirks {
-    /// Quirks are allowed.
-    Yes,
     /// Quirks are not allowed.
     No,
+    /// Quirks are allowed, in quirks mode.
+    Yes,
+    /// Quirks are always allowed, used for SVG lengths.
+    Always,
 }
 
 impl AllowQuirks {
     /// Returns `true` if quirks are allowed in this context.
     pub fn allowed(self, quirks_mode: QuirksMode) -> bool {
-        self == AllowQuirks::Yes && quirks_mode == QuirksMode::Quirks
+        match self {
+            AllowQuirks::Always => true,
+            AllowQuirks::No => false,
+            AllowQuirks::Yes => quirks_mode == QuirksMode::Quirks,
+        }
     }
 }
 
 /// An attr(...) rule
 ///
 /// `[namespace? `|`]? ident`
 #[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToComputedValue)]
 #[css(function)]
--- a/servo/components/style/values/specified/svg.rs
+++ b/servo/components/style/values/specified/svg.rs
@@ -1,79 +1,100 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 //! Specified types for SVG properties.
 
 use crate::parser::{Parse, ParserContext};
 use crate::values::generics::svg as generic;
+use crate::values::specified::AllowQuirks;
 use crate::values::specified::color::Color;
 use crate::values::specified::url::SpecifiedUrl;
 use crate::values::specified::LengthPercentage;
-use crate::values::specified::{NonNegativeLengthPercentage, NonNegativeNumber};
-use crate::values::specified::{Number, Opacity};
+use crate::values::specified::{NonNegativeLengthPercentage, Opacity};
 use crate::values::CustomIdent;
 use cssparser::Parser;
 use std::fmt::{self, Write};
 use style_traits::{CommaWithSpace, CssWriter, ParseError, Separator};
 use style_traits::{StyleParseErrorKind, ToCss};
 
 /// Specified SVG Paint value
 pub type SVGPaint = generic::SVGPaint<Color, SpecifiedUrl>;
 
 /// Specified SVG Paint Kind value
 pub type SVGPaintKind = generic::SVGPaintKind<Color, SpecifiedUrl>;
 
-/// A value of <length> | <percentage> | <number> for stroke-dashoffset.
-/// <https://www.w3.org/TR/SVG11/painting.html#StrokeProperties>
-pub type SvgLengthPercentageOrNumber =
-    generic::SvgLengthPercentageOrNumber<LengthPercentage, Number>;
+/// <length> | <percentage> | <number> | context-value
+pub type SVGLength = generic::SVGLength<LengthPercentage>;
+
+/// A non-negative version of SVGLength.
+pub type SVGWidth = generic::SVGLength<NonNegativeLengthPercentage>;
+
+/// [ <length> | <percentage> | <number> ]# | context-value
+pub type SVGStrokeDashArray = generic::SVGStrokeDashArray<NonNegativeLengthPercentage>;
+
+/// Whether the `context-value` value is enabled.
+#[cfg(feature = "gecko")]
+pub fn is_context_value_enabled() -> bool {
+    use crate::gecko_bindings::structs::mozilla;
+    unsafe { mozilla::StaticPrefs_sVarCache_gfx_font_rendering_opentype_svg_enabled }
+}
 
-/// <length> | <percentage> | <number> | context-value
-pub type SVGLength = generic::SVGLength<SvgLengthPercentageOrNumber>;
+/// Whether the `context-value` value is enabled.
+#[cfg(not(feature = "gecko"))]
+pub fn is_context_value_enabled() -> bool {
+    false
+}
 
-impl From<SvgLengthPercentageOrNumber> for SVGLength {
-    fn from(length: SvgLengthPercentageOrNumber) -> Self {
-        generic::SVGLength::Length(length)
+macro_rules! parse_svg_length {
+    ($ty:ty, $lp:ty) => {
+        impl Parse for $ty {
+            fn parse<'i, 't>(
+                context: &ParserContext,
+                input: &mut Parser<'i, 't>,
+            ) -> Result<Self, ParseError<'i>> {
+                if let Ok(lp) = input.try(|i| {
+                    <$lp>::parse_quirky(context, i, AllowQuirks::Always)
+                }) {
+                    return Ok(generic::SVGLength::LengthPercentage(lp))
+                }
+
+                try_match_ident_ignore_ascii_case! { input,
+                    "context-value" if is_context_value_enabled() => {
+                        Ok(generic::SVGLength::ContextValue)
+                    },
+                }
+            }
+        }
     }
 }
 
-/// A value of <length> | <percentage> | <number> for stroke-width/stroke-dasharray.
-/// <https://www.w3.org/TR/SVG11/painting.html#StrokeProperties>
-pub type NonNegativeSvgLengthPercentageOrNumber =
-    generic::SvgLengthPercentageOrNumber<NonNegativeLengthPercentage, NonNegativeNumber>;
-
-/// A non-negative version of SVGLength.
-pub type SVGWidth = generic::SVGLength<NonNegativeSvgLengthPercentageOrNumber>;
-
-impl From<NonNegativeSvgLengthPercentageOrNumber> for SVGWidth {
-    fn from(length: NonNegativeSvgLengthPercentageOrNumber) -> Self {
-        generic::SVGLength::Length(length)
-    }
-}
-
-/// [ <length> | <percentage> | <number> ]# | context-value
-pub type SVGStrokeDashArray = generic::SVGStrokeDashArray<NonNegativeSvgLengthPercentageOrNumber>;
+parse_svg_length!(SVGLength, LengthPercentage);
+parse_svg_length!(SVGWidth, NonNegativeLengthPercentage);
 
 impl Parse for SVGStrokeDashArray {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         if let Ok(values) = input.try(|i| {
             CommaWithSpace::parse(i, |i| {
-                NonNegativeSvgLengthPercentageOrNumber::parse(context, i)
+                NonNegativeLengthPercentage::parse_quirky(
+                    context,
+                    i,
+                    AllowQuirks::Always,
+                )
             })
         }) {
             return Ok(generic::SVGStrokeDashArray::Values(values));
         }
 
         try_match_ident_ignore_ascii_case! { input,
-            "context-value" if generic::is_context_value_enabled(context) => {
+            "context-value" if is_context_value_enabled() => {
                 Ok(generic::SVGStrokeDashArray::ContextValue)
             },
             "none" => Ok(generic::SVGStrokeDashArray::Values(vec![])),
         }
     }
 }
 
 /// <opacity-value> | context-fill-opacity | context-stroke-opacity