From bcafe21dc9998c75f39e6a37f04395acd462b0fa Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 22 Feb 2017 21:43:35 +1100 Subject: [PATCH] Animation shorthand should be serialized only when the value lists have the same length. Fixes #15398 The previous commit did most of the work here of updating the algorithm to skip shorthands if a shorthand value was an empty string. This commit just updates animations LonghandToSerialize's implementation of to_css_declared to write an empty string value if the list lengths differ (and updates the test to match). --- .../style/properties/declaration_block.rs | 18 +++++----- .../style/properties/properties.mako.rs | 2 +- .../style/properties/shorthand/box.mako.rs | 32 ++++++++--------- tests/unit/style/properties/serialization.rs | 34 +++++++++---------- 4 files changed, 40 insertions(+), 46 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 7f28235917b..bde41fc6d8e 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -344,23 +344,21 @@ impl ToCss for PropertyDeclarationBlock { // Substep 5 - Let value be the result of invoking serialize a CSS // value of current longhands. - let mut value = String::from(""); + let mut value = String::new(); match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) { None => continue, Some(appendable_value) => { - // Also demo test of overflow with important - I think it'll not - // be correct. try!(append_declaration_value(&mut value, appendable_value)); } } // Substep 6 - if value == "" { + if value.is_empty() { continue } // Substeps 7 and 8 - try!(append_serialization::>, _>( + try!(append_serialization::>, _>( dest, &shorthand, AppendableValue::Css(&value), importance, &mut is_first_serialization)); @@ -417,7 +415,8 @@ pub enum AppendableValue<'a, I> /// FIXME: This needs more docs, where are the shorthands expanded? We print /// the property name before-hand, don't we? DeclarationsForShorthand(ShorthandId, I), - /// A raw CSS string, coming for example from a property with CSS variables. + /// A raw CSS string, coming for example from a property with CSS variables, + /// or when storing a serialized shorthand value before appending directly. Css(&'a str) } @@ -444,7 +443,6 @@ pub fn append_declaration_value<'a, W, I>(dest: &mut W, I: Iterator, { match appendable_value { - // Should be able to use this enum to pass the already serialized css? AppendableValue::Css(css) => { try!(write!(dest, "{}", css)) }, @@ -485,8 +483,10 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W, // for normal parsed values, add a space between key: and value try!(write!(dest, " ")); } - }, - &AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " ")) + }, + // Currently append_serialization is only called with a Css or + // a Declaration AppendableValue. + &AppendableValue::DeclarationsForShorthand(..) => unreachable!() } try!(append_declaration_value(dest, appendable_value)); diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 51c1b8d8432..c3b60b7df0e 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -549,7 +549,7 @@ impl ShorthandId { } } - /// Find an return an appendable value for the given declarations. + /// Finds and returns an appendable value for the given declarations. /// /// Returns the optional appendable value. pub fn get_shorthand_appendable_value<'a, I>(self, diff --git a/components/style/properties/shorthand/box.mako.rs b/components/style/properties/shorthand/box.mako.rs index 9f23b743a27..f2d8f6db5d9 100644 --- a/components/style/properties/shorthand/box.mako.rs +++ b/components/style/properties/shorthand/box.mako.rs @@ -279,27 +279,27 @@ macro_rules! try_parse_one { } } - // TODO: When the lengths are different, shorthand shouldn't be serialized - // at all. - use std::cmp; - let mut len = 0; - % for name in "duration timing_function delay direction fill_mode iteration_count play_state name".split(): - len = cmp::max(len, extract_value(self.animation_${name}).map(|i| i.0.len()) - .unwrap_or(0)); - % endfor - + let len = extract_value(self.animation_name).map(|i| i.0.len()).unwrap_or(0); // There should be at least one declared value if len == 0 { return dest.write_str("") } + // If any value list length is differs then we don't do a shorthand serialization + // either. + % for name in "duration timing_function delay direction fill_mode iteration_count play_state".split(): + if len != extract_value(self.animation_${name}).map(|i| i.0.len()).unwrap_or(0) { + return dest.write_str("") + } + % endfor + let mut first = true; for i in 0..len { % for name in "duration timing_function delay direction fill_mode iteration_count play_state name".split(): let ${name} = if let DeclaredValue::Value(ref arr) = *self.animation_${name} { - arr.0.get(i % arr.0.len()) + &arr.0[i] } else { - None + unreachable!() }; % endfor @@ -310,15 +310,11 @@ macro_rules! try_parse_one { } % for name in "duration timing_function delay direction fill_mode iteration_count play_state".split(): - if let Some(${name}) = ${name} { - try!(${name}.to_css(dest)); - try!(write!(dest, " ")); - } + try!(${name}.to_css(dest)); + try!(write!(dest, " ")); % endfor - if let Some(name) = name { - try!(name.to_css(dest)); - } + try!(name.to_css(dest)); } Ok(()) } diff --git a/tests/unit/style/properties/serialization.rs b/tests/unit/style/properties/serialization.rs index e54ef253059..2b0085ccf13 100644 --- a/tests/unit/style/properties/serialization.rs +++ b/tests/unit/style/properties/serialization.rs @@ -1192,27 +1192,25 @@ mod shorthand_serialization { #[test] fn serialize_multiple_animations_unequal_property_lists() { - // Currently the implementation cycles values if the lists are - // uneven. This is incorrect, in that we should serialize only - // when the lists have the same length (both here and for background - // and transition. See https://github.com/servo/servo/issues/15398 ) - let block = property_declaration_block("\ - animation-name: bounce, roll, flip, jump;\ - animation-duration: 1s, 0.2s;\ - animation-timing-function: ease-in, linear;\ - animation-delay: 0s, 1s, 0.5s;\ - animation-direction: normal;\ - animation-fill-mode: forwards, backwards;\ - animation-iteration-count: infinite, 2;\ - animation-play-state: paused, running;"); + // When the lengths of property values are different, the shorthand serialization + // should not be used. Previously the implementation cycled values if the lists were + // uneven. This is incorrect, in that we should serialize to a shorthand only when the + // lists have the same length (both here and for background and transition. See + // https://github.com/servo/servo/issues/15398 ) + let block_text = "\ + animation-name: bounce, roll, flip, jump; \ + animation-duration: 1s, 0.2s; \ + animation-timing-function: ease-in, linear; \ + animation-delay: 0s, 1s, 0.5s; \ + animation-direction: normal; \ + animation-fill-mode: forwards, backwards; \ + animation-iteration-count: infinite, 2; \ + animation-play-state: paused, running;"; + let block = property_declaration_block(block_text); let serialization = block.to_css_string(); - assert_eq!(serialization, "animation: \ - 1s ease-in 0s normal forwards infinite paused bounce, \ - 0.2s linear 1s normal backwards 2 running roll, \ - 1s ease-in 0.5s normal forwards infinite paused flip, \ - 0.2s linear 0s normal backwards 2 running jump;") + assert_eq!(serialization, block_text); } #[test]