From 6741f8fcccad79da57bf59ee644d8f58b9f20196 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 17 May 2023 19:58:11 +0000 Subject: [PATCH] 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 | 16 ++--- .../style/values/specified/basic_shape.rs | 12 ++-- .../style/values/specified/motion.rs | 6 +- .../components/style/values/specified/svg.rs | 2 +- .../style/values/specified/svg_path.rs | 68 +++++++++++++------ .../parsing/clip-path-invalid.html | 2 + .../css-masking/parsing/clip-path-valid.html | 4 -- .../parsing/offset-path-parsing-invalid.html | 2 + .../parsing/offset-path-parsing-valid.html | 4 -- 9 files changed, 71 insertions(+), 45 deletions(-) diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js index d9f409456610..dfe76389ec4f 100644 --- a/layout/style/test/property_database.js +++ b/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")) { diff --git a/servo/components/style/values/specified/basic_shape.rs b/servo/components/style/values/specified/basic_shape.rs index 3c571ff8e84c..c085446c642f 100644 --- a/servo/components/style/values/specified/basic_shape.rs +++ b/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> { 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> { + 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 }) } } diff --git a/servo/components/style/values/specified/motion.rs b/servo/components/style/values/specified/motion.rs index 57cdfac48069..4c5a77af71d4 100644 --- a/servo/components/style/values/specified/motion.rs +++ b/servo/components/style/values/specified/motion.rs @@ -69,6 +69,8 @@ impl Parse for OffsetPath { context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { + 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 , , // and . - "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()) diff --git a/servo/components/style/values/specified/svg.rs b/servo/components/style/values/specified/svg.rs index 73e0b16ac5c1..24b7d3f3aecc 100644 --- a/servo/components/style/values/specified/svg.rs +++ b/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)) } } diff --git a/servo/components/style/values/specified/svg_path.rs b/servo/components/style/values/specified/svg_path.rs index 7096e1b35d50..4dea8bc72e24 100644 --- a/servo/components/style/values/specified/svg_path.rs +++ b/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> { + 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> { - 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) } } diff --git a/testing/web-platform/tests/css/css-masking/parsing/clip-path-invalid.html b/testing/web-platform/tests/css/css-masking/parsing/clip-path-invalid.html index 6b91f74ad484..1dbdb5acab0c 100644 --- a/testing/web-platform/tests/css/css-masking/parsing/clip-path-invalid.html +++ b/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)'); diff --git a/testing/web-platform/tests/css/css-masking/parsing/clip-path-valid.html b/testing/web-platform/tests/css/css-masking/parsing/clip-path-valid.html index ec6ac5ae9bc3..895cdfacac6c 100644 --- a/testing/web-platform/tests/css/css-masking/parsing/clip-path-valid.html +++ b/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, "")']); // test_valid_value("clip-path", "border-box"); diff --git a/testing/web-platform/tests/css/motion/parsing/offset-path-parsing-invalid.html b/testing/web-platform/tests/css/motion/parsing/offset-path-parsing-invalid.html index 949e45d828f1..28b45db8469f 100644 --- a/testing/web-platform/tests/css/motion/parsing/offset-path-parsing-invalid.html +++ b/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)"); diff --git a/testing/web-platform/tests/css/motion/parsing/offset-path-parsing-valid.html b/testing/web-platform/tests/css/motion/parsing/offset-path-parsing-valid.html index 25915b9a29ee..aab1412577bc 100644 --- a/testing/web-platform/tests/css/motion/parsing/offset-path-parsing-valid.html +++ b/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")');