Auto merge of #16527 - BorisChiou:stylo/transition/shorthands, r=emilio

stylo: Bug 1353628 - Support shorthand properties for CSS Transition.

These patches add shorthand properties into TransitionProperty, so we can parse the shorthand properties properly and create transitions for them.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix Bug 1353628
- [X] There are tests for these changes

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

components/style/animation.rs

@@ -272,6 +272,15 @@ impl PropertyAnimation {
         let timing_function = box_style.transition_timing_function_mod(transition_index);
         let duration = box_style.transition_duration_mod(transition_index);
 
+        if transition_property.is_shorthand() {
+            return transition_property.longhands().iter().filter_map(|transition_property| {
+                PropertyAnimation::from_transition_property(*transition_property,
+                                                            timing_function,
+                                                            duration,
+                                                            old_style,
+                                                            new_style)
+            }).collect();
+        }
 
         if transition_property != TransitionProperty::All {
             if let Some(property_animation) =
@@ -305,6 +314,8 @@ impl PropertyAnimation {
                                 old_style: &ComputedValues,
                                 new_style: &ComputedValues)
                                 -> Option<PropertyAnimation> {
+        debug_assert!(!transition_property.is_shorthand() &&
+                      transition_property != TransitionProperty::All);
         let animated_property = AnimatedProperty::from_transition_property(&transition_property,
                                                                            old_style,
                                                                            new_style);

components/style/gecko/wrapper.rs

@@ -775,7 +775,7 @@ impl<'le> TElement for GeckoElement<'le> {
                                 after_change_style: &Arc<ComputedValues>,
                                 pseudo: Option<&PseudoElement>) -> bool {
         use gecko_bindings::structs::nsCSSPropertyID;
-        use properties::animated_properties;
+        use properties::{PropertyId, animated_properties};
         use std::collections::HashSet;
 
         debug_assert!(self.might_need_transitions_update(&Some(before_change_style),
@@ -825,16 +825,24 @@ impl<'le> TElement for GeckoElement<'le> {
                 }
                 false
             };
-            // FIXME: Bug 1353628: Shorthand properties are parsed failed now, so after fixing
-            //        that, we have to handle shorthand.
             if property == nsCSSPropertyID::eCSSPropertyExtra_all_properties {
                 if TransitionProperty::any(property_check_helper) {
                     return true;
                 }
             } else {
-                if animated_properties::nscsspropertyid_is_animatable(property) &&
-                   property_check_helper(property.into()) {
-                    return true;
+                let is_shorthand = PropertyId::from_nscsspropertyid(property).ok().map_or(false, |p| {
+                        p.as_shorthand().is_ok()
+                });
+                if is_shorthand {
+                    let shorthand: TransitionProperty = property.into();
+                    if shorthand.longhands().iter().any(|&p| property_check_helper(p)) {
+                        return true;
+                    }
+                } else {
+                    if animated_properties::nscsspropertyid_is_animatable(property) &&
+                       property_check_helper(property.into()) {
+                        return true;
+                    }
                 }
             }
         }

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

@@ -54,10 +54,16 @@ pub enum TransitionProperty {
             ${prop.camel_case},
         % endif
     % endfor
+    // Shorthand properties may or may not contain any animatable property. Either should still be
+    // parsed properly.
+    % for prop in data.shorthands_except_all():
+        /// ${prop.name}
+        ${prop.camel_case},
+    % endfor
 }
 
 impl TransitionProperty {
-    /// Iterates over each property that is not `All`.
+    /// Iterates over each longhand property.
     pub fn each<F: FnMut(TransitionProperty) -> ()>(mut cb: F) {
         % for prop in data.longhands:
             % if prop.animatable:
@@ -88,6 +94,9 @@ impl TransitionProperty {
                     "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}),
                 % endif
             % endfor
+            % for prop in data.shorthands_except_all():
+                "${prop.name}" => Ok(TransitionProperty::${prop.camel_case}),
+            % endfor
             _ => Err(())
         }
     }
@@ -118,7 +127,8 @@ impl TransitionProperty {
         }
     }
 
-    /// Returns true if this TransitionProperty is one of the discrete animatable properties.
+    /// Returns true if this TransitionProperty is one of the discrete animatable properties and
+    /// this TransitionProperty should be a longhand property.
     pub fn is_discrete(&self) -> bool {
         match *self {
             % for prop in data.longhands:
@@ -129,6 +139,35 @@ impl TransitionProperty {
             _ => false
         }
     }
+
+    /// Return animatable longhands of this shorthand TransitionProperty, except for "all".
+    pub fn longhands(&self) -> &'static [TransitionProperty] {
+        % for prop in data.shorthands_except_all():
+            static ${prop.ident.upper()}: &'static [TransitionProperty] = &[
+                % for sub in prop.sub_properties:
+                    % if sub.animatable:
+                        TransitionProperty::${sub.camel_case},
+                    % endif
+                % endfor
+            ];
+        % endfor
+        match *self {
+            % for prop in data.shorthands_except_all():
+                TransitionProperty::${prop.camel_case} => ${prop.ident.upper()},
+            % endfor
+            _ => panic!("Not allowed to call longhands() for this TransitionProperty")
+        }
+    }
+
+    /// Returns true if this TransitionProperty is a shorthand.
+    pub fn is_shorthand(&self) -> bool {
+        match *self {
+            % for prop in data.shorthands_except_all():
+                TransitionProperty::${prop.camel_case} => true,
+            % endfor
+            _ => false
+        }
+    }
 }
 
 /// Returns true if this nsCSSPropertyID is one of the animatable properties.
@@ -155,6 +194,9 @@ impl ToCss for TransitionProperty {
                     TransitionProperty::${prop.camel_case} => dest.write_str("${prop.name}"),
                 % endif
             % endfor
+            % for prop in data.shorthands_except_all():
+                TransitionProperty::${prop.camel_case} => dest.write_str("${prop.name}"),
+            % endfor
         }
     }
 }
@@ -171,6 +213,10 @@ impl From<TransitionProperty> for nsCSSPropertyID {
                         => ${helpers.to_nscsspropertyid(prop.ident)},
                 % endif
             % endfor
+            % for prop in data.shorthands_except_all():
+                TransitionProperty::${prop.camel_case}
+                    => ${helpers.to_nscsspropertyid(prop.ident)},
+            % endfor
             TransitionProperty::All => nsCSSPropertyID::eCSSPropertyExtra_all_properties,
         }
     }
