Bug 1832729 - Reject empty svg path string for basic shape path function. r=emilio

Per CSS shape spec, the empty path string is invalid. However, for SVG
d property, the EBNF allows the path data string in the d property to be
empty. We just disable the rendering of the path.

Note: only offset-path and clip-path are affected.

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

layout/style/test/property_database.js

@@ -1382,8 +1382,6 @@ if (/* mozGradientsEnabled */ true) {
 
 const pathValues = {
   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')",
@@ -2884,7 +2882,7 @@ var gCSSProperties = {
     inherited: false,
     type: CSS_TYPE_LONGHAND,
     initial_values: ["none"],
-    other_values: pathValues.other_values,
+    other_values: ["path('')", "path(' ')"].concat(pathValues.other_values),
     invalid_values: pathValues.invalid_values,
   },
   "-moz-float-edge": {
@@ -8671,7 +8669,6 @@ var gCSSProperties = {
     type: CSS_TYPE_LONGHAND,
     initial_values: ["none"],
     other_values: [
-      "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')",
@@ -8681,9 +8678,12 @@ var gCSSProperties = {
     ]
       .concat(basicShapeSVGBoxValues)
       .concat(basicShapeOtherValues),
-    invalid_values: ["path(nonzero)", "path(abs, 'M 10 10 L 10 10 z')"].concat(
-      basicShapeInvalidValues
-    ),
+    invalid_values: [
+      "path(nonzero)",
+      "path(abs, 'M 10 10 L 10 10 z')",
+      "path(evenodd, '')",
+      "path('')",
+    ].concat(basicShapeInvalidValues),
     unbalanced_values: basicShapeUnbalancedValues,
   },
   "clip-rule": {
@@ -13442,7 +13442,7 @@ if (IsCSSPropertyPrefEnabled("layout.css.motion-path.enabled")) {
     type: CSS_TYPE_LONGHAND,
     initial_values: ["none"],
     other_values: [...pathValues.other_values],
-    invalid_values: [...pathValues.invalid_values],
+    invalid_values: ["path('')"].concat(pathValues.invalid_values),
   };
 
   if (IsCSSPropertyPrefEnabled("layout.css.motion-path-ray.enabled")) {

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

@@ -15,8 +15,7 @@ use crate::values::specified::border::BorderRadius;
 use crate::values::specified::image::Image;
 use crate::values::specified::position::{HorizontalPosition, Position, VerticalPosition};
 use crate::values::specified::url::SpecifiedUrl;
-use crate::values::specified::SVGPathData;
-use crate::values::specified::{LengthPercentage, NonNegativeLengthPercentage};
+use crate::values::specified::{LengthPercentage, NonNegativeLengthPercentage, SVGPathData};
 use crate::Zero;
 use cssparser::Parser;
 use style_traits::{ParseError, StyleParseErrorKind};
@@ -294,20 +293,21 @@ impl Polygon {
 
 impl Parse for Path {
     fn parse<'i, 't>(
-        context: &ParserContext,
+        _context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         input.expect_function_matching("path")?;
-        input.parse_nested_block(|i| Self::parse_function_arguments(context, i))
+        input.parse_nested_block(Self::parse_function_arguments)
     }
 }
 
 impl Path {
     /// Parse the inner arguments of a `path` function.
     fn parse_function_arguments<'i, 't>(
-        context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
+        use crate::values::specified::svg_path::AllowEmpty;
+
         let fill = input
             .try_parse(|i| -> Result<_, ParseError> {
                 let fill = FillRule::parse(i)?;
@@ -315,7 +315,7 @@ impl Path {
                 Ok(fill)
             })
             .unwrap_or_default();
-        let path = SVGPathData::parse(context, input)?;
+        let path = SVGPathData::parse(input, AllowEmpty::No)?;
         Ok(Path { fill, path })
     }
 }

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

@@ -69,6 +69,8 @@ impl Parse for OffsetPath {
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
+        use crate::values::specified::svg_path::AllowEmpty;
+
         // Parse none.
         if input.try_parse(|i| i.expect_ident_matching("none")).is_ok() {
             return Ok(OffsetPath::none());
@@ -81,8 +83,8 @@ impl Parse for OffsetPath {
             match_ignore_ascii_case! { &function,
                 // Bug 1186329: Implement the parser for <basic-shape>, <geometry-box>,
                 // and <url>.
-                "path" => SVGPathData::parse(context, i).map(GenericOffsetPath::Path),
-                "ray" => RayFunction::parse(context, i).map(GenericOffsetPath::Ray),
+                "path" => SVGPathData::parse(i, AllowEmpty::No).map(OffsetPath::Path),
+                "ray" => RayFunction::parse(context, i).map(OffsetPath::Ray),
                 _ => {
                     Err(location.new_custom_error(
                         StyleParseErrorKind::UnexpectedFunction(function.clone())

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

@@ -385,7 +385,7 @@ impl Parse for DProperty {
 
         // Parse possible functions.
         input.expect_function_matching("path")?;
-        let path_data = input.parse_nested_block(|i| SVGPathData::parse(context, i))?;
+        let path_data = input.parse_nested_block(|i| Parse::parse(context, i))?;
         Ok(DProperty::Path(path_data))
     }
 }

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

@@ -5,7 +5,7 @@
 //! Specified types for SVG Path.
 
 use crate::parser::{Parse, ParserContext};
-use crate::values::animated::{Animate, Procedure, ToAnimatedZero, lists};
+use crate::values::animated::{lists, Animate, Procedure, ToAnimatedZero};
 use crate::values::distance::{ComputeSquaredDistance, SquaredDistance};
 use crate::values::CSSFloat;
 use cssparser::Parser;
@@ -15,6 +15,14 @@ use std::slice;
 use style_traits::values::SequenceWriter;
 use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};
 
+/// Whether to allow empty string in the parser.
+#[derive(Clone, Debug, Eq, PartialEq)]
+#[allow(missing_docs)]
+pub enum AllowEmpty {
+    Yes,
+    No,
+}
+
 /// The SVG path data.
 ///
 /// https://www.w3.org/TR/SVG11/paths.html#PathData
@@ -160,6 +168,41 @@ impl SVGPathData {
 
         Ok(SVGPathData(crate::ArcSlice::from_iter(result.into_iter())))
     }
+
+    /// Parse this SVG path string with the argument that indicates whether we should allow the
+    /// empty string.
+    // We cannot use cssparser::Parser to parse a SVG path string because the spec wants to make
+    // the SVG path string as compact as possible. (i.e. The whitespaces may be dropped.)
+    // e.g. "M100 200L100 200" is a valid SVG path string. If we use tokenizer, the first ident
+    // is "M100", instead of "M", and this is not correct. Therefore, we use a Peekable
+    // str::Char iterator to check each character.
+    pub fn parse<'i, 't>(
+        input: &mut Parser<'i, 't>,
+        allow_empty: AllowEmpty,
+    ) -> Result<Self, ParseError<'i>> {
+        let location = input.current_source_location();
+        let path_string = input.expect_string()?.as_ref();
+
+        // Parse the svg path string as multiple sub-paths.
+        let mut path_parser = PathParser::new(path_string);
+        while skip_wsp(&mut path_parser.chars) {
+            if path_parser.parse_subpath().is_err() {
+                return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+            }
+        }
+
+        // The css-shapes-1 says a path data string that does conform but defines an empty path is
+        // invalid and causes the entire path() to be invalid, so we use the argement to decide
+        // whether we should allow the empty string.
+        // https://drafts.csswg.org/css-shapes-1/#typedef-basic-shape
+        if matches!(allow_empty, AllowEmpty::No) && path_parser.path.is_empty() {
+            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+        }
+
+        Ok(SVGPathData(crate::ArcSlice::from_iter(
+            path_parser.path.into_iter(),
+        )))
+    }
 }
 
 impl ToCss for SVGPathData {
@@ -180,29 +223,14 @@ impl ToCss for SVGPathData {
 }
 
 impl Parse for SVGPathData {
-    // We cannot use cssparser::Parser to parse a SVG path string because the spec wants to make
-    // the SVG path string as compact as possible. (i.e. The whitespaces may be dropped.)
-    // e.g. "M100 200L100 200" is a valid SVG path string. If we use tokenizer, the first ident
-    // is "M100", instead of "M", and this is not correct. Therefore, we use a Peekable
-    // str::Char iterator to check each character.
     fn parse<'i, 't>(
         _context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
-        let location = input.current_source_location();
-        let path_string = input.expect_string()?.as_ref();
-
-        // Parse the svg path string as multiple sub-paths.
-        let mut path_parser = PathParser::new(path_string);
-        while skip_wsp(&mut path_parser.chars) {
-            if path_parser.parse_subpath().is_err() {
-                return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
-            }
-        }
-
-        Ok(SVGPathData(crate::ArcSlice::from_iter(
-            path_parser.path.into_iter(),
-        )))
+        // Note that the EBNF allows the path data string in the d property to be empty, so we
+        // don't reject empty SVG path data.
+        // https://svgwg.org/svg2-draft/single-page.html#paths-PathDataBNF
+        SVGPathData::parse(input, AllowEmpty::Yes)
     }
 }
 

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

@@ -46,6 +46,8 @@ test_invalid_value("clip-path", "polygon(1%)");
 
 test_invalid_value("clip-path", "unknown-box");
 
+test_invalid_value("clip-path", 'path(" ")');
+test_invalid_value("clip-path", 'path(evenodd, "")');
 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("m 20 0 h -100", nonzero)');

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

@@ -49,10 +49,6 @@ 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-invalid.html

@@ -15,6 +15,8 @@
 // arc path segments must have at least 7 arguments.
 // https://www.w3.org/TR/SVG/paths.html#PathDataEllipticalArcCommands
 test_invalid_value("offset-path", 'path("M 20 30 A 60 70 80")');
+test_invalid_value("offset-path", 'path("")');
+test_invalid_value("offset-path", 'path(" ")');
 
 test_invalid_value("offset-path", "ray(0 sides)");
 test_invalid_value("offset-path", "ray(closest-side)");

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

@@ -41,10 +41,6 @@ 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"  )  ',
     '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")');