Auto merge of #20757 - emilio:animations, r=jdm

style: Some animation cleanups and fixes.

The transitions code is still terribly broken, but I ran out of time fixing it. We have nothing that stops transitions, which is just plain wrong. Most of this code should probably be rewritten, since with the current setup is pretty hard to get it right. Anyway...

Fixes #20731.
Fixes #20116.

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

components/layout/animation.rs

@@ -79,7 +79,7 @@ pub fn update_animation_state<E>(
         let mut animations_still_running = vec![];
         for mut running_animation in running_animations.drain(..) {
             let still_running = !running_animation.is_expired() && match running_animation {
-                Animation::Transition(_, started_at, ref frame, _expired) => {
+                Animation::Transition(_, started_at, ref frame) => {
                     now < started_at + frame.duration
                 },
                 Animation::Keyframes(_, _, _, ref mut state) => {
@@ -89,18 +89,19 @@ pub fn update_animation_state<E>(
                 },
             };
 
+            debug!("update_animation_state({:?}): {:?}", still_running, running_animation);
+
             if still_running {
                 animations_still_running.push(running_animation);
                 continue;
             }
 
-            if let Animation::Transition(node, _, ref frame, _) = running_animation {
+            if let Animation::Transition(node, _, ref frame) = running_animation {
-                script_chan
+                script_chan.send(ConstellationControlMsg::TransitionEnd(
-                    .send(ConstellationControlMsg::TransitionEnd(
+                    node.to_untrusted_node_address(),
-                        node.to_untrusted_node_address(),
+                    frame.property_animation.property_name().into(),
-                        frame.property_animation.property_name().into(),
+                    frame.duration,
-                        frame.duration,
+                )).unwrap();
-                    )).unwrap();
             }
 
             expired_animations

components/style/animation.rs

@@ -214,9 +214,7 @@ pub enum Animation {
     /// A transition is just a single frame triggered at a time, with a reflow.
     ///
     /// the f64 field is the start time as returned by `time::precise_time_s()`.
-    ///
+    Transition(OpaqueNode, f64, AnimationFrame),
-    /// The `bool` field is werther this animation should no longer run.
-    Transition(OpaqueNode, f64, AnimationFrame, bool),
     /// A keyframes animation is identified by a name, and can have a
     /// node-dependent state (i.e. iteration count, etc.).
     ///
@@ -230,21 +228,11 @@ pub enum Animation {
 }
 
 impl Animation {
-    /// Mark this animation as expired.
-    #[inline]
-    pub fn mark_as_expired(&mut self) {
-        debug_assert!(!self.is_expired());
-        match *self {
-            Animation::Transition(_, _, _, ref mut expired) => *expired = true,
-            Animation::Keyframes(_, _, _, ref mut state) => state.expired = true,
-        }
-    }
-
     /// Whether this animation is expired.
     #[inline]
     pub fn is_expired(&self) -> bool {
         match *self {
-            Animation::Transition(_, _, _, expired) => expired,
+            Animation::Transition(..) => false,
             Animation::Keyframes(_, _, _, ref state) => state.expired,
         }
     }
@@ -253,7 +241,7 @@ impl Animation {
     #[inline]
     pub fn node(&self) -> &OpaqueNode {
         match *self {
-            Animation::Transition(ref node, _, _, _) => node,
+            Animation::Transition(ref node, _, _) => node,
             Animation::Keyframes(ref node, _, _, _) => node,
         }
     }
@@ -357,8 +345,8 @@ impl PropertyAnimation {
 
         let property_animation = PropertyAnimation {
             property: animated_property,
-            timing_function: timing_function,
+            timing_function,
-            duration: duration,
+            duration,
         };
 
         if property_animation.does_animate() {
@@ -450,9 +438,16 @@ pub fn start_transitions_if_applicable(
                 .iter()
                 .any(|animation| animation.has_the_same_end_value_as(&property_animation))
             {
+                debug!(
+                    "Not initiating transition for {}, other transition \
+                     found with the same end value",
+                    property_animation.property_name()
+                );
                 continue;
             }
 
+            debug!("Kicking off transition of {:?}", property_animation);
+
             // Kick off the animation.
             let box_style = new_style.get_box();
             let now = timer.seconds();
@@ -463,9 +458,8 @@ pub fn start_transitions_if_applicable(
                     start_time,
                     AnimationFrame {
                         duration: box_style.transition_duration_mod(i).seconds() as f64,
-                        property_animation: property_animation,
+                        property_animation,
                     },
-                    /* is_expired = */ false,
                 )).unwrap();
 
             had_animations = true;
@@ -544,10 +538,9 @@ where
 
     let box_style = new_style.get_box();
     for (i, name) in box_style.animation_name_iter().enumerate() {
-        let name = if let Some(atom) = name.as_atom() {
+        let name = match name.as_atom() {
-            atom
+            Some(atom) => atom,
-        } else {
+            None => continue,
-            continue;
         };
 
         debug!("maybe_start_animations: name={}", name);
@@ -556,61 +549,65 @@ where
             continue;
         }
 
-        if let Some(anim) = context.stylist.get_animation(name, element) {
+        let anim = match context.stylist.get_animation(name, element) {
-            debug!("maybe_start_animations: animation {} found", name);
+            Some(animation) => animation,
+            None => continue,
+        };
 
-            // If this animation doesn't have any keyframe, we can just continue
+        debug!("maybe_start_animations: animation {} found", name);
-            // without submitting it to the compositor, since both the first and
-            // the second keyframes would be synthetised from the computed
-            // values.
-            if anim.steps.is_empty() {
-                continue;
-            }
 
-            let delay = box_style.animation_delay_mod(i).seconds();
+        // If this animation doesn't have any keyframe, we can just continue
-            let now = context.timer.seconds();
+        // without submitting it to the compositor, since both the first and
-            let animation_start = now + delay as f64;
+        // the second keyframes would be synthetised from the computed
-            let duration = box_style.animation_duration_mod(i).seconds();
+        // values.
-            let iteration_state = match box_style.animation_iteration_count_mod(i) {
+        if anim.steps.is_empty() {
-                AnimationIterationCount::Infinite => KeyframesIterationState::Infinite,
+            continue;
-                AnimationIterationCount::Number(n) => KeyframesIterationState::Finite(0.0, n),
-            };
-
-            let animation_direction = box_style.animation_direction_mod(i);
-
-            let initial_direction = match animation_direction {
-                AnimationDirection::Normal | AnimationDirection::Alternate => {
-                    AnimationDirection::Normal
-                },
-                AnimationDirection::Reverse | AnimationDirection::AlternateReverse => {
-                    AnimationDirection::Reverse
-                },
-            };
-
-            let running_state = match box_style.animation_play_state_mod(i) {
-                AnimationPlayState::Paused => KeyframesRunningState::Paused(0.),
-                AnimationPlayState::Running => KeyframesRunningState::Running,
-            };
-
-            new_animations_sender
-                .send(Animation::Keyframes(
-                    node,
-                    anim.clone(),
-                    name.clone(),
-                    KeyframesAnimationState {
-                        started_at: animation_start,
-                        duration: duration as f64,
-                        delay: delay as f64,
-                        iteration_state: iteration_state,
-                        running_state: running_state,
-                        direction: animation_direction,
-                        current_direction: initial_direction,
-                        expired: false,
-                        cascade_style: new_style.clone(),
-                    },
-                )).unwrap();
-            had_animations = true;
         }
+
+        let delay = box_style.animation_delay_mod(i).seconds();
+        let now = context.timer.seconds();
+        let animation_start = now + delay as f64;
+        let duration = box_style.animation_duration_mod(i).seconds();
+        let iteration_state = match box_style.animation_iteration_count_mod(i) {
+            AnimationIterationCount::Infinite => KeyframesIterationState::Infinite,
+            AnimationIterationCount::Number(n) => KeyframesIterationState::Finite(0.0, n),
+        };
+
+        let animation_direction = box_style.animation_direction_mod(i);
+
+        let initial_direction = match animation_direction {
+            AnimationDirection::Normal | AnimationDirection::Alternate => {
+                AnimationDirection::Normal
+            },
+            AnimationDirection::Reverse | AnimationDirection::AlternateReverse => {
+                AnimationDirection::Reverse
+            },
+        };
+
+        let running_state = match box_style.animation_play_state_mod(i) {
+            AnimationPlayState::Paused => KeyframesRunningState::Paused(0.),
+            AnimationPlayState::Running => KeyframesRunningState::Running,
+        };
+
+        new_animations_sender
+            .send(Animation::Keyframes(
+                node,
+                anim.clone(),
+                name.clone(),
+                KeyframesAnimationState {
+                    started_at: animation_start,
+                    duration: duration as f64,
+                    delay: delay as f64,
+                    iteration_state,
+                    running_state,
+                    direction: animation_direction,
+                    current_direction: initial_direction,
+                    expired: false,
+                    cascade_style: new_style.clone(),
+                },
+            ))
+            .unwrap();
+        had_animations = true;
     }
 
     had_animations
@@ -640,21 +637,38 @@ pub fn update_style_for_animation_frame(
     true
 }
 
+/// Returns the kind of animation update that happened.
+pub enum AnimationUpdate {
+    /// The style was successfully updated, the animation is still running.
+    Regular,
+    /// A style change canceled this animation.
+    AnimationCanceled,
+}
+
 /// Updates a single animation and associated style based on the current time.
+///
+/// FIXME(emilio): This doesn't handle any kind of dynamic change to the
+/// animation or transition properties in any reasonable way.
+///
+/// This should probably be split in two, one from updating animations and
+/// transitions in response to a style change (that is,
+/// consider_starting_transitions + maybe_start_animations, but handling
+/// canceled animations, duration changes, etc, there instead of here), and this
+/// function should be only about the style update in response of a transition.
 pub fn update_style_for_animation<E>(
     context: &SharedStyleContext,
     animation: &Animation,
     style: &mut Arc<ComputedValues>,
     font_metrics_provider: &FontMetricsProvider,
-) where
+) -> AnimationUpdate
+where
     E: TElement,
 {
-    debug!("update_style_for_animation: entering");
+    debug!("update_style_for_animation: {:?}", animation);
     debug_assert!(!animation.is_expired());
 
     match *animation {
-        Animation::Transition(_, start_time, ref frame, _) => {
+        Animation::Transition(_, start_time, ref frame) => {
-            debug!("update_style_for_animation: transition found");
             let now = context.timer.seconds();
             let mut new_style = (*style).clone();
             let updated_style =
@@ -662,12 +676,14 @@ pub fn update_style_for_animation<E>(
             if updated_style {
                 *style = new_style
             }
+            // FIXME(emilio): Should check before updating the style that the
+            // transition_property still transitions this, or bail out if not.
+            //
+            // Or doing it in process_animations, only if transition_property
+            // changed somehow (even better).
+            AnimationUpdate::Regular
         },
         Animation::Keyframes(_, ref animation, ref name, ref state) => {
-            debug!(
-                "update_style_for_animation: animation found: \"{}\", {:?}",
-                name, state
-            );
             let duration = state.duration;
             let started_at = state.started_at;
 
@@ -685,38 +701,23 @@ pub fn update_style_for_animation<E>(
 
             let index = match maybe_index {
                 Some(index) => index,
-                None => {
+                None => return AnimationUpdate::AnimationCanceled,
-                    warn!(
-                        "update_style_for_animation: Animation {:?} not found in style",
-                        name
-                    );
-                    return;
-                },
             };
 
             let total_duration = style.get_box().animation_duration_mod(index).seconds() as f64;
             if total_duration == 0. {
-                debug!(
+                return AnimationUpdate::AnimationCanceled;
-                    "update_style_for_animation: zero duration for animation {:?}",
-                    name
-                );
-                return;
             }
 
             let mut total_progress = (now - started_at) / total_duration;
             if total_progress < 0. {
                 warn!("Negative progress found for animation {:?}", name);
-                return;
+                return AnimationUpdate::Regular;
             }
             if total_progress > 1. {
                 total_progress = 1.;
             }
 
-            debug!(
-                "update_style_for_animation: anim \"{}\", steps: {:?}, state: {:?}, progress: {}",
-                name, animation.steps, state, total_progress
-            );
-
             // Get the target and the last keyframe position.
             let last_keyframe_position;
             let target_keyframe_position;
@@ -758,11 +759,7 @@ pub fn update_style_for_animation<E>(
 
             let target_keyframe = match target_keyframe_position {
                 Some(target) => &animation.steps[target],
-                None => {
+                None => return AnimationUpdate::Regular,
-                    warn!("update_style_for_animation: No current keyframe found for animation \"{}\" at progress {}",
-                          name, total_progress);
-                    return;
-                },
             };
 
             let last_keyframe = &animation.steps[last_keyframe_position];
@@ -846,6 +843,7 @@ pub fn update_style_for_animation<E>(
                 name
             );
             *style = new_style;
+            AnimationUpdate::Regular
         },
     }
 }
@@ -864,8 +862,9 @@ pub fn complete_expired_transitions(
         had_animations_to_expire = animations_to_expire.is_some();
         if let Some(ref animations) = animations_to_expire {
             for animation in *animations {
+                debug!("Updating expired animation {:?}", animation);
                 // TODO: support animation-fill-mode
-                if let Animation::Transition(_, _, ref frame, _) = *animation {
+                if let Animation::Transition(_, _, ref frame) = *animation {
                     frame.property_animation.update(Arc::make_mut(style), 1.0);
                 }
             }

components/style/matching.rs

@@ -331,7 +331,7 @@ trait PrivateMatchMethods: TElement {
     fn process_animations(
         &self,
         context: &mut StyleContext<Self>,
-        old_values: &mut Option<Arc<ComputedValues>>,
+        old_values: Option<&Arc<ComputedValues>>,
         new_values: &mut Arc<ComputedValues>,
         restyle_hint: RestyleHint,
         important_rules_changed: bool,
@@ -413,7 +413,7 @@ trait PrivateMatchMethods: TElement {
     fn process_animations(
         &self,
         context: &mut StyleContext<Self>,
-        old_values: &mut Option<Arc<ComputedValues>>,
+        old_values: Option<&Arc<ComputedValues>>,
         new_values: &mut Arc<ComputedValues>,
         _restyle_hint: RestyleHint,
         _important_rules_changed: bool,
@@ -423,11 +423,10 @@ trait PrivateMatchMethods: TElement {
 
         let mut possibly_expired_animations = vec![];
         let shared_context = context.shared;
-        if let Some(ref mut old) = *old_values {
+        if old_values.is_some() {
-            // FIXME(emilio, #20116): This makes no sense.
             self.update_animations_for_cascade(
                 shared_context,
-                old,
+                new_values,
                 &mut possibly_expired_animations,
                 &context.thread_local.font_metrics_provider,
             );
@@ -446,7 +445,10 @@ trait PrivateMatchMethods: TElement {
 
         // Trigger transitions if necessary. This will reset `new_values` back
         // to its old value if it did trigger a transition.
-        if let Some(ref values) = *old_values {
+        //
+        // FIXME(emilio): We need logic to also stop the old transitions
+        // from running if transition-property / transition-duration changed.
+        if let Some(ref values) = old_values {
             animation::start_transitions_if_applicable(
                 new_animations_sender,
                 this_opaque,
@@ -573,10 +575,6 @@ trait PrivateMatchMethods: TElement {
 
     // 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(
         &self,
@@ -585,7 +583,7 @@ trait PrivateMatchMethods: TElement {
         possibly_expired_animations: &mut Vec<::animation::PropertyAnimation>,
         font_metrics: &::font_metrics::FontMetricsProvider,
     ) {
-        use animation::{self, Animation};
+        use animation::{self, Animation, AnimationUpdate};
         use dom::TNode;
 
         // Finish any expired transitions.
@@ -603,30 +601,29 @@ trait PrivateMatchMethods: TElement {
         }
 
         let mut all_running_animations = context.running_animations.write();
-        for running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
+        for mut running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
-            // This shouldn't happen frequently, but under some circumstances
+            if let Animation::Transition(_, _, ref frame) = *running_animation {
-            // mainly huge load or debug builds, the constellation might be
+                possibly_expired_animations.push(frame.property_animation.clone());
-            // delayed in sending the `TickAllAnimations` message to layout.
-            //
-            // Thus, we can't assume all the animations have been already
-            // updated by layout, because other restyle due to script might be
-            // triggered by layout before the animation tick.
-            //
-            // See #12171 and the associated PR for an example where this
-            // happened while debugging other release panic.
-            if running_animation.is_expired() {
                 continue;
             }
 
-            animation::update_style_for_animation::<Self>(
+            let update = animation::update_style_for_animation::<Self>(
                 context,
-                running_animation,
+                &mut running_animation,
                 style,
                 font_metrics,
             );
 
-            if let Animation::Transition(_, _, ref frame, _) = *running_animation {
+            match *running_animation {
-                possibly_expired_animations.push(frame.property_animation.clone())
+                Animation::Transition(..) => unreachable!(),
+                Animation::Keyframes(_, _, _, ref mut state) => {
+                    match update {
+                        AnimationUpdate::Regular => {},
+                        AnimationUpdate::AnimationCanceled => {
+                            state.expired = true;
+                        }
+                    }
+                }
             }
         }
     }
@@ -680,7 +677,7 @@ pub trait MatchMethods: TElement {
 
         self.process_animations(
             context,
-            &mut data.styles.primary,
+            data.styles.primary.as_ref(),
             &mut new_styles.primary.style.0,
             data.hint,
             important_rules_changed,

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

@@ -127,17 +127,22 @@ pub enum AnimatedProperty {
 }
 
 impl AnimatedProperty {
-    /// Get the name of this property.
+    /// Get the id of the property we're animating.
-    pub fn name(&self) -> &'static str {
+    pub fn id(&self) -> LonghandId {
         match *self {
             % for prop in data.longhands:
-                % if prop.animatable and not prop.logical:
+            % if prop.animatable and not prop.logical:
-                    AnimatedProperty::${prop.camel_case}(..) => "${prop.name}",
+            AnimatedProperty::${prop.camel_case}(..) => LonghandId::${prop.camel_case},
-                % endif
+            % endif
             % endfor
         }
     }
 
+    /// Get the name of this property.
+    pub fn name(&self) -> &'static str {
+        self.id().name()
+    }
+
     /// Whether this interpolation does animate, that is, whether the start and
     /// end values are different.
     pub fn does_animate(&self) -> bool {

tests/wpt/metadata/css/css-fonts/variations/font-style-interpolation.html.ini

@@ -1,6 +1,6 @@
 [font-style-interpolation.html]
   bug: https://github.com/servo/servo/issues/21570
   expected: TIMEOUT
-  [font-style animation]
+  [font-style transition]
     expected: TIMEOUT
 

tests/wpt/metadata/css/css-variables/variable-transitions-transition-property-all-before-value.html.ini

@@ -0,0 +1,3 @@
+[variable-transitions-transition-property-all-before-value.html]
+  [Verify substituted color value after transition]
+    expected: FAIL

tests/wpt/metadata/css/css-variables/variable-transitions-value-before-transition-property-all.html.ini

@@ -0,0 +1,3 @@
+[variable-transitions-value-before-transition-property-all.html]
+  [Verify substituted color value after transition]
+    expected: FAIL