@@ -188,6 +234,10 @@ impl From<nsCSSPropertyID> for TransitionProperty {
                         => TransitionProperty::${prop.camel_case},
                 % endif
             % endfor
+            % for prop in data.shorthands_except_all():
+                ${helpers.to_nscsspropertyid(prop.ident)}
+                    => TransitionProperty::${prop.camel_case},
+            % endfor
             nsCSSPropertyID::eCSSPropertyExtra_all_properties => TransitionProperty::All,
             _ => panic!("Unsupported Servo transition property: {:?}", property),
         }
@@ -206,7 +256,7 @@ impl<'a> From<TransitionProperty> for PropertyDeclarationId<'a> {
                         => PropertyDeclarationId::Longhand(LonghandId::${prop.camel_case}),
                 % endif
             % endfor
-            TransitionProperty::All => panic!(),
+            _ => panic!(),
         }
     }
 }
@@ -304,6 +354,7 @@ impl AnimatedProperty {
                     }
                 % endif
             % endfor
+            other => panic!("Can't use TransitionProperty::{:?} here", other),
         }
     }
 }
@@ -459,6 +510,7 @@ impl AnimationValue {
                     }
                 % endif
             % endfor
+            other => panic!("Can't use TransitionProperty::{:?} here.", other),
         }
     }
 }

components/style/properties/properties.mako.rs

@@ -257,7 +257,7 @@ impl LonghandIdSet {
                     TransitionProperty::${prop.camel_case} => self.insert(LonghandId::${prop.camel_case}),
                 % endif
             % endfor
-            TransitionProperty::All => unreachable!("Tried to set TransitionProperty::All in a PropertyBitfield"),
+            other => unreachable!("Tried to set TransitionProperty::{:?} in a PropertyBitfield", other),
         }
     }
 
