Bug 1903546 - fix non-scaling-stroke r=emilio. a=dmeehan

This implements w3c/svgwg#582 and therefore mostly reverts bug 1904891

Differential Revision: https://phabricator.services.mozilla.com/D216303

dom/svg/SVGContentUtils.cpp

@@ -489,7 +489,9 @@ SVGViewportElement* SVGContentUtils::GetNearestViewportElement(
   return nullptr;
 }
 
-static gfx::Matrix GetCTMInternal(SVGElement* aElement, bool aScreenCTM,
+enum class CTMType { NearestViewport, OuterViewport, Screen };
+
+static gfx::Matrix GetCTMInternal(SVGElement* aElement, CTMType aCTMType,
                                   bool aHaveRecursed) {
   auto getLocalTransformHelper =
       [](SVGElement const* e, bool shouldIncludeChildToUserSpace) -> gfxMatrix {
@@ -530,7 +532,8 @@ static gfx::Matrix GetCTMInternal(SVGElement* aElement, bool aScreenCTM,
          !ancestor->IsSVGElement(nsGkAtoms::foreignObject)) {
     element = static_cast<SVGElement*>(ancestor);
     matrix *= getLocalTransformHelper(element, true);
-    if (!aScreenCTM && SVGContentUtils::EstablishesViewport(element)) {
+    if (aCTMType == CTMType::NearestViewport &&
+        SVGContentUtils::EstablishesViewport(element)) {
       if (!element->IsAnyOfSVGElements(nsGkAtoms::svg, nsGkAtoms::symbol)) {
         NS_ERROR("New (SVG > 1.1) SVG viewport establishing element?");
         return gfx::Matrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0);  // singular
@@ -540,7 +543,7 @@ static gfx::Matrix GetCTMInternal(SVGElement* aElement, bool aScreenCTM,
     }
     ancestor = ancestor->GetFlattenedTreeParent();
   }
-  if (!aScreenCTM) {
+  if (aCTMType == CTMType::NearestViewport) {
     // didn't find a nearestViewportElement
     return gfx::Matrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0);  // singular
   }
@@ -568,13 +571,16 @@ static gfx::Matrix GetCTMInternal(SVGElement* aElement, bool aScreenCTM,
     int32_t appUnitsPerCSSPixel = AppUnitsPerCSSPixel();
     tm.PostTranslate(NSAppUnitsToFloatPixels(bp.left, appUnitsPerCSSPixel),
                      NSAppUnitsToFloatPixels(bp.top, appUnitsPerCSSPixel));
+    if (aCTMType == CTMType::OuterViewport) {
+      return tm;
+    }
   }
 
   if (!ancestor || !ancestor->IsElement()) {
     return tm;
   }
   if (auto* ancestorSVG = SVGElement::FromNode(ancestor)) {
-    return tm * GetCTMInternal(ancestorSVG, true, true);
+    return tm * GetCTMInternal(ancestorSVG, aCTMType, true);
   }
   nsIFrame* parentFrame = frame->GetParent();
   if (!parentFrame) {
@@ -612,16 +618,20 @@ static gfx::Matrix GetCTMInternal(SVGElement* aElement, bool aScreenCTM,
   }
   return nearestSVGAncestor
              ? tm * GetCTMInternal(static_cast<SVGElement*>(nearestSVGAncestor),
-                                   true, true)
+                                   aCTMType, true)
              : tm;
 }
 
 gfx::Matrix SVGContentUtils::GetCTM(SVGElement* aElement) {
-  return GetCTMInternal(aElement, false, false);
+  return GetCTMInternal(aElement, CTMType::NearestViewport, false);
+}
+
+gfx::Matrix SVGContentUtils::GetOuterViewportCTM(SVGElement* aElement) {
+  return GetCTMInternal(aElement, CTMType::OuterViewport, false);
 }
 
 gfx::Matrix SVGContentUtils::GetScreenCTM(SVGElement* aElement) {
-  return GetCTMInternal(aElement, true, false);
+  return GetCTMInternal(aElement, CTMType::Screen, false);
 }
 
 void SVGContentUtils::RectilinearGetStrokeBounds(

dom/svg/SVGContentUtils.h

@@ -197,6 +197,8 @@ class SVGContentUtils {
 
   static Matrix GetCTM(dom::SVGElement* aElement);
 
+  static Matrix GetOuterViewportCTM(dom::SVGElement* aElement);
+
   static Matrix GetScreenCTM(dom::SVGElement* aElement);
 
   /**

dom/svg/SVGGeometryElement.cpp

@@ -201,9 +201,7 @@ bool SVGGeometryElement::IsPointInStroke(const DOMPointInit& aPoint) {
   SVGGeometryProperty::DoForComputedStyle(this, [&](const ComputedStyle* s) {
     // Per spec, we should take vector-effect into account.
     if (s->StyleSVGReset()->HasNonScalingStroke()) {
-      auto mat = s->StyleSVGReset()->mVectorEffect.IsScreen()
-                     ? SVGContentUtils::GetScreenCTM(this)
-                     : SVGContentUtils::GetCTM(this);
+      auto mat = SVGContentUtils::GetOuterViewportCTM(this);
       if (mat.HasNonTranslation()) {
         // We have non-scaling-stroke as well as a non-translation transform.
         // We should transform the path first then apply the stroke on the

layout/style/ServoBindings.toml

@@ -587,8 +587,6 @@ cbindgen-types = [
     { gecko = "StylePageOrientation", servo = "crate::values::generics::page::PageOrientation" },
     { gecko = "StylePageSize", servo = "crate::values::computed::page::PageSize" },
     { gecko = "StyleDProperty", servo = "crate::values::specified::svg::DProperty" },
-    { gecko = "StyleVectorEffectType", servo = "crate::values::specified::svg::VectorEffectType" },
-    { gecko = "StyleCoordinateSpace", servo = "crate::values::specified::svg::CoordinateSpace" },
     { gecko = "StyleVectorEffect", servo = "crate::values::specified::svg::VectorEffect" },
     { gecko = "StyleImageRendering", servo = "crate::values::computed::ImageRendering" },
     { gecko = "StylePrintColorAdjust", servo = "crate::values::computed::PrintColorAdjust" },

layout/style/nsStyleStruct.cpp

@@ -909,6 +909,7 @@ nsStyleSVGReset::nsStyleSVGReset()
       mLightingColor(StyleColor::White()),
       mStopOpacity(1.0f),
       mFloodOpacity(1.0f),
+      mVectorEffect(StyleVectorEffect::NONE),
       mMaskType(StyleMaskType::Luminance),
       mD(StyleDProperty::None()) {
   MOZ_COUNT_CTOR(nsStyleSVGReset);

layout/style/test/property_database.js

@@ -9787,18 +9787,8 @@ var gCSSProperties = {
     applies_to_first_letter: true,
     applies_to_first_line: true,
     initial_values: ["none"],
-    other_values: [
-      "non-scaling-stroke",
-      "non-scaling-stroke viewport",
-      "non-scaling-stroke screen",
-    ],
-    invalid_values: [
-      "none non-scaling-stroke",
-      "non-scaling-stroke viewport screen",
-      "none viewport",
-      "none screen",
-      "viewport non-scaling-stroke",
-    ],
+    other_values: ["non-scaling-stroke"],
+    invalid_values: ["none non-scaling-stroke"],
   },
   "-moz-window-dragging": {
     domProp: "MozWindowDragging",

layout/svg/SVGUtils.cpp

@@ -1083,9 +1083,7 @@ bool SVGUtils::GetNonScalingStrokeTransform(const nsIFrame* aFrame,
 
   SVGElement* content = static_cast<SVGElement*>(aFrame->GetContent());
   *aUserToOuterSVG =
-      ThebesMatrix(aFrame->StyleSVGReset()->mVectorEffect.IsScreen()
-                       ? SVGContentUtils::GetScreenCTM(content)
-                       : SVGContentUtils::GetCTM(content));
+      ThebesMatrix(SVGContentUtils::GetOuterViewportCTM(content));
 
   return aUserToOuterSVG->HasNonTranslation() && !aUserToOuterSVG->IsSingular();
 }

servo/components/style/values/specified/svg.rs

@@ -420,104 +420,21 @@ impl Parse for DProperty {
 )]
 #[css(bitflags(single = "none", mixed = "non-scaling-stroke"))]
 #[repr(C)]
-/// Values for vector-effect:
 /// https://svgwg.org/svg2-draft/coords.html#VectorEffects
-pub struct VectorEffectType(u8);
+pub struct VectorEffect(u8);
 bitflags! {
-    impl VectorEffectType: u8 {
+    impl VectorEffect: u8 {
         /// `none`
         const NONE = 0;
         /// `non-scaling-stroke`
         const NON_SCALING_STROKE = 1 << 0;
     }
 }
 
-#[allow(missing_docs)]
-impl VectorEffectType {
-    pub fn is_none(&self) -> bool {
-        *self == VectorEffectType::NONE
-    }
-}
-
-#[derive(
-    Clone,
-    Copy,
-    Debug,
-    Default,
-    Eq,
-    MallocSizeOf,
-    Parse,
-    PartialEq,
-    SpecifiedValueInfo,
-    ToComputedValue,
-    ToCss,
-    ToResolvedValue,
-    ToShmem,
-)]
-#[repr(C)]
-/// co-ordinate space for vector-effect:
-/// https://svgwg.org/svg2-draft/coords.html#VectorEffects
-pub enum CoordinateSpace {
-    #[default]
-    /// `viewport`
-    Viewport,
-    /// `screen`
-    Screen,
-}
-
-#[allow(missing_docs)]
-impl CoordinateSpace {
-    pub fn is_viewport(&self) -> bool {
-        *self == Self::Viewport
-    }
-}
-
-#[derive(
-    Clone,
-    Copy,
-    Debug,
-    MallocSizeOf,
-    PartialEq,
-    SpecifiedValueInfo,
-    ToCss,
-    ToComputedValue,
-    ToResolvedValue,
-    ToShmem,
-)]
-#[repr(C)]
-/// Specified value for the vector-effect property.
-/// (The spec grammar gives
-/// `none | [ non-scaling-stroke | non-scaling-size | non-rotation | fixed-position ]+ [ viewport | screen ]?`.)
-/// https://svgwg.org/svg2-draft/coords.html#VectorEffects
-pub struct VectorEffect {
-    /// none or non-scaling-stroke
-    pub effect_type: VectorEffectType,
-    /// screen or viewport
-    #[css(skip_if = "CoordinateSpace::is_viewport")]
-    pub coordinate_space: CoordinateSpace,
-}
-
-#[allow(missing_docs)]
 impl VectorEffect {
+    /// Returns the initial value of vector-effect
     #[inline]
     pub fn none() -> Self {
-        Self {
-            effect_type: VectorEffectType::NONE,
-            coordinate_space: CoordinateSpace::Viewport,
-        }
-    }
-}
-
-impl Parse for VectorEffect {
-    fn parse<'i, 't>(
-        context: &ParserContext,
-        input: &mut Parser<'i, 't>,
-    ) -> Result<Self, ParseError<'i>> {
-        let effect_type = VectorEffectType::parse(context, input)?;
-        if effect_type.is_none() {
-            return Ok(Self::none());
-        }
-        let coordinate_space = input.try_parse(CoordinateSpace::parse).unwrap_or(CoordinateSpace::Viewport);
-        Ok(Self { effect_type, coordinate_space })
+        Self::NONE
     }
 }

servo/ports/geckolib/cbindgen.toml

@@ -557,12 +557,7 @@ renaming_overrides_prefixing = true
 """
 
 "VectorEffect" = """
-  StyleVectorEffect()
-    : effect_type(StyleVectorEffectType::NONE),
-      coordinate_space(StyleCoordinateSpace::Viewport) {}
-
-  bool HasNonScalingStroke() const { return bool(effect_type & StyleVectorEffectType::NON_SCALING_STROKE); }
-  bool IsScreen() const { return coordinate_space == StyleCoordinateSpace::Screen; }
+  bool HasNonScalingStroke() const { return bool(*this & StyleVectorEffect::NON_SCALING_STROKE); }
 """
 
 # TODO(emilio): Add hooks to cbindgen to be able to generate [[nodiscard]]

testing/web-platform/tests/svg/painting/reftests/non-scaling-stroke-003.html

@@ -1,6 +1,6 @@
 <!DOCTYPE html>
 <html>
-<title>non-scaling-stroke with screen transform</title>
+<title>non-scaling-stroke with outer viewport transform</title>
 <link rel="help" href="https://svgwg.org/svg2-draft/painting.html#PaintingVectorEffects" />
 <link rel="match" href="green-100x100.svg" />
 <body>
@@ -10,24 +10,30 @@
       margin: 0;
       width: 200px;
       height: 200px;
-      transform: scale(0.5);
     }
     svg {
-      width: 100px;
-      height: 100px;
-      transform: scale(2) translate(-25px, -25px);
       transform-origin: center;
       transform-bxox: fill-box;
     }
+    #outer {
+      width: 100px;
+      height: 100px;
+      transform: scale(2);
+    }
+    #inner {
+      transform: scale(0.5);
+    }
     rect {
       fill: red;
       stroke: green;
       stroke-width: 75px;
-      vector-effect: non-scaling-stroke screen;
+      vector-effect: non-scaling-stroke;
     }
   </style>
-  <svg>
-    <rect width="75" height="75"/>
+  <svg id="outer">
+    <svg id="inner">
+      <rect width="75" height="75"/>
+    </svg>
   </svg>
 </body>
 </html>

testing/web-platform/tests/svg/styling/vector-effect-invalid.html

@@ -18,6 +18,9 @@
 test_invalid_value("vector-effect", "viewport");
 test_invalid_value("vector-effect", "screen");
 test_invalid_value("vector-effect", "screen non-scaling-stroke");
+// The following were removed by https://github.com/w3c/svgwg/issues/582
+test_invalid_value("vector-effect", "non-scaling-stroke viewport");
+test_invalid_value("vector-effect", "non-scaling-stroke screen");
 </script>
 </body>
 </html>