Auto merge of #14509 - canaltinova:position, r=Manishearth

Implement background-position-x/y

<!-- Please describe your changes on the following line: -->
This is a WIP PR. Just HorizontalPosition / VerticalPosition implementations are complete. I would like to get early feedbacks about this architecture. Here's some architectural topics to consider:

- I created `HorizontalPosition` and `VerticalPosition` structs for this and used them in `Position` as well. We have decided to split `Keyword` enum, but we need them as unified for `PositionComponent` enum. So I didn't split but I can split it if we prefer to change PositionComponent as well.
- If we prefer Keyword enum like this, we can create a SubPosition(or something like this) instead of HorizontalPosition/VerticalPosition enums since only difference is 2 lines in `parse` functions. We can create a `parse_horizontal` and `parse_vertical` instead and a lot of code duplication can be cleared.
- I couldn't find a good way to use HorizontalPosition/VerticalPosition's parse functions in `Position`'s parse function. It is a bit more complicated. I'm open to suggestions :)
- I don't know much about logical keywords so do I need to do something different? I placed some comments where logical keywords are processing.

Any advice about these?

---
<!-- 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 #14458 (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/14509)
<!-- Reviewable:end -->

components/style/properties/gecko.mako.rs

@@ -1389,6 +1389,7 @@ fn static_assert() {
         }
     </%self:simple_image_array_property>
 