@@ -270,7 +270,7 @@ impl LonghandIdSet {
                     TransitionProperty::${prop.camel_case} => self.contains(LonghandId::${prop.camel_case}),
                 % endif
             % endfor
-            TransitionProperty::All => unreachable!("Tried to get TransitionProperty::All in a PropertyBitfield"),
+            other => unreachable!("Tried to get TransitionProperty::{:?} in a PropertyBitfield", other),
         }
     }
 }

tests/unit/style/parsing/mod.rs

@@ -99,6 +99,7 @@ mod selectors;
 mod supports;
 mod text;
 mod text_overflow;
+mod transition_property;
 mod transition_timing_function;
 mod ui;
 mod value;

tests/unit/style/parsing/transition_property.rs

@@ -0,0 +1,67 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+use parsing::parse;
+use style::properties::animated_properties::TransitionProperty;
+use style::properties::longhands::transition_property;
+use style::properties::shorthands::transition;
+use style_traits::ToCss;
+
+#[test]
+fn test_longhand_properties() {
+    assert_roundtrip_with_context!(transition_property::parse, "margin-left");
+    assert_roundtrip_with_context!(transition_property::parse, "background-color");
+    assert_roundtrip_with_context!(transition_property::parse, "width");
+
+    assert_eq!(parse_longhand!(transition_property, "margin-left, width"),
+               transition_property::SpecifiedValue(
+                   vec![TransitionProperty::MarginLeft,
+                        TransitionProperty::Width]));
+
+    // TODO: If one of the identifiers listed is not a recognized property name or is not an
+    // animatable property, the implementation must still start transitions on the animatable
+    // properties. Therefore, the parser shouldn't return Err for non-animatable property.
+    assert!(parse(transition_property::parse, "transition-duration").is_err());
+}
+
+#[test]
+fn test_shorthand_properties() {
+    assert_roundtrip_with_context!(transition_property::parse, "margin");
+    assert_roundtrip_with_context!(transition_property::parse, "background");
+    assert_roundtrip_with_context!(transition_property::parse, "border-bottom");
+
+    assert_eq!(parse_longhand!(transition_property, "margin, background"),
+               transition_property::SpecifiedValue(
+                   vec![TransitionProperty::Margin,
+                        TransitionProperty::Background]));
+}
+
+#[test]
+fn test_keywords() {
+    assert_roundtrip_with_context!(transition_property::parse, "all");
+    assert_roundtrip_with_context!(transition_property::parse, "none");
+
+    assert_eq!(parse_longhand!(transition_property, "all"),
+               transition_property::SpecifiedValue(vec![TransitionProperty::All]));
+    assert_eq!(parse_longhand!(transition_property, "none"),
+               transition_property::SpecifiedValue(vec![]));
+
+    assert!(parse(transition_property::parse, "inherit").is_err());
+    assert!(parse(transition_property::parse, "initial").is_err());
+}
+
+#[test]
+fn test_transition_shorthand() {
+    let result = parse(transition::parse_value, "2s margin-left, 4s background").unwrap();
+    assert_eq!(result.transition_property,
+               parse_longhand!(transition_property, "margin-left, background"));
+
+    let result = parse(transition::parse_value, "2s margin, 4s all").unwrap();
+    assert_eq!(result.transition_property,
+               parse_longhand!(transition_property, "margin, all"));
+
+    let result = parse(transition::parse_value, "none").unwrap();
+    assert_eq!(result.transition_property,
+               parse_longhand!(transition_property, "none"));
+}

tests/wpt/metadata-css/css-transitions-1_dev/html/events-001.htm.ini

@@ -1,11 +0,0 @@
-[events-001.htm]
-  type: testharness
-  [transition:padding, changing padding-left / events]
-    expected: FAIL
-
-  [transition:padding, changing padding / events]
-    expected: FAIL
-
-  [transition:padding, changing padding but not padding-bottom / events]
-    expected: FAIL
-

tests/wpt/metadata-css/css-transitions-1_dev/html/transition-property-002.htm.ini

@@ -15,9 +15,6 @@
   [parse 'foobar, all']
     expected: FAIL
 
-  [parse 'inherit']
-    expected: FAIL
-
   [parse 'initial']
     expected: FAIL