Auto merge of #16453 - KuoE0:generate-valid-form-for-position, r=upsuper

Generate valid form for {background|mask}-position

<!-- Please describe your changes on the following line: -->

This issue is reported at https://bugzilla.mozilla.org/show_bug.cgi?id=1355017.

The following style
> background-position-x: 10%;
> background-position-y: top 3em;

generates

> background-position: 10% top 3em;

which is invalid. The correct form is

> background-position: left 10% top 3em;

The serialization of background-position should be implemented as a whole rather than just calling `to_css()` on its two longhand properties separately.

spec: https://drafts.csswg.org/css-backgrounds-4/#propdef-background-position

---
<!-- 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 [Bug 1355017](https://bugzilla.mozilla.org/show_bug.cgi?id=1355017)

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

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/16453)
<!-- Reviewable:end -->

components/style/properties/shorthand/background.mako.rs

@@ -159,11 +159,17 @@
                 }
 
                 try!(image.to_css(dest));
-                % for name in "repeat attachment position_x position_y".split():
+                % for name in "repeat attachment".split():
                     try!(write!(dest, " "));
                     try!(${name}.to_css(dest));
                 % endfor
 
+                try!(write!(dest, " "));
+                Position {
+                    horizontal: position_x.clone(),
+                    vertical: position_y.clone()
+                }.to_css(dest)?;
+
                 if *size != background_size::single_value::get_initial_specified_value() {
                     try!(write!(dest, " / "));
                     try!(size.to_css(dest));
@@ -225,9 +231,11 @@
                 return Ok(());
             }
             for i in 0..len {
-                self.background_position_x.0[i].to_css(dest)?;
+                Position {
-                dest.write_str(" ")?;
+                    horizontal: self.background_position_x.0[i].clone(),
-                self.background_position_y.0[i].to_css(dest)?;
+                    vertical: self.background_position_y.0[i].clone()
+                }.to_css(dest)?;
+
                 if i < len - 1 {
                     dest.write_str(", ")?;
                 }

components/style/properties/shorthand/mask.mako.rs

@@ -148,9 +148,12 @@
                 dest.write_str(" ")?;
                 mode.to_css(dest)?;
                 dest.write_str(" ")?;
-                position_x.to_css(dest)?;
+
-                dest.write_str(" ")?;
+                Position {
-                position_y.to_css(dest)?;
+                    horizontal: position_x.clone(),
+                    vertical: position_y.clone()
+                }.to_css(dest)?;
+
                 if *size != mask_size::single_value::get_initial_specified_value() {
                     dest.write_str(" / ")?;
                     size.to_css(dest)?;
@@ -218,9 +221,11 @@
             }
 
             for i in 0..len {
-                self.mask_position_x.0[i].to_css(dest)?;
+                Position {
-                dest.write_str(" ")?;
+                    horizontal: self.mask_position_x.0[i].clone(),
-                self.mask_position_y.0[i].to_css(dest)?;
+                    vertical: self.mask_position_y.0[i].clone()
+                }.to_css(dest)?;
+
                 if i < len - 1 {
                     dest.write_str(", ")?;
                 }

components/style/values/specified/position.rs

@@ -32,27 +32,36 @@ pub struct Position {
 
 impl ToCss for Position {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-        let mut space_present = false;
+        macro_rules! to_css_with_keyword {
-        if let Some(horiz_key) = self.horizontal.keyword {
+            ($pos:expr, $default_keyword:expr) => {
-            try!(horiz_key.to_css(dest));
+                $pos.keyword.unwrap_or($default_keyword).to_css(dest)?;
-            try!(dest.write_str(" "));
+
-            space_present = true;
+                if let Some(ref position) = $pos.position {
-        };
+                    dest.write_str(" ")?;
-        if let Some(ref horiz_pos) = self.horizontal.position {
+                    position.to_css(dest)?;
-            try!(horiz_pos.to_css(dest));
+                };
-            try!(dest.write_str(" "));
+            };
-            space_present = true;
+        }
-        };
+
-        if let Some(vert_key) = self.vertical.keyword {
+        let mut is_keyword_needed = false;
-            try!(vert_key.to_css(dest));
+        let position_x = &self.horizontal;
-            space_present = false;
+        let position_y = &self.vertical;
-        };
+
-        if let Some(ref vert_pos) = self.vertical.position {
+        if (position_x.keyword.is_some() && position_x.position.is_some()) ||
-            if space_present == false {
+           (position_y.keyword.is_some() && position_y.position.is_some()) {
-                try!(dest.write_str(" "));
+            is_keyword_needed = true;
-            }
+        }
-            try!(vert_pos.to_css(dest));
+
-        };
+        if is_keyword_needed {
+            to_css_with_keyword!(position_x, Keyword::Left);
+            dest.write_str(" ")?;
+            to_css_with_keyword!(position_y, Keyword::Top);
+        } else {
+            position_x.to_css(dest)?;
+            dest.write_str(" ")?;
+            position_y.to_css(dest)?;
+        }
+
         Ok(())
     }
 }
@@ -403,6 +412,7 @@ impl ToCss for VerticalPosition {
         };
         Ok(())
     }
+
 }
 
 impl Parse for VerticalPosition {

tests/unit/style/properties/serialization.rs

@@ -660,7 +660,7 @@ mod shorthand_serialization {
                 background-attachment: scroll; \
                 background-size: 70px 50px; \
                 background-position-x: 7px; \
-                background-position-y: 4px; \
+                background-position-y: bottom 4px; \
                 background-origin: border-box; \
                 background-clip: padding-box;";
 
@@ -671,7 +671,7 @@ mod shorthand_serialization {
             assert_eq!(
                 serialization,
                 "background: rgb(255, 0, 0) url(\"http://servo/test.png\") repeat-x \
-                scroll 7px 4px / 70px 50px border-box padding-box;"
+                scroll left 7px bottom 4px / 70px 50px border-box padding-box;"
             );
         }
 
@@ -749,6 +749,27 @@ mod shorthand_serialization {
 
             assert_eq!(serialization, block_text);
         }
+
+        #[test]
+        fn background_position_should_be_a_valid_form_its_longhands() {
+            // If there is any longhand consisted of both keyword and position,
+            // the shorthand result should be the 4-value format.
+            let block_text = "\
+                background-position-x: 30px;\
+                background-position-y: bottom 20px;";
+            let block = parse(|c, i| Ok(parse_property_declaration_list(c, i)), block_text).unwrap();
+            let serialization = block.to_css_string();
+            assert_eq!(serialization, "background-position: left 30px bottom 20px;");
+
+            // If there is no longhand consisted of both keyword and position,
+            // the shorthand result should be the 2-value format.
+            let block_text = "\
+                background-position-x: center;\
+                background-position-y: 20px;";
+            let block = parse(|c, i| Ok(parse_property_declaration_list(c, i)), block_text).unwrap();
+            let serialization = block.to_css_string();
+            assert_eq!(serialization, "background-position: center 20px;");
+        }
     }
 
     mod mask {
@@ -762,7 +783,7 @@ mod shorthand_serialization {
         use style::properties::longhands::mask_repeat as repeat;
         use style::properties::longhands::mask_size as size;
         use style::values::specified::Image;
-        use style::values::specified::position::{HorizontalPosition, VerticalPosition};
+        use style::values::specified::position::{HorizontalPosition, VerticalPosition, Keyword};
         use super::*;
 
         macro_rules! single_vec_value_typedef {
@@ -805,7 +826,7 @@ mod shorthand_serialization {
             );
             let position_y = single_vec_value_typedef!(position_y,
                 VerticalPosition {
-                    keyword: None,
+                    keyword: Some(Keyword::Bottom),
                     position: Some(LengthOrPercentage::Length(NoCalcLength::from_px(4f32))),
                 }
             );
@@ -837,7 +858,7 @@ mod shorthand_serialization {
             let serialization = shorthand_properties_to_string(properties);
             assert_eq!(
                 serialization,
-                "mask: url(\"http://servo/test.png\") luminance 7px 4px / 70px 50px \
+                "mask: url(\"http://servo/test.png\") luminance left 7px bottom 4px / 70px 50px \
                 repeat-x padding-box border-box subtract;"
             );
         }
@@ -905,6 +926,27 @@ mod shorthand_serialization {
             let serialization = block.to_css_string();
             assert_eq!(serialization, block_text);
         }
+
+        #[test]
+        fn mask_position_should_be_a_valid_form_its_longhands() {
+            // If there is any longhand consisted of both keyword and position,
+            // the shorthand result should be the 4-value format.
+            let block_text = "\
+                mask-position-x: 30px;\
+                mask-position-y: bottom 20px;";
+            let block = parse(|c, i| Ok(parse_property_declaration_list(c, i)), block_text).unwrap();
+            let serialization = block.to_css_string();
+            assert_eq!(serialization, "mask-position: left 30px bottom 20px;");
+
+            // If there is no longhand consisted of both keyword and position,
+            // the shorthand result should be the 2-value format.
+            let block_text = "\
+                mask-position-x: center;\
+                mask-position-y: 20px;";
+            let block = parse(|c, i| Ok(parse_property_declaration_list(c, i)), block_text).unwrap();
+            let serialization = block.to_css_string();
+            assert_eq!(serialization, "mask-position: center 20px;");
+        }
     }
 
     mod scroll_snap_type {