From 2358e4654b4848b9d12c9d0a38522359a515b234 Mon Sep 17 00:00:00 2001 From: KuoE0 Date: Thu, 13 Apr 2017 16:35:26 +0800 Subject: [PATCH 1/2] Generate valid form for {background|mask}-position from their longhands. If there is any longhand consisting of both keyword and position, we should attach both keyword and position for both longhands when serialize them. 1. To generate a serialized result with keyword, we add a macro function `to_css_with_keyword` accepted HorizontalPosition and VerticalPosition as the parameter to finish that. For the longhands which missing keyword, we use the default keyword. (`left` for `*-position-x` and `top` for `*-position-y`) 2. Update `Position::to_css` to generate the valid format by calling `to_css_with_keyword` with HorizontalPosition and VerticalPosition. 3. Update `to_css` to use new `Position::to_css` in background, background-position, mask and mask-position. MozReview-Commit-ID: 5Bnhdsi5yeM --- .../properties/shorthand/background.mako.rs | 16 ++++-- .../style/properties/shorthand/mask.mako.rs | 17 +++--- components/style/values/specified/position.rs | 52 +++++++++++-------- 3 files changed, 54 insertions(+), 31 deletions(-) diff --git a/components/style/properties/shorthand/background.mako.rs b/components/style/properties/shorthand/background.mako.rs index 4e69dca81c3..8eaa94772f1 100644 --- a/components/style/properties/shorthand/background.mako.rs +++ b/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)?; - dest.write_str(" ")?; - self.background_position_y.0[i].to_css(dest)?; + Position { + horizontal: self.background_position_x.0[i].clone(), + vertical: self.background_position_y.0[i].clone() + }.to_css(dest)?; + if i < len - 1 { dest.write_str(", ")?; } diff --git a/components/style/properties/shorthand/mask.mako.rs b/components/style/properties/shorthand/mask.mako.rs index 8419a7d5951..8096f223757 100644 --- a/components/style/properties/shorthand/mask.mako.rs +++ b/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_y.to_css(dest)?; + + Position { + 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)?; - dest.write_str(" ")?; - self.mask_position_y.0[i].to_css(dest)?; + Position { + horizontal: self.mask_position_x.0[i].clone(), + vertical: self.mask_position_y.0[i].clone() + }.to_css(dest)?; + if i < len - 1 { dest.write_str(", ")?; } diff --git a/components/style/values/specified/position.rs b/components/style/values/specified/position.rs index 5c251cf54d5..edbe8c8472d 100644 --- a/components/style/values/specified/position.rs +++ b/components/style/values/specified/position.rs @@ -32,27 +32,36 @@ pub struct Position { impl ToCss for Position { fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { - let mut space_present = false; - if let Some(horiz_key) = self.horizontal.keyword { - try!(horiz_key.to_css(dest)); - try!(dest.write_str(" ")); - space_present = true; - }; - if let Some(ref horiz_pos) = self.horizontal.position { - try!(horiz_pos.to_css(dest)); - try!(dest.write_str(" ")); - space_present = true; - }; - if let Some(vert_key) = self.vertical.keyword { - try!(vert_key.to_css(dest)); - space_present = false; - }; - if let Some(ref vert_pos) = self.vertical.position { - if space_present == false { - try!(dest.write_str(" ")); - } - try!(vert_pos.to_css(dest)); - }; + macro_rules! to_css_with_keyword { + ($pos:expr, $default_keyword:expr) => { + $pos.keyword.unwrap_or($default_keyword).to_css(dest)?; + + if let Some(ref position) = $pos.position { + dest.write_str(" ")?; + position.to_css(dest)?; + }; + }; + } + + let mut is_keyword_needed = false; + let position_x = &self.horizontal; + let position_y = &self.vertical; + + if (position_x.keyword.is_some() && position_x.position.is_some()) || + (position_y.keyword.is_some() && position_y.position.is_some()) { + is_keyword_needed = true; + } + + 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 { From e52b6b85910e2cda8a72dc518dd13f3749044a64 Mon Sep 17 00:00:00 2001 From: KuoE0 Date: Fri, 14 Apr 2017 10:54:53 +0800 Subject: [PATCH 2/2] Add test case for {background|mask}-position. MozReview-Commit-ID: B6torf6dw5N --- tests/unit/style/properties/serialization.rs | 52 ++++++++++++++++++-- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/tests/unit/style/properties/serialization.rs b/tests/unit/style/properties/serialization.rs index 3de30df2a97..770e94701e7 100644 --- a/tests/unit/style/properties/serialization.rs +++ b/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 {