style: Refactor some animations code

This change modifies the names of some methods to make it clearer what
they are doing. It also adds some clarifying comments to explain some
confusing behavior.

components/style/animation.rs

@@ -328,8 +328,8 @@ impl PropertyAnimation {
 
         let property_animation = PropertyAnimation {
             property: animated_property,
-            timing_function: timing_function,
+            timing_function,
-            duration: duration,
+            duration,
         };
 
         if property_animation.does_animate() {
@@ -411,7 +411,7 @@ pub fn start_transitions_if_applicable(
     old_style: &ComputedValues,
     new_style: &mut Arc<ComputedValues>,
     timer: &Timer,
-    possibly_expired_animations: &[PropertyAnimation],
+    running_and_expired_transitions: &[PropertyAnimation],
 ) -> bool {
     let mut had_animations = false;
     for i in 0..new_style.get_box().transition_property_count() {
@@ -425,17 +425,16 @@ pub fn start_transitions_if_applicable(
             // above.
             property_animation.update(Arc::get_mut(new_style).unwrap(), 0.0);
 
-            debug!(
-                "checking {:?} for matching end value",
-                possibly_expired_animations
-            );
-
             // Per [1], don't trigger a new transition if the end state for that
             // transition is the same as that of a transition that's already
             // running on the same node.
             //
             // [1]: https://drafts.csswg.org/css-transitions/#starting
-            if possibly_expired_animations
+            debug!(
+                "checking {:?} for matching end value",
+                running_and_expired_transitions
+            );
+            if running_and_expired_transitions
                 .iter()
                 .any(|animation| animation.has_the_same_end_value_as(&property_animation))
             {
@@ -447,9 +446,8 @@ pub fn start_transitions_if_applicable(
                 continue;
             }
 
-            debug!("Kicking off transition of {:?}", property_animation);
-
             // Kick off the animation.
+            debug!("Kicking off transition of {:?}", property_animation);
             let box_style = new_style.get_box();
             let now = timer.seconds();
             let start_time = now + (box_style.transition_delay_mod(i).seconds() as f64);
@@ -850,25 +848,3 @@ where
         },
     }
 }
-
-/// Update the style in the node when it finishes.
-#[cfg(feature = "servo")]
-pub fn complete_expired_transitions(
-    node: OpaqueNode,
-    style: &mut Arc<ComputedValues>,
-    context: &SharedStyleContext,
-    expired_animations: &mut Vec<crate::animation::PropertyAnimation>,
-) {
-    let mut all_expired_animations = context.expired_animations.write();
-    if let Some(animations) = all_expired_animations.remove(&node) {
-        debug!("removing expired animations for {:?}", node);
-        for animation in animations {
-            debug!("Updating expired animation {:?}", animation);
-            // TODO: support animation-fill-mode
-            if let Animation::Transition(_, _, frame) = animation {
-                frame.property_animation.update(Arc::make_mut(style), 1.0);
-                expired_animations.push(frame.property_animation);
-            }
-        }
-    }
-}

components/style/matching.rs

@@ -12,6 +12,8 @@ use crate::context::{ElementCascadeInputs, QuirksMode, SelectorFlagsMap};
 use crate::context::{SharedStyleContext, StyleContext};
 use crate::data::ElementData;
 use crate::dom::TElement;
+#[cfg(feature = "servo")]
+use crate::dom::{OpaqueNode, TNode};
 use crate::invalidation::element::restyle_hints::RestyleHint;
 use crate::properties::longhands::display::computed_value::T as Display;
 use crate::properties::ComputedValues;
@@ -436,22 +438,35 @@ trait PrivateMatchMethods: TElement {
         _important_rules_changed: bool,
     ) {
         use crate::animation;
-        use crate::dom::TNode;
 
-        let mut possibly_expired_animations = vec![];
+        let this_opaque = self.as_node().opaque();
+        let mut running_and_expired_transitions = vec![];
         let shared_context = context.shared;
-        if let Some(ref mut old) = *old_values {
+        if let Some(ref mut old_values) = *old_values {
-            // FIXME(emilio, #20116): This makes no sense.
+            // We apply the expired transitions and animations to the old style
-            self.update_animations_for_cascade(
+            // here, because we later compare the old style to the new style in
+            // `start_transitions_if_applicable`. If the styles differ then it will
+            // cause the expired transition to restart.
+            //
+            // TODO(mrobinson): We should really be following spec behavior and calculate
+            // after-change-style and before-change-style here.
+            Self::collect_and_update_style_for_expired_transitions(
                 shared_context,
-                old,
+                this_opaque,
-                &mut possibly_expired_animations,
+                old_values,
+                &mut running_and_expired_transitions,
+            );
+
+            Self::update_style_for_animations_and_collect_running_transitions(
+                shared_context,
+                this_opaque,
+                old_values,
+                &mut running_and_expired_transitions,
                 &context.thread_local.font_metrics_provider,
             );
         }
 
         let new_animations_sender = &context.thread_local.new_animations_sender;
-        let this_opaque = self.as_node().opaque();
         // Trigger any present animations if necessary.
         animation::maybe_start_animations(
             *self,
@@ -461,16 +476,16 @@ trait PrivateMatchMethods: TElement {
             &new_values,
         );
 
-        // Trigger transitions if necessary. This will reset `new_values` back
+        // Trigger transitions if necessary. This will set `new_values` to
-        // to its old value if it did trigger a transition.
+        // the starting value of the transition if it did trigger a transition.
-        if let Some(ref values) = *old_values {
+        if let Some(ref values) = old_values {
             animation::start_transitions_if_applicable(
                 new_animations_sender,
                 this_opaque,
                 &values,
                 new_values,
                 &shared_context.timer,
-                &possibly_expired_animations,
+                &running_and_expired_transitions,
             );
         }
     }
@@ -588,46 +603,48 @@ trait PrivateMatchMethods: TElement {
         ChildCascadeRequirement::MustCascadeChildrenIfInheritResetStyle
     }
 
-    // FIXME(emilio, #20116): It's not clear to me that the name of this method
-    // represents anything of what it does.
-    //
-    // Also, this function gets the old style, for some reason I don't really
-    // get, but the functions called (mainly update_style_for_animation) expects
-    // the new style, wtf?
     #[cfg(feature = "servo")]
-    fn update_animations_for_cascade(
+    fn collect_and_update_style_for_expired_transitions(
-        &self,
         context: &SharedStyleContext,
+        node: OpaqueNode,
         style: &mut Arc<ComputedValues>,
-        possibly_expired_animations: &mut Vec<crate::animation::PropertyAnimation>,
+        expired_transitions: &mut Vec<crate::animation::PropertyAnimation>,
+    ) {
+        use crate::animation::Animation;
+
+        let mut all_expired_animations = context.expired_animations.write();
+        if let Some(animations) = all_expired_animations.remove(&node) {
+            debug!("removing expired animations for {:?}", node);
+            for animation in animations {
+                debug!("Updating expired animation {:?}", animation);
+                // TODO: support animation-fill-mode
+                if let Animation::Transition(_, _, frame) = animation {
+                    frame.property_animation.update(Arc::make_mut(style), 1.0);
+                    expired_transitions.push(frame.property_animation);
+                }
+            }
+        }
+    }
+
+    #[cfg(feature = "servo")]
+    fn update_style_for_animations_and_collect_running_transitions(
+        context: &SharedStyleContext,
+        node: OpaqueNode,
+        style: &mut Arc<ComputedValues>,
+        running_transitions: &mut Vec<crate::animation::PropertyAnimation>,
         font_metrics: &dyn crate::font_metrics::FontMetricsProvider,
     ) {
         use crate::animation::{self, Animation, AnimationUpdate};
-        use crate::dom::TNode;
 
-        // Finish any expired transitions.
+        let had_running_animations = context.running_animations.read().get(&node).is_some();
-        let this_opaque = self.as_node().opaque();
-        animation::complete_expired_transitions(
-            this_opaque,
-            style,
-            context,
-            possibly_expired_animations,
-        );
-
-        // Merge any running animations into the current style, and cancel them.
-        let had_running_animations = context
-            .running_animations
-            .read()
-            .get(&this_opaque)
-            .is_some();
         if !had_running_animations {
             return;
         }
 
         let mut all_running_animations = context.running_animations.write();
-        for mut running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
+        for mut running_animation in all_running_animations.get_mut(&node).unwrap() {
             if let Animation::Transition(_, _, ref frame) = *running_animation {
-                possibly_expired_animations.push(frame.property_animation.clone());
+                running_transitions.push(frame.property_animation.clone());
                 continue;
             }