Auto merge of #18572 - hiikezoe:immportant-in-keyframe, r=emilio

Handle !important in keyframe

<!-- Please describe your changes on the following line: -->
https://bugzilla.mozilla.org/show_bug.cgi?id=1400926

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

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

components/style/animation.rs

@@ -481,11 +481,13 @@ fn compute_style_for_animation_step(context: &SharedStyleContext,
         KeyframesStepValue::Declarations { block: ref declarations } => {
             let guard = declarations.read_with(context.guards.author);
 
-            // No !important in keyframes.
-            debug_assert!(!guard.any_important());
-
             let iter = || {
-                guard.declarations().iter().rev()
+                // It's possible to have !important properties in keyframes
+                // so we have to filter them out.
+                // See the spec issue https://github.com/w3c/csswg-drafts/issues/1824
+                // Also we filter our non-animatable properties.
+                guard.normal_declaration_iter()
+                     .filter(|declaration| declaration.is_animatable())
                      .map(|decl| (decl, CascadeLevel::Animations))
             };
 

components/style/properties/declaration_block.rs

@@ -112,6 +112,40 @@ impl<'a> DoubleEndedIterator for DeclarationImportanceIterator<'a> {
     }
 }
 
+/// Iterator over `PropertyDeclaration` for Importance::Normal.
+pub struct NormalDeclarationIterator<'a>(DeclarationImportanceIterator<'a>);
+
+impl<'a> NormalDeclarationIterator<'a> {
+    /// Constructor
+    pub fn new(declarations: &'a [PropertyDeclaration], important: &'a SmallBitVec) -> Self {
+        NormalDeclarationIterator(
+            DeclarationImportanceIterator::new(declarations, important)
+        )
+    }
+}
+
+impl<'a> Iterator for NormalDeclarationIterator<'a> {
+    type Item = &'a PropertyDeclaration;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        loop {
+            let next = self.0.iter.next();
+            match next {
+                Some((decl, importance)) => {
+                    if !importance {
+                        return Some(decl);
+                    }
+                },
+                None => return None,
+            }
+        }
+    }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        self.0.iter.size_hint()
+    }
+}
+
 /// Iterator for AnimationValue to be generated from PropertyDeclarationBlock.
 pub struct AnimationValueIterator<'a, 'cx, 'cx_a:'cx> {
     iter: DeclarationImportanceIterator<'a>,
@@ -208,6 +242,11 @@ impl PropertyDeclarationBlock {
         DeclarationImportanceIterator::new(&self.declarations, &self.declarations_importance)
     }
 
+    /// Iterate over `PropertyDeclaration` for Importance::Normal
+    pub fn normal_declaration_iter(&self) -> NormalDeclarationIterator {
+        NormalDeclarationIterator::new(&self.declarations, &self.declarations_importance)
+    }
+
     /// Return an iterator of (AnimatableLonghand, AnimationValue).
     pub fn to_animation_value_iter<'a, 'cx, 'cx_a:'cx>(&'a self,
                                                        context: &'cx mut Context<'cx_a>,

components/style/stylesheets/keyframes_rule.rs

@@ -375,9 +375,13 @@ fn get_animated_properties(keyframes: &[Arc<Locked<Keyframe>>], guard: &SharedRw
     for keyframe in keyframes {
         let keyframe = keyframe.read_with(&guard);
         let block = keyframe.block.read_with(guard);
-        for (declaration, importance) in block.declaration_importance_iter() {
+        // CSS Animations spec clearly defines that properties with !important
-            assert!(!importance.important());
+        // in keyframe rules are invalid and ignored, but it's still ambiguous
-
+        // whether we should drop the !important properties or retain the
+        // properties when they are set via CSSOM. So we assume there might
+        // be properties with !important in keyframe rules here.
+        // See the spec issue https://github.com/w3c/csswg-drafts/issues/1824
+        for declaration in block.normal_declaration_iter() {
             if let Some(property) = AnimatableLonghand::from_declaration(declaration) {
                 // Skip the 'display' property because although it is animatable from SMIL,
                 // it should not be animatable from CSS Animations or Web Animations.

ports/geckolib/glue.rs

@@ -3609,10 +3609,9 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(raw_data: RawServoStyleSetB
             },
             KeyframesStepValue::Declarations { ref block } => {
                 let guard = block.read_with(&guard);
-                // Filter out non-animatable properties.
+                // Filter out non-animatable properties and properties with !important.
                 let animatable =
-                    guard.declarations()
+                    guard.normal_declaration_iter()
-                         .iter()
                          .filter(|declaration| declaration.is_animatable());
 
                 for declaration in animatable {