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).

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::<W, Cloned<slice::Iter< &PropertyDeclaration>>, _>(
+                    try!(append_serialization::<W, Cloned<slice::Iter< _>>, _>(
                              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<Item=&'a PropertyDeclaration>,
 {
     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));

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,

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
+            let len = extract_value(self.animation_name).map(|i| i.0.len()).unwrap_or(0);
-            // 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
-
             // 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!(${name}.to_css(dest));
+                try!(write!(dest, " "));
-                    try!(write!(dest, " "));
-                }
             % endfor
 
-                if let Some(name) = name {
+                try!(name.to_css(dest));
-                    try!(name.to_css(dest));
-                }
             }
             Ok(())
         }

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
+            // When the lengths of property values are different, the shorthand serialization
-            // uneven. This is incorrect, in that we should serialize only
+            // should not be used. Previously the implementation cycled values if the lists were
-            // when the lists have the same length (both here and for background
+            // uneven. This is incorrect, in that we should serialize to a shorthand only when the
-            // and transition. See https://github.com/servo/servo/issues/15398 )
+            // lists have the same length (both here and for background and transition. See
-            let block = property_declaration_block("\
+            // https://github.com/servo/servo/issues/15398 )
-                animation-name: bounce, roll, flip, jump;\
+            let block_text = "\
-                animation-duration: 1s, 0.2s;\
+                animation-name: bounce, roll, flip, jump; \
-                animation-timing-function: ease-in, linear;\
+                animation-duration: 1s, 0.2s; \
-                animation-delay: 0s, 1s, 0.5s;\
+                animation-timing-function: ease-in, linear; \
-                animation-direction: normal;\
+                animation-delay: 0s, 1s, 0.5s; \
-                animation-fill-mode: forwards, backwards;\
+                animation-direction: normal; \
-                animation-iteration-count: infinite, 2;\
+                animation-fill-mode: forwards, backwards; \
-                animation-play-state: paused, running;");
+                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: \
+            assert_eq!(serialization, block_text);
-                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;")
         }
 
         #[test]