Auto merge of #15683 - hiikezoe:fill-missing-property-in-keyframe, r=emilio

Fill missing property in keyframe

<!-- Please describe your changes on the following line: -->
This is a PR of https://bugzilla.mozilla.org/show_bug.cgi?id=1340961
All patches has been reviewed by @emilio.

Thanks!

---
<!-- 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
- [ ] These changes fix #__ (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/15683)
<!-- Reviewable:end -->

components/style/keyframes.rs

@@ -15,6 +15,7 @@ use properties::{PropertyDeclarationId, LonghandId, DeclaredValue};
 use properties::PropertyDeclarationParseResult;
 use properties::animated_properties::TransitionProperty;
 use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
+use properties::property_bit_field::PropertyBitField;
 use std::fmt;
 use std::sync::Arc;
 use style_traits::ToCss;
@@ -244,21 +245,23 @@ pub struct KeyframesAnimation {
     pub properties_changed: Vec<TransitionProperty>,
 }
 
-/// Get all the animated properties in a keyframes animation. Note that it's not
-/// defined what happens when a property is not on a keyframe, so we only peek
-/// the props of the first one.
-///
-/// In practice, browsers seem to try to do their best job at it, so we might
-/// want to go through all the actual keyframes and deduplicate properties.
-fn get_animated_properties(keyframe: &Keyframe) -> Vec<TransitionProperty> {
+/// Get all the animated properties in a keyframes animation.
+fn get_animated_properties(keyframes: &[Arc<RwLock<Keyframe>>]) -> Vec<TransitionProperty> {
     let mut ret = vec![];
+    let mut seen = PropertyBitField::new();
     // NB: declarations are already deduplicated, so we don't have to check for
     // it here.
-    for &(ref declaration, importance) in keyframe.block.read().declarations.iter() {
-        assert!(!importance.important());
+    for keyframe in keyframes {
+        let keyframe = keyframe.read();
+        for &(ref declaration, importance) in keyframe.block.read().declarations.iter() {
+            assert!(!importance.important());
 
-        if let Some(property) = TransitionProperty::from_declaration(declaration) {
-            ret.push(property);
+            if let Some(property) = TransitionProperty::from_declaration(declaration) {
+                if !seen.has_transition_property_bit(&property) {
+                    ret.push(property);
+                    seen.set_transition_property_bit(&property);
+                }
+            }
         }
     }
 
@@ -284,7 +287,7 @@ impl KeyframesAnimation {
             return result;
         }
 
-        result.properties_changed = get_animated_properties(&keyframes[0].read());
+        result.properties_changed = get_animated_properties(keyframes);
         if result.properties_changed.is_empty() {
             return result;
         }

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

@@ -43,7 +43,7 @@ use values::specified::Angle as SpecifiedAngle;
 /// property.
 // NB: This needs to be here because it needs all the longhands generated
 // beforehand.
-#[derive(Copy, Clone, Debug, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub enum TransitionProperty {
     /// All, any animatable property changing should generate a transition.

components/style/properties/properties.mako.rs

@@ -181,8 +181,10 @@ pub mod animated_properties {
 
 // TODO(SimonSapin): Convert this to a syntax extension rather than a Mako template.
 // Maybe submit for inclusion in libstd?
-mod property_bit_field {
+#[allow(missing_docs)]
+pub mod property_bit_field {
     use logical_geometry::WritingMode;
+    use properties::animated_properties::TransitionProperty;
 
     /// A bitfield for all longhand properties, in order to quickly test whether
     /// we've seen one of them.
@@ -213,7 +215,7 @@ mod property_bit_field {
                 pub fn get_${property.ident}(&self) -> bool {
                     self.get(${i})
                 }
-                #[allow(non_snake_case)]
+                #[allow(non_snake_case, missing_docs)]
                 #[inline]
                 pub fn set_${property.ident}(&mut self) {
                     self.set(${i})
@@ -238,6 +240,32 @@ mod property_bit_field {
                 }
             % endif
         % endfor
+
+        /// Set the corresponding bit of TransitionProperty.
+        /// This function will panic if TransitionProperty::All is given.
+        pub fn set_transition_property_bit(&mut self, property: &TransitionProperty) {
+            match *property {
+                % for i, prop in enumerate(data.longhands):
+                    % if prop.animatable:
+                        TransitionProperty::${prop.camel_case} => self.set(${i}),
+                    % endif
+                % endfor
+                TransitionProperty::All => unreachable!("Tried to set TransitionProperty::All in a PropertyBitfield"),
+            }
+        }
+
+        /// Return true if the corresponding bit of TransitionProperty is set.
+        /// This function will panic if TransitionProperty::All is given.
+        pub fn has_transition_property_bit(&self, property: &TransitionProperty) -> bool {
+            match *property {
+                % for i, prop in enumerate(data.longhands):
+                    % if prop.animatable:
+                        TransitionProperty::${prop.camel_case} => self.get(${i}),
+                    % endif
+                % endfor
+                TransitionProperty::All => unreachable!("Tried to get TransitionProperty::All in a PropertyBitfield"),
+            }
+        }
     }
 }
 

ports/geckolib/glue.rs

@@ -1435,6 +1435,9 @@ pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSet
                                                       timing_function: *const nsTimingFunction,
                                                       style: ServoComputedValuesBorrowed,
                                                       keyframes: RawGeckoKeyframeListBorrowedMut) -> bool {
+    use style::gecko_bindings::structs::Keyframe;
+    use style::properties::property_bit_field::PropertyBitField;
+
     let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
     let name = unsafe { Atom::from(name.as_ref().unwrap().as_str_unchecked()) };
     let style_timing_function = unsafe { timing_function.as_ref().unwrap() };
@@ -1455,16 +1458,25 @@ pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSet
                                               &timing_function)
           };
 
+          fn add_computed_property_value(keyframe: *mut Keyframe,
+                                         index: usize,
+                                         style: &ComputedValues,
+                                         property: &TransitionProperty) {
+              let block = style.to_declaration_block(property.clone().into());
+              unsafe {
+                  (*keyframe).mPropertyValues.set_len((index + 1) as u32);
+                  (*keyframe).mPropertyValues[index].mProperty = property.clone().into();
+                  // FIXME. Do not set computed values once we handles missing keyframes
+                  // with additive composition.
+                  (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
+                      Arc::new(RwLock::new(block)));
+              }
+          }
+
           match step.value {
               KeyframesStepValue::ComputedValues => {
                   for (index, property) in animation.properties_changed.iter().enumerate() {
-                      let block = style.to_declaration_block(property.clone().into());
-                      unsafe {
-                          (*keyframe).mPropertyValues.set_len((index + 1) as u32);
-                          (*keyframe).mPropertyValues[index].mProperty = property.clone().into();
-                          (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
-                              Arc::new(RwLock::new(block)));
-                      }
+                      add_computed_property_value(keyframe, index, style, property);
                   }
               },
               KeyframesStepValue::Declarations { ref block } => {
@@ -1476,16 +1488,33 @@ pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSet
                            .filter(|&&(ref declaration, _)| {
                                declaration.is_animatable()
                            });
+
+                  let mut seen = PropertyBitField::new();
+
                   for (index, &(ref declaration, _)) in animatable.enumerate() {
                       unsafe {
+                          let property = TransitionProperty::from_declaration(declaration).unwrap();
                           (*keyframe).mPropertyValues.set_len((index + 1) as u32);
-                          (*keyframe).mPropertyValues[index].mProperty =
-                              TransitionProperty::from_declaration(declaration).unwrap().into();
+                          (*keyframe).mPropertyValues[index].mProperty = property.into();
                           (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
                               Arc::new(RwLock::new(
                                   PropertyDeclarationBlock { declarations: vec![ (declaration.clone(),
                                                                                   Importance::Normal) ],
                                                              important_count: 0 })));
+                          if step.start_percentage.0 == 0. ||
+                             step.start_percentage.0 == 1. {
+                              seen.set_transition_property_bit(&property);
+                          }
+                      }
+                  }
+
+                  // Append missing property values in the initial or the finial keyframes.
+                  if step.start_percentage.0 == 0. ||
+                     step.start_percentage.0 == 1. {
+                      for (index, property) in animation.properties_changed.iter().enumerate() {
+                          if !seen.has_transition_property_bit(&property) {
+                              add_computed_property_value(keyframe, index, style, property);
+                          }
                       }
                   }
               },

tests/unit/style/keyframes.rs

@@ -5,7 +5,10 @@
 use parking_lot::RwLock;
 use std::sync::Arc;
 use style::keyframes::{Keyframe, KeyframesAnimation, KeyframePercentage,  KeyframeSelector};
-use style::properties::PropertyDeclarationBlock;
+use style::keyframes::{KeyframesStep, KeyframesStepValue};
+use style::properties::{DeclaredValue, PropertyDeclaration, PropertyDeclarationBlock, Importance};
+use style::properties::animated_properties::TransitionProperty;
+use style::values::specified::{LengthOrPercentageOrAuto, NoCalcLength};
 
 #[test]
 fn test_empty_keyframe() {
@@ -38,3 +41,177 @@ fn test_no_property_in_keyframe() {
 
     assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
 }
+
+#[test]
+fn test_missing_property_in_initial_keyframe() {
+    let declarations_on_initial_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let declarations_on_final_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+
+                (PropertyDeclaration::Height(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let keyframes = vec![
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
+            block: declarations_on_initial_keyframe.clone(),
+        })),
+
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
+            block: declarations_on_final_keyframe.clone(),
+        })),
+    ];
+    let animation = KeyframesAnimation::from_keyframes(&keyframes);
+    let expected = KeyframesAnimation {
+        steps: vec![
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_initial_keyframe },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(1.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_final_keyframe },
+                declared_timing_function: false,
+            },
+        ],
+        properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height],
+    };
+
+    assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
+}
+
+#[test]
+fn test_missing_property_in_final_keyframe() {
+    let declarations_on_initial_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+
+                (PropertyDeclaration::Height(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let declarations_on_final_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Height(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let keyframes = vec![
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
+            block: declarations_on_initial_keyframe.clone(),
+        })),
+
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
+            block: declarations_on_final_keyframe.clone(),
+        })),
+    ];
+    let animation = KeyframesAnimation::from_keyframes(&keyframes);
+    let expected = KeyframesAnimation {
+        steps: vec![
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_initial_keyframe },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(1.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_final_keyframe },
+                declared_timing_function: false,
+            },
+        ],
+        properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height],
+    };
+
+    assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
+}
+
+#[test]
+fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() {
+    let declarations =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                        DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+
+                (PropertyDeclaration::Height(
+                        DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let keyframes = vec![
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
+            block: Arc::new(RwLock::new(PropertyDeclarationBlock {
+                declarations: vec![],
+                important_count: 0,
+            }))
+        })),
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.5)]),
+            block: declarations.clone(),
+        })),
+    ];
+    let animation = KeyframesAnimation::from_keyframes(&keyframes);
+    let expected = KeyframesAnimation {
+        steps: vec![
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.),
+                value: KeyframesStepValue::Declarations {
+                    block: Arc::new(RwLock::new(PropertyDeclarationBlock {
+                        // XXX: Should we use ComputedValues in this case?
+                        declarations: vec![],
+                        important_count: 0,
+                    }))
+                },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.5),
+                value: KeyframesStepValue::Declarations { block: declarations },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(1.),
+                value: KeyframesStepValue::ComputedValues,
+                declared_timing_function: false,
+            }
+        ],
+        properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height],
+    };
+
+    assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
+}