Auto merge of #13804 - canaltinova:gradient-parsing, r=Manishearth

Fix radial gradient's <size>/<ending-shape> parsing

<!-- Please describe your changes on the following line: -->
Parsing now handles sizes and shapes in various order.
I had to delete `EndingShape`'s parse implementation and mix it with `Position` in `parse_radial`. It became a bit complicated to read but I couldn't make it simpler.
r? @Manishearth

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13664 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13804)
<!-- Reviewable:end -->

components/style/values/specified/image.rs

@@ -14,7 +14,7 @@ use std::fmt;
 use url::Url;
 use values::computed::ComputedValueAsSpecified;
 use values::specified::{Angle, CSSColor, Length, LengthOrPercentage, UrlExtraData};
-use values::specified::position::{Keyword, Position};
+use values::specified::position::Position;
 
 /// Specified values for an image according to CSS-IMAGES.
 /// https://drafts.csswg.org/css-images/#image-values
@@ -163,23 +163,48 @@ impl GradientKind {
 
     /// Parses a radial gradient from the given arguments.
     pub fn parse_radial(input: &mut Parser) -> Result<GradientKind, ()> {
-        let mut needs_comma = false;
+        let mut needs_comma = true;
-        let shape = if let Ok(shape) = EndingShape::parse(input) {
-            needs_comma = true;
-            shape
-        } else {
-            EndingShape::Circle(LengthOrKeyword::Keyword(SizeKeyword::FarthestSide))
-        };
 
-        let position = if input.try(|input| input.expect_ident_matching("at")).is_ok() {
+        // Ending shape and position can be in various order. Checks all probabilities.
-            needs_comma = true;
+        let (shape, position) = if let Ok(position) = input.try(parse_position) {
-            try!(Position::parse(input))
+            // Handle just <position>
+            (EndingShape::Ellipse(LengthOrPercentageOrKeyword::Keyword(SizeKeyword::FarthestCorner)), position)
+        } else if let Ok((first, second)) = input.try(parse_two_length) {
+            // Handle <LengthOrPercentage> <LengthOrPercentage> <shape>? <position>?
+            let _ = input.try(|input| input.expect_ident_matching("ellipse"));
+            (EndingShape::Ellipse(LengthOrPercentageOrKeyword::LengthOrPercentage(first, second)),
+             input.try(parse_position).unwrap_or(Position::center()))
+        } else if let Ok(length) = input.try(Length::parse) {
+            // Handle <Length> <circle>? <position>?
+            let _ = input.try(|input| input.expect_ident_matching("circle"));
+            (EndingShape::Circle(LengthOrKeyword::Length(length)),
+             input.try(parse_position).unwrap_or(Position::center()))
+        } else if let Ok(keyword) = input.try(SizeKeyword::parse) {
+            // Handle <keyword> <shape-keyword>? <position>?
+            let shape = if input.try(|input| input.expect_ident_matching("circle")).is_ok() {
+                EndingShape::Circle(LengthOrKeyword::Keyword(keyword))
+            } else {
+                let _ = input.try(|input| input.expect_ident_matching("ellipse"));
+                EndingShape::Ellipse(LengthOrPercentageOrKeyword::Keyword(keyword))
+            };
+            (shape, input.try(parse_position).unwrap_or(Position::center()))
         } else {
-            Position {
+            // Handle <shape-keyword> <length>? <position>?
-                horiz_keyword: Some(Keyword::Center),
+            if input.try(|input| input.expect_ident_matching("ellipse")).is_ok() {
-                horiz_position: None,
+                // Handle <ellipse> <LengthOrPercentageOrKeyword>? <position>?
-                vert_keyword: Some(Keyword::Center),
+                let length = input.try(LengthOrPercentageOrKeyword::parse)
-                vert_position: None,
+                                  .unwrap_or(LengthOrPercentageOrKeyword::Keyword(SizeKeyword::FarthestCorner));
+                (EndingShape::Ellipse(length), input.try(parse_position).unwrap_or(Position::center()))
+            } else if input.try(|input| input.expect_ident_matching("circle")).is_ok() {
+                // Handle <ellipse> <LengthOrKeyword>? <position>?
+                let length = input.try(LengthOrKeyword::parse)
+                                  .unwrap_or(LengthOrKeyword::Keyword(SizeKeyword::FarthestCorner));
+                (EndingShape::Circle(length), input.try(parse_position).unwrap_or(Position::center()))
+            } else {
+                // If there is no shape keyword, it should set to default.
+                needs_comma = false;
+                (EndingShape::Ellipse(LengthOrPercentageOrKeyword::Keyword(SizeKeyword::FarthestCorner)),
+                 input.try(parse_position).unwrap_or(Position::center()))
             }
         };
 
@@ -191,6 +216,17 @@ impl GradientKind {
     }
 }
 
+fn parse_two_length(input: &mut Parser) -> Result<(LengthOrPercentage, LengthOrPercentage), ()> {
+    let first = try!(LengthOrPercentage::parse(input));
+    let second = try!(LengthOrPercentage::parse(input));
+    Ok((first, second))
+}
+
+fn parse_position(input: &mut Parser) -> Result<Position, ()> {
+    try!(input.expect_ident_matching("at"));
+    input.try(Position::parse)
+}
+
 /// Specified values for an angle or a corner in a linear gradient.
 #[derive(Clone, PartialEq, Copy, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
@@ -301,44 +337,6 @@ pub enum EndingShape {
     Ellipse(LengthOrPercentageOrKeyword),
 }
 
-impl Parse for EndingShape {
-    fn parse(input: &mut Parser) -> Result<Self, ()> {
-        // FIXME(#13664): Normally size can come before shape keywords but currently
-        // parsing fails if size comes before shape keyword.
-        match_ignore_ascii_case! { try!(input.expect_ident()),
-            "circle" => {
-                let position = input.try(LengthOrKeyword::parse).unwrap_or(
-                    LengthOrKeyword::Keyword(SizeKeyword::FarthestSide));
-                Ok(EndingShape::Circle(position))
-            },
-            "ellipse" => {
-              let length = input.try(LengthOrPercentageOrKeyword::parse)
-                                .unwrap_or(LengthOrPercentageOrKeyword::Keyword(SizeKeyword::FarthestSide));
-                Ok(EndingShape::Ellipse(length))
-            },
-            _ => {
-                // If two <length> is present, it defaults to ellipse, otherwise defaults to circle.
-                if let Ok(length) = LengthOrPercentageOrKeyword::parse(input) {
-                    if let LengthOrPercentageOrKeyword::Keyword(keyword) = length {
-                        // A single keyword is valid for both ellipse and circle, but we default to circle.
-                        // The grammar for ending shapes for circle and ellipse have overlap so we cannot simply
-                        // try to parse as circle first
-                        Ok(EndingShape::Circle(LengthOrKeyword::Keyword(keyword)))
-                    } else {
-                        Ok(EndingShape::Ellipse(length))
-                    }
-                } else {
-                    // If both shape and size are omitted, we do not parse as an EndingShape
-                    // Instead, GradientKind::parse_radial will go ahead and parse the stops
-                    // This is necessary because GradientKind::parse_radial needs to know
-                    // whether or not to expect a comma
-                    Ok(EndingShape::Circle(try!(input.try(LengthOrKeyword::parse))))
-                }
-            }
-        }
-    }
-}
-
 impl ToCss for EndingShape {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         match *self {

components/style/values/specified/position.rs

@@ -215,6 +215,15 @@ impl Position {
             }
         }
     }
+
+    pub fn center() -> Position {
+        Position {
+            horiz_keyword: Some(Keyword::Center),
+            horiz_position: None,
+            vert_keyword: Some(Keyword::Center),
+            vert_position: None,
+        }
+    }
 }
 
 impl Keyword {

tests/unit/style/parsing/image.rs

@@ -0,0 +1,87 @@
+/* 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 http://mozilla.org/MPL/2.0/. */
+
+use cssparser::Parser;
+use media_queries::CSSErrorReporterTest;
+use style::parser::ParserContext;
+use style::stylesheets::Origin;
+use style::values::specified::image::*;
+use url::Url;
+
+#[test]
+fn test_radial_gradient() {
+    // Parsing with all values
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(circle closest-side at 20px 30px, red, green)");
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(ellipse closest-side at 20px 30px, red, green)");
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(closest-side circle at 20px 30px, red, green)",
+                                                 "radial-gradient(circle closest-side at 20px 30px, red, green)");
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(closest-side ellipse at 20px 30px, red, green)",
+                                                 "radial-gradient(ellipse closest-side at 20px 30px, red, green)");
+
+    // Parsing with <shape-keyword> and <size> reversed
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(closest-side circle at 20px 30px, red, green)",
+                                                 "radial-gradient(circle closest-side at 20px 30px, red, green)");
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(closest-corner ellipse at 20px 30px, red, green)",
+                                                 "radial-gradient(ellipse closest-corner at 20px 30px, red, green)");
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(30px circle, red, green)",
+                                                 "radial-gradient(circle 30px at center center, red, green)");
+    assert_roundtrip_with_context!(Image::parse, "radial-gradient(30px 40px ellipse, red, green)",
+                                                 "radial-gradient(ellipse 30px 40px at center center, red, green)");
+
+    // Parsing without <size>
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(circle, red, green)",
+                                   "radial-gradient(circle farthest-corner at center center, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(ellipse, red, green)",
+                                   "radial-gradient(ellipse farthest-corner at center center, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(circle at 20px 30px, red, green)",
+                                   "radial-gradient(circle farthest-corner at 20px 30px, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(ellipse at 20px 30px, red, green)",
+                                   "radial-gradient(ellipse farthest-corner at 20px 30px, red, green)");
+
+
+    // Parsing without <shape-keyword>
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(20px at 20px 30px, red, green)",
+                                   "radial-gradient(circle 20px at 20px 30px, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(20px 30px at left center, red, green)",
+                                   "radial-gradient(ellipse 20px 30px at left center, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(closest-side at center, red, green)",
+                                   "radial-gradient(ellipse closest-side at center center, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(20px, red, green)",
+                                   "radial-gradient(circle 20px at center center, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(20px 30px, red, green)",
+                                   "radial-gradient(ellipse 20px 30px at center center, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(closest-side, red, green)",
+                                   "radial-gradient(ellipse closest-side at center center, red, green)");
+
+    // Parsing without <shape-keyword> and <size>
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(at center, red, green)",
+                                   "radial-gradient(ellipse farthest-corner at center center, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(at center bottom, red, green)",
+                                   "radial-gradient(ellipse farthest-corner at center bottom, red, green)");
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(at 40px 50px, red, green)",
+                                   "radial-gradient(ellipse farthest-corner at 40px 50px, red, green)");
+
+    // Parsing with just color stops
+    assert_roundtrip_with_context!(Image::parse,
+                                   "radial-gradient(red, green)",
+                                   "radial-gradient(ellipse farthest-corner at center center, red, green)");
+
+    // Parsing repeating radial gradient
+    assert_roundtrip_with_context!(Image::parse,
+                                   "repeating-radial-gradient(red, green)",
+                                   "repeating-radial-gradient(ellipse farthest-corner at center center, red, green)");
+}

