Bug 1609786 - Make the empty svg path valid. r=emilio

Per SVG2 spec, the EBNF allows the path data string to be empty.
An empty path data string disables rendering of the path.
Therefore, we should make path('') a valid path string.

The related spec issue: https://github.com/w3c/fxtf-drafts/issues/392.
Now we serialize `path("")` as `path("")` for offset-path and clip-path.

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

--HG--
extra : moz-landing-system : lando

layout/base/MotionPathUtils.cpp

@@ -346,7 +346,7 @@ Maybe<ResolvedMotionPathData> MotionPathUtils::ResolveMotionPath(
   if (aPath.IsPath()) {
     const auto& path = aPath.AsPath();
     if (!path.mGfxPath) {
-      NS_WARNING("could not get a valid gfx path");
+      // Empty gfx::Path means it is path('') (i.e. empty path string).
       return Nothing();
     }
 
@@ -439,11 +439,13 @@ static OffsetPathData GenerateOffsetPathData(const nsIFrame* aFrame) {
   const StyleOffsetPath& path = aFrame->StyleDisplay()->mOffsetPath;
   switch (path.tag) {
     case StyleOffsetPath::Tag::Path: {
+      const StyleSVGPathData& pathData = path.AsPath();
       RefPtr<gfx::Path> gfxPath =
           aFrame->GetProperty(nsIFrame::OffsetPathCache());
-      MOZ_ASSERT(gfxPath,
-                 "Should have a valid cached gfx::Path for offset-path");
-      return OffsetPathData::Path(path.AsPath(), gfxPath.forget());
+      MOZ_ASSERT(
+          gfxPath || pathData._0.IsEmpty(),
+          "Should have a valid cached gfx::Path or an empty path string");
+      return OffsetPathData::Path(pathData, gfxPath.forget());
     }
     case StyleOffsetPath::Tag::Ray:
       return OffsetPathData::Ray(path.AsRay(), RayReferenceData(aFrame));
@@ -494,16 +496,16 @@ static OffsetPathData GenerateOffsetPathData(
     gfx::Path* aCachedMotionPath) {
   switch (aPath.tag) {
     case StyleOffsetPath::Tag::Path: {
-      const StyleSVGPathData& svgPathData = aPath.AsPath();
+      const StyleSVGPathData& pathData = aPath.AsPath();
       // If aCachedMotionPath is valid, we have a fixed path.
       // This means we have pre-built it already and no need to update.
       RefPtr<gfx::Path> path = aCachedMotionPath;
       if (!path) {
         RefPtr<gfx::PathBuilder> builder =
             MotionPathUtils::GetCompositorPathBuilder();
-        path = MotionPathUtils::BuildPath(svgPathData, builder);
+        path = MotionPathUtils::BuildPath(pathData, builder);
       }
-      return OffsetPathData::Path(svgPathData, path.forget());
+      return OffsetPathData::Path(pathData, path.forget());
     }
     case StyleOffsetPath::Tag::Ray:
       return OffsetPathData::Ray(aPath.AsRay(), aRayReferenceData);

layout/generic/nsFrame.cpp

@@ -1353,8 +1353,16 @@ void nsFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle) {
           gfxPlatform::GetPlatform()
               ->ScreenReferenceDrawTarget()
               ->CreatePathBuilder(gfx::FillRule::FILL_WINDING);
-      SetProperty(nsIFrame::OffsetPathCache(),
-                  MotionPathUtils::BuildPath(newPath.AsPath(), builder).take());
+      RefPtr<gfx::Path> path =
+          MotionPathUtils::BuildPath(newPath.AsPath(), builder);
+      if (path) {
+        // The newPath could be path('') (i.e. empty path), so its gfx path
+        // could be nullptr, and so we only set property for a non-empty path.
+        SetProperty(nsIFrame::OffsetPathCache(), path.forget().take());
+      } else {
+        // May have an old cached path, so we have to delete it.
+        DeleteProperty(nsIFrame::OffsetPathCache());
+      }
     } else if (oldPath) {
       DeleteProperty(nsIFrame::OffsetPathCache());
     }
@@ -2890,9 +2898,12 @@ static Maybe<nsRect> ComputeClipForMaskItem(nsDisplayListBuilder* aBuilder,
 
   Maybe<gfxRect> combinedClip;
   if (maskUsage.shouldApplyBasicShapeOrPath) {
-    Rect result = nsCSSClipPathInstance::GetBoundingRectForBasicShapeOrPathClip(
-        aMaskedFrame, svgReset->mClipPath);
-    combinedClip = Some(ThebesRect(result));
+    Maybe<Rect> result =
+        nsCSSClipPathInstance::GetBoundingRectForBasicShapeOrPathClip(
+            aMaskedFrame, svgReset->mClipPath);
+    if (result) {
+      combinedClip = Some(ThebesRect(*result));
+    }
   } else if (maskUsage.shouldApplyClipPath) {
     gfxRect result = nsSVGUtils::GetBBox(
         aMaskedFrame,

layout/style/crashtests/1609786.html

@@ -0,0 +1,9 @@
+<html>
+<head>
+    <style>
+        * {
+            offset-path: path(' ') ! important;
+        }
+    </style>
+</head>
+</html>

layout/style/crashtests/crashtests.list

@@ -315,3 +315,4 @@ pref(layout.css.motion-path.enabled,true) load 1594949.html
 pref(layout.css.motion-path.enabled,true) pref(layout.css.individual-transform.enabled,true) load 1594960.html
 load 1586444.html
 load 1599286.html
+pref(layout.css.motion-path.enabled,true) load 1609786.html

layout/style/test/property_database.js

@@ -12838,6 +12838,8 @@ if (IsCSSPropertyPrefEnabled("layout.css.motion-path.enabled")) {
     type: CSS_TYPE_LONGHAND,
     initial_values: ["none"],
     other_values: [
+      "path('')",
+      "path(' ')",
       "path('M 10 10 20 20 H 90 V 90 Z')",
       "path('M10 10 20,20H90V90Z')",
       "path('M 10 10 C 20 20, 40 20, 50 10')",
@@ -12856,7 +12858,6 @@ if (IsCSSPropertyPrefEnabled("layout.css.motion-path.enabled")) {
       "ray(calc(180deg - 45deg) farthest-side)",
     ],
     invalid_values: [
-      "path('')",
       "path()",
       "path(a)",
       "path('M 10 Z')",
@@ -12907,6 +12908,7 @@ if (IsCSSPropertyPrefEnabled("layout.css.motion-path.enabled")) {
 
 if (IsCSSPropertyPrefEnabled("layout.css.clip-path-path.enabled")) {
   gCSSProperties["clip-path"].other_values.push(
+    "path(evenodd, '')",
     "path(nonzero, 'M 10 10 h 100 v 100 h-100 v-100 z')",
     "path(evenodd, 'M 10 10 h 100 v 100 h-100 v-100 z')",
     "path('M10,30A20,20 0,0,1 50,30A20,20 0,0,1 90,30Q90,60 50,90Q10,60 10,30z')"
@@ -12914,7 +12916,6 @@ if (IsCSSPropertyPrefEnabled("layout.css.clip-path-path.enabled")) {
 
   gCSSProperties["clip-path"].invalid_values.push(
     "path(nonzero)",
-    "path(evenodd, '')",
     "path(abs, 'M 10 10 L 10 10 z')"
   );
 }

layout/svg/nsCSSClipPathInstance.cpp

@@ -36,12 +36,14 @@ void nsCSSClipPathInstance::ApplyBasicShapeOrPathClip(
                  type == StyleShapeSourceType::Path,
              "This is used with basic-shape, geometry-box, and path() only");
 #endif
-
   nsCSSClipPathInstance instance(aFrame, clipPathStyle);
 
   aContext.NewPath();
   RefPtr<Path> path =
       instance.CreateClipPath(aContext.GetDrawTarget(), aTransform);
+  if (!path) {
+    return;
+  }
   aContext.SetPath(path);
   aContext.Clip();
 }
@@ -66,11 +68,11 @@ bool nsCSSClipPathInstance::HitTestBasicShapeOrPathClip(
       drawTarget, nsSVGUtils::GetCSSPxToDevPxMatrix(aFrame));
   float pixelRatio = float(AppUnitsPerCSSPixel()) /
                      aFrame->PresContext()->AppUnitsPerDevPixel();
-  return path->ContainsPoint(ToPoint(aPoint) * pixelRatio, Matrix());
+  return path && path->ContainsPoint(ToPoint(aPoint) * pixelRatio, Matrix());
 }
 
 /* static */
-Rect nsCSSClipPathInstance::GetBoundingRectForBasicShapeOrPathClip(
+Maybe<Rect> nsCSSClipPathInstance::GetBoundingRectForBasicShapeOrPathClip(
     nsIFrame* aFrame, const StyleShapeSource& aClipPathStyle) {
   MOZ_ASSERT(aClipPathStyle.GetType() == StyleShapeSourceType::Shape ||
              aClipPathStyle.GetType() == StyleShapeSourceType::Box ||
@@ -82,7 +84,7 @@ Rect nsCSSClipPathInstance::GetBoundingRectForBasicShapeOrPathClip(
       gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget();
   RefPtr<Path> path = instance.CreateClipPath(
       drawTarget, nsSVGUtils::GetCSSPxToDevPxMatrix(aFrame));
-  return path->GetBounds();
+  return path ? Some(path->GetBounds()) : Nothing();
 }
 
 already_AddRefed<Path> nsCSSClipPathInstance::CreateClipPath(

layout/svg/nsCSSClipPathInstance.h

@@ -30,7 +30,7 @@ class nsCSSClipPathInstance {
   static bool HitTestBasicShapeOrPathClip(nsIFrame* aFrame,
                                           const gfxPoint& aPoint);
 
-  static Rect GetBoundingRectForBasicShapeOrPathClip(
+  static Maybe<Rect> GetBoundingRectForBasicShapeOrPathClip(
       nsIFrame* aFrame, const StyleShapeSource& aClipPathStyle);
 
  private:

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

@@ -42,7 +42,6 @@ impl SVGPathData {
     /// Get the array of PathCommand.
     #[inline]
     pub fn commands(&self) -> &[PathCommand] {
-        debug_assert!(!self.0.is_empty());
         &self.0
     }
 
@@ -92,10 +91,6 @@ impl Parse for SVGPathData {
     ) -> Result<Self, ParseError<'i>> {
         let location = input.current_source_location();
         let path_string = input.expect_string()?.as_ref();
-        if path_string.is_empty() {
-            // Treat an empty string as invalid, so we will not set it.
-            return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
-        }
 
         // Parse the svg path string as multiple sub-paths.
         let mut path_parser = PathParser::new(path_string);

testing/web-platform/tests/css/css-masking/parsing/clip-path-invalid.html

@@ -48,7 +48,6 @@ test_invalid_value("clip-path", "unknown-box");
 
 test_invalid_value("clip-path", 'path(abc, "m 20 0 h -100 z")');
 test_invalid_value("clip-path", 'path(nonzero)');
-test_invalid_value("clip-path", 'path(evenodd, "")');
 test_invalid_value("clip-path", 'path("m 20 0 h -100", nonzero)');
 
 

testing/web-platform/tests/css/css-masking/parsing/clip-path-valid.html

@@ -49,6 +49,10 @@ test_valid_value("clip-path", 'path(evenodd, "M 20 20 h 60 v 60 h -60 Z M 30 30
 test_valid_value("clip-path",
   'path(nonzero, "M20,20h60 v60 h-60z M30,30 h40 v40 h-40z")',
   'path("M 20 20 h 60 v 60 h -60 Z M 30 30 h 40 v 40 h -40 Z")');
+// See https://github.com/w3c/fxtf-drafts/issues/392. If empty path string,
+// Blink serializes it as none, but Gecko serializes as path("").
+test_valid_value("clip-path", 'path(" ")', ["none", 'path("")']);
+test_valid_value("clip-path", 'path(evenodd, "")', ["none", 'path(evenodd, "")']);
 
 // <geometry-box>
 test_valid_value("clip-path", "border-box");

testing/web-platform/tests/css/motion/parsing/offset-path-parsing-valid.html

@@ -28,6 +28,10 @@ test_valid_value("offset-path", 'path("M 0 0 L 100 100 m 0 100 l 100 0 Z l 160 2
 test_valid_value("offset-path", 'path("m 10 20 l 20 30 Z l 50 60 Z m 70 80 l 90 60 Z t 70 120")');
 test_valid_value("offset-path", 'path("m 10 170 h 90 v 30 m 0 0 s 1 2 3 4 z c 9 8 7 6 5 4")', 'path("m 10 170 h 90 v 30 m 0 0 s 1 2 3 4 Z c 9 8 7 6 5 4")');
 test_valid_value("offset-path", 'path("m 10 20 a 10 20 30 1 0 40 50 a 110 120 30 1 1 140 50")');
+// See https://github.com/w3c/fxtf-drafts/issues/392. If empty path string,
+// Blink serializes it as none, but Gecko serializes as path("").
+test_valid_value("offset-path", 'path("")', ['none', 'path("")']);
+test_valid_value("offset-path", 'path(" ")', ['none', 'path("")']);
 
 test_valid_value("offset-path", 'url("http://www.example.com/index.html#polyline1")');