+    % if shorthand != "background":
     pub fn copy_${shorthand}_position_from(&mut self, other: &Self) {
         use gecko_bindings::structs::nsStyleImageLayers_LayerType as LayerType;
 
@@ -1424,7 +1425,7 @@ fn static_assert() {
     pub fn clone_${shorthand}_position(&self)
         -> longhands::${shorthand}_position::computed_value::T {
         use values::computed::position::Position;
-        longhands::background_position::computed_value::T(
+        longhands::${shorthand}_position::computed_value::T(
             self.gecko.${image_layers_field}.mLayers.iter()
                 .take(self.gecko.${image_layers_field}.mPositionXCount as usize)
                 .take(self.gecko.${image_layers_field}.mPositionYCount as usize)
@@ -1453,6 +1454,7 @@ fn static_assert() {
             geckolayer.mPosition.mYPosition = servo.vertical.into();
         }
     }
+    % endif
 
     <%self:simple_image_array_property name="size" shorthand="${shorthand}" field_name="mSize">
         use gecko_bindings::structs::nsStyleImageLayers_Size_Dimension;
@@ -1602,7 +1604,9 @@ fn static_assert() {
                                   background-image background-clip
                                   background-origin background-attachment
                                   background-size background-position
-                                  background-blend-mode""" %>
+                                  background-blend-mode
+                                  background-position-x
+                                  background-position-y""" %>
 <%self:impl_trait style_struct_name="Background"
                   skip_longhands="${skip_background_longhands}"
                   skip_additionals="*">
@@ -1640,6 +1644,56 @@ fn static_assert() {
             T::luminosity => structs::NS_STYLE_BLEND_LUMINOSITY as u8,
         }
     </%self:simple_image_array_property>
+
+    % for orientation in [("x", "Horizontal"), ("y", "Vertical")]:
+    pub fn copy_background_position_${orientation[0]}_from(&mut self, other: &Self) {
+        use gecko_bindings::structs::nsStyleImageLayers_LayerType as LayerType;
+
+        self.gecko.mImage.mPosition${orientation[0].upper()}Count
+                = cmp::min(1, other.gecko.mImage.mPosition${orientation[0].upper()}Count);
+        self.gecko.mImage.mLayers.mFirstElement.mPosition =
+            other.gecko.mImage.mLayers.mFirstElement.mPosition;
+        unsafe {
+            Gecko_EnsureImageLayersLength(&mut self.gecko.mImage,
+                                          other.gecko.mImage.mLayers.len(),
+                                          LayerType::Background);
+        }
+        for (layer, other) in self.gecko.mImage.mLayers.iter_mut()
+                                  .zip(other.gecko.mImage.mLayers.iter()) {
+            layer.mPosition.m${orientation[0].upper()}Position
+                = other.mPosition.m${orientation[0].upper()}Position;
+        }
+        self.gecko.mImage.mPosition${orientation[0].upper()}Count
+                = other.gecko.mImage.mPosition${orientation[0].upper()}Count;
+    }
+
+    pub fn clone_background_position_${orientation[0]}(&self)
+        -> longhands::background_position_${orientation[0]}::computed_value::T {
+        use values::computed::position::${orientation[1]}Position;
+        longhands::background_position_${orientation[0]}::computed_value::T(
+            self.gecko.mImage.mLayers.iter()
+                .take(self.gecko.mImage.mPosition${orientation[0].upper()}Count as usize)
+                .map(|position| ${orientation[1]}Position(position.mPosition.m${orientation[0].upper()}Position.into()))
+                .collect()
+        )
+    }
+
+    pub fn set_background_position_${orientation[0]}(&mut self,
+                                     v: longhands::background_position_${orientation[0]}::computed_value::T) {
+        use gecko_bindings::structs::nsStyleImageLayers_LayerType as LayerType;
+
+        unsafe {
+          Gecko_EnsureImageLayersLength(&mut self.gecko.mImage, v.0.len(),
+                                        LayerType::Background);
+        }
+
+        self.gecko.mImage.mPosition${orientation[0].upper()}Count = v.0.len() as u32;
+        for (servo, geckolayer) in v.0.into_iter().zip(self.gecko.mImage
+                                                           .mLayers.iter_mut()) {
+            geckolayer.mPosition.m${orientation[0].upper()}Position = servo.0.into();
+        }
+    }
+    % endfor
 </%self:impl_trait>
 
 <%self:impl_trait style_struct_name="List"

components/style/properties/helpers/animated_properties.mako.rs

@@ -7,7 +7,8 @@ use cssparser::{Color as CSSParserColor, Parser, RGBA};
 use euclid::{Point2D, Size2D};
 use properties::PropertyDeclaration;
 use properties::longhands;
-use properties::longhands::background_position::computed_value::T as BackgroundPosition;
+use properties::longhands::background_position_x::computed_value::T as BackgroundPositionX;
+use properties::longhands::background_position_y::computed_value::T as BackgroundPositionY;
 use properties::longhands::background_size::computed_value::T as BackgroundSize;
 use properties::longhands::font_weight::computed_value::T as FontWeight;
 use properties::longhands::line_height::computed_value::T as LineHeight;
@@ -26,7 +27,7 @@ use values::Either;
 use values::computed::{Angle, LengthOrPercentageOrAuto, LengthOrPercentageOrNone};
 use values::computed::{BorderRadiusSize, LengthOrNone};
 use values::computed::{CalcLengthOrPercentage, LengthOrPercentage};
-use values::computed::position::Position;
+use values::computed::position::{HorizontalPosition, Position, VerticalPosition};
 use values::computed::ToComputedValue;
 
 
@@ -574,10 +575,37 @@ impl Interpolate for Position {
 
 impl RepeatableListInterpolate for Position {}
 
-impl Interpolate for BackgroundPosition {
+/// https://drafts.csswg.org/css-transitions/#animtype-simple-list
+impl Interpolate for HorizontalPosition {
     #[inline]
     fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
-        Ok(BackgroundPosition(try!(self.0.interpolate(&other.0, progress))))
+        Ok(HorizontalPosition(try!(self.0.interpolate(&other.0, progress))))
+    }
+}
+
+impl RepeatableListInterpolate for HorizontalPosition {}
+
+/// https://drafts.csswg.org/css-transitions/#animtype-simple-list
+impl Interpolate for VerticalPosition {
+    #[inline]
+    fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
+        Ok(VerticalPosition(try!(self.0.interpolate(&other.0, progress))))
+    }
+}
+
+impl RepeatableListInterpolate for VerticalPosition {}
+
+impl Interpolate for BackgroundPositionX {
+    #[inline]
+    fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
+        Ok(BackgroundPositionX(try!(self.0.interpolate(&other.0, progress))))
+    }
+}
+
+impl Interpolate for BackgroundPositionY {
+    #[inline]
+    fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
+        Ok(BackgroundPositionY(try!(self.0.interpolate(&other.0, progress))))
     }
 }
 

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

@@ -86,43 +86,89 @@ ${helpers.predefined_type("background-color", "CSSColor",
     }
 </%helpers:vector_longhand>
 
-<%helpers:vector_longhand name="background-position" animatable="True">
+<%helpers:vector_longhand name="background-position-x" animatable="True">
         use std::fmt;
         use style_traits::ToCss;
         use values::HasViewportPercentage;
-        use values::specified::position::Position;
+        use values::specified::position::HorizontalPosition;
 
         pub mod computed_value {
-            use values::computed::position::Position;
+            use values::computed::position::HorizontalPosition;
             use properties::animated_properties::{Interpolate, RepeatableListInterpolate};
 
-            pub type T = Position;
+            pub type T = HorizontalPosition;
         }
 
-        pub type SpecifiedValue = Position;
+        pub type SpecifiedValue = HorizontalPosition;
 
         #[inline]
         pub fn get_initial_value() -> computed_value::T {
-            use values::computed::position::Position;
-            Position {
-                horizontal: computed::LengthOrPercentage::Percentage(0.0),
-                vertical: computed::LengthOrPercentage::Percentage(0.0),
-            }
+            use values::computed::position::HorizontalPosition;
+            HorizontalPosition(computed::LengthOrPercentage::Percentage(0.0))
         }
         #[inline]
         pub fn get_initial_specified_value() -> SpecifiedValue {
-            use values::specified::Percentage;
-            Position {
-                horiz_keyword: None,
-                horiz_position: Some(specified::LengthOrPercentage::Percentage(Percentage(0.0))),
-                vert_keyword: None,
-                vert_position: Some(specified::LengthOrPercentage::Percentage(Percentage(0.0))),
+            use values::specified::position::Keyword;
+            HorizontalPosition {
+                keyword: Some(Keyword::Left),
+                position: None,
+            }
+        }
+        #[inline]
+        pub fn get_initial_position_value() -> SpecifiedValue {
+            use values::specified::{LengthOrPercentage, Percentage};
+            HorizontalPosition {
+                keyword: None,
+                position: Some(LengthOrPercentage::Percentage(Percentage(0.0))),
             }
         }
 
         pub fn parse(context: &ParserContext, input: &mut Parser)
                      -> Result<SpecifiedValue, ()> {
-            Ok(try!(Position::parse(context, input)))
+            HorizontalPosition::parse(context, input)
+        }
+</%helpers:vector_longhand>
+
+<%helpers:vector_longhand name="background-position-y" animatable="True">
+        use std::fmt;
+        use style_traits::ToCss;
+        use values::HasViewportPercentage;
+        use values::specified::position::VerticalPosition;
+
+        pub mod computed_value {
+            use values::computed::position::VerticalPosition;
+            use properties::animated_properties::{Interpolate, RepeatableListInterpolate};
+
+            pub type T = VerticalPosition;
+        }
+
+        pub type SpecifiedValue = VerticalPosition;
+
+        #[inline]
+        pub fn get_initial_value() -> computed_value::T {
+            use values::computed::position::VerticalPosition;
+            VerticalPosition(computed::LengthOrPercentage::Percentage(0.0))
+        }
+        #[inline]
+        pub fn get_initial_specified_value() -> SpecifiedValue {
+            use values::specified::position::Keyword;
+            VerticalPosition {
+                keyword: Some(Keyword::Top),
+                position: None,
+            }
+        }
+        #[inline]
+        pub fn get_initial_position_value() -> SpecifiedValue {
+            use values::specified::{LengthOrPercentage, Percentage};
+            VerticalPosition {
+                keyword: None,
+                position: Some(LengthOrPercentage::Percentage(Percentage(0.0))),
+            }
+        }
+
+        pub fn parse(context: &ParserContext, input: &mut Parser)
+                     -> Result<SpecifiedValue, ()> {
+            VerticalPosition::parse(context, input)
         }
 </%helpers:vector_longhand>
 

components/style/properties/longhand/svg.mako.rs

@@ -91,21 +91,59 @@ ${helpers.single_keyword("mask-repeat",
                          products="gecko",
                          animatable=False)}
 
-<%helpers:longhand name="mask-position" products="gecko" animatable="True">
-    use properties::longhands::background_position;
-    pub use ::properties::longhands::background_position::SpecifiedValue;
-    pub use ::properties::longhands::background_position::single_value as single_value;
-    pub use ::properties::longhands::background_position::computed_value as computed_value;
+<%helpers:vector_longhand name="mask-position" products="gecko" animatable="True">
+    use std::fmt;
+    use style_traits::ToCss;
+    use values::HasViewportPercentage;
+    use values::specified::position::Position;
+
+    pub mod computed_value {
+        use values::computed::position::Position;
+        use properties::animated_properties::{Interpolate, RepeatableListInterpolate};
+        use properties::longhands::mask_position::computed_value::T as MaskPosition;
+
+        pub type T = Position;
+
+        impl RepeatableListInterpolate for MaskPosition {}
+
+        impl Interpolate for MaskPosition {
+            fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
+                Ok(MaskPosition(try!(self.0.interpolate(&other.0, progress))))
+            }
+        }
+    }
+
+    pub type SpecifiedValue = Position;
 
     #[inline]
     pub fn get_initial_value() -> computed_value::T {
-        background_position::get_initial_value()
+        use values::computed::position::Position;
+        Position {
+            horizontal: computed::LengthOrPercentage::Percentage(0.0),
+            vertical: computed::LengthOrPercentage::Percentage(0.0),
+        }
+    }
+    #[inline]
+    pub fn get_initial_specified_value() -> SpecifiedValue {
+        use values::specified::Percentage;
+        use values::specified::position::{HorizontalPosition, VerticalPosition};
+        Position {
+            horizontal: HorizontalPosition {
+                keyword: None,
+                position: Some(specified::LengthOrPercentage::Percentage(Percentage(0.0))),
+            },
+            vertical: VerticalPosition {
+                keyword: None,
+                position: Some(specified::LengthOrPercentage::Percentage(Percentage(0.0))),
+            },
+        }
     }
 
-    pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
-        background_position::parse(context, input)
+    pub fn parse(context: &ParserContext, input: &mut Parser)
+                 -> Result<SpecifiedValue, ()> {
+        Position::parse(context, input)
     }
-</%helpers:longhand>
+</%helpers:vector_longhand>
 
 // missing: margin-box fill-box stroke-box view-box no-clip
 // (gecko doesn't implement these)

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

@@ -6,10 +6,14 @@
 
 // TODO: other background-* properties
 <%helpers:shorthand name="background"
-                    sub_properties="background-color background-position background-repeat background-attachment
-                                    background-image background-size background-origin background-clip">
-    use properties::longhands::{background_color, background_position, background_repeat, background_attachment};
-    use properties::longhands::{background_image, background_size, background_origin, background_clip};
+                    sub_properties="background-color background-position-x background-position-y background-repeat
+                                    background-attachment background-image background-size background-origin
+                                    background-clip">
+    use properties::longhands::{background_color, background_position_x, background_position_y, background_repeat};
+    use properties::longhands::{background_attachment, background_image, background_size, background_origin};
+    use properties::longhands::background_clip;
+    use values::specified::position::Position;
+    use parser::Parse;
 
     impl From<background_origin::single_value::SpecifiedValue> for background_clip::single_value::SpecifiedValue {
         fn from(origin: background_origin::single_value::SpecifiedValue) ->
@@ -28,11 +32,11 @@
     pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result<Longhands, ()> {
         let mut background_color = None;
 
-        % for name in "image position repeat size attachment origin clip".split():
+        % for name in "image position_x position_y repeat size attachment origin clip".split():
             let mut background_${name} = background_${name}::SpecifiedValue(Vec::new());
         % endfor
         try!(input.parse_comma_separated(|input| {
-            % for name in "image position repeat size attachment origin clip".split():
+            % for name in "image position_x position_y repeat size attachment origin clip".split():
                 let mut ${name} = None;
             % endfor
             loop {
@@ -45,10 +49,10 @@
                         return Err(())
                     }
                 }
-                if position.is_none() {
-                    if let Ok(value) = input.try(|input| background_position::single_value
-                                                                            ::parse(context, input)) {
-                        position = Some(value);
+                if position_x.is_none() && position_y.is_none() {
+                    if let Ok(value) = input.try(|input| Position::parse(context, input)) {
+                        position_x = Some(value.horizontal);
+                        position_y = Some(value.vertical);
 
                         // Parse background size, if applicable.
                         size = input.try(|input| {
@@ -76,12 +80,24 @@
                 }
             }
             let mut any = false;
-            % for name in "image position repeat size attachment origin clip".split():
+            % for name in "image position_x position_y repeat size attachment origin clip".split():
                 any = any || ${name}.is_some();
             % endfor
             any = any || background_color.is_some();
             if any {
-                % for name in "image position repeat size attachment origin clip".split():
+                if position_x.is_some() || position_y.is_some() {
+                    % for name in "position_x position_y".split():
+                        if let Some(bg_${name}) = ${name} {
+                            background_${name}.0.push(bg_${name});
+                        }
+                    % endfor
+                } else {
+                    % for name in "position_x position_y".split():
+                        background_${name}.0.push(background_${name}::single_value
+                                                                    ::get_initial_position_value());
+                    % endfor
+                }
+                % for name in "image repeat size attachment origin clip".split():
                     if let Some(bg_${name}) = ${name} {
                         background_${name}.0.push(bg_${name});
                     } else {
@@ -98,7 +114,8 @@
         Ok(Longhands {
              background_color: background_color,
              background_image: Some(background_image),
-             background_position: Some(background_position),
+             background_position_x: Some(background_position_x),
+             background_position_y: Some(background_position_y),
              background_repeat: Some(background_repeat),
              background_attachment: Some(background_attachment),
              background_size: Some(background_size),
@@ -118,7 +135,7 @@
             }
             use std::cmp;
             let mut len = 0;
-            % for name in "image position repeat size attachment origin clip".split():
+            % for name in "image position_x position_y repeat size attachment origin clip".split():
                 len = cmp::max(len, extract_value(self.background_${name}).map(|i| i.0.len())
                                                                           .unwrap_or(0));
             % endfor
@@ -130,7 +147,7 @@
 
             let mut first = true;
             for i in 0..len {
-                % for name in "image position repeat size attachment origin clip".split():
+                % for name in "image position_x position_y repeat size attachment origin clip".split():
                     let ${name} = if let DeclaredValue::Value(ref arr) = *self.background_${name} {
                         arr.0.get(i % arr.0.len())
                     } else {
@@ -185,8 +202,14 @@
 
                 try!(write!(dest, " "));
 
-                try!(position.unwrap_or(&background_position::single_value
-                                                            ::get_initial_specified_value())
+                try!(position_x.unwrap_or(&background_position_x::single_value
+                                                                ::get_initial_position_value())
+                             .to_css(dest));
+
+                try!(write!(dest, " "));
+
+                try!(position_y.unwrap_or(&background_position_y::single_value
+                                                                ::get_initial_position_value())
                              .to_css(dest));
 
                 if let Some(size) = size {
@@ -223,6 +246,87 @@
             }
 
 
+            Ok(())
+        }
+    }
+</%helpers:shorthand>
+
+<%helpers:shorthand name="background-position"
+                    sub_properties="background-position-x background-position-y">
+    use properties::longhands::{background_position_x,background_position_y};
+    use values::specified::position::Position;
+    use parser::Parse;
+
+    pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result<Longhands, ()> {
+        let mut position_x = background_position_x::SpecifiedValue(Vec::new());
+        let mut position_y = background_position_y::SpecifiedValue(Vec::new());
+        let mut any = false;
+
+        try!(input.parse_comma_separated(|input| {
+            loop {
+                if let Ok(value) = input.try(|input| Position::parse(context, input)) {
+                    position_x.0.push(value.horizontal);
+                    position_y.0.push(value.vertical);
+                    any = true;
+                    continue
+                }
+                break
+            }
+            Ok(())
+        }));
+        if any == false {
+            return Err(());
+        }
+
+        Ok(Longhands {
+            background_position_x: Some(position_x),
+            background_position_y: Some(position_y),
+        })
+    }
+
+    impl<'a> LonghandsToSerialize<'a>  {
+        fn to_css_declared<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+            // mako doesn't like ampersands following `<`
+            fn extract_value<T>(x: &DeclaredValue<T>) -> Option< &T> {
+                match *x {
+                    DeclaredValue::Value(ref val) => Some(val),
+                    _ => None,
+                }
+            }
+            use std::cmp;
+            let mut len = 0;
+            % for name in "x y".split():
+                len = cmp::max(len, extract_value(self.background_position_${name})
+                                                      .map(|i| i.0.len())
+                                                      .unwrap_or(0));
+            % endfor
+
+            // There should be at least one declared value
+            if len == 0 {
+                return dest.write_str("")
+            }
+
+            for i in 0..len {
+                % for name in "x y".split():
+                    let position_${name} = if let DeclaredValue::Value(ref arr) =
+                                           *self.background_position_${name} {
+                        arr.0.get(i % arr.0.len())
+                    } else {
+                        None
+                    };
+                % endfor
+
+                try!(position_x.unwrap_or(&background_position_x::single_value
+                                                                ::get_initial_position_value())
+                               .to_css(dest));
+
+                try!(write!(dest, " "));
+
+                try!(position_y.unwrap_or(&background_position_y::single_value
+                                                                ::get_initial_position_value())
+                               .to_css(dest));
+            }
+
             Ok(())
         }
     }