tests/unit/style/parsing/mod.rs

@@ -31,6 +31,27 @@ macro_rules! assert_roundtrip {
     }
 }
 
+macro_rules! assert_roundtrip_with_context {
+    ($fun:expr, $string:expr) => {
+        assert_roundtrip_with_context!($fun, $string, $string);
+    };
+    ($fun:expr,$input:expr, $output:expr) => {
+        let url = Url::parse("http://localhost").unwrap();
+        let context = ParserContext::new(Origin::Author, &url, Box::new(CSSErrorReporterTest));
+        let mut parser = Parser::new($input);
+        let parsed = $fun(&context, &mut parser)
+                     .expect(&format!("Failed to parse {}", $input));
+        let serialized = ::cssparser::ToCss::to_css_string(&parsed);
+        assert_eq!(serialized, $output);
+
+        let mut parser = Parser::new(&serialized);
+        let re_parsed = $fun(&context, &mut parser)
+                        .expect(&format!("Failed to parse {}", $input));
+        let re_serialized = ::cssparser::ToCss::to_css_string(&re_parsed);
+        assert_eq!(serialized, re_serialized);
+    }
+}
+
 
 macro_rules! parse_longhand {
     ($name:ident, $s:expr) => {{
@@ -41,6 +62,7 @@ macro_rules! parse_longhand {
 }
 
 mod basic_shape;
+mod image;
 mod mask;
 mod position;
 mod selectors;