Bug 1643201 - servo: Split animation cancellation from update_style_for_animation.
authorMartin Robinson <mrobinson@igalia.com>
Thu, 04 Jun 2020 00:34:09 +0000
changeset 533798 7937d276db3e02d7dd8aafb169e9dc21e0fd2ea2
parent 533797 6afd89eee82bed152461b0ec7e4a050ec38fe663
child 533799 1779bf62260b1eb8894cd320d2183a7c20d13cf0
push id37478
push userabutkovits@mozilla.com
push dateThu, 04 Jun 2020 09:29:07 +0000
treeherdermozilla-central@e87e4800d332 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1643201
milestone79.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1643201 - servo: Split animation cancellation from update_style_for_animation. `update_style_for_animation` previously handled both canceling defunct animations and also updating style to reflect current animation state. This change splits those two concerns because we want to start handling replaced or canceled animations and finished animations in two different places. This is a refactor, so ideally it shouldn't change any behavior. Depends on D78185 Differential Revision: https://phabricator.services.mozilla.com/D78186
servo/components/style/animation.rs
servo/components/style/matching.rs
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -4,17 +4,17 @@
 
 //! CSS transitions and animations.
 
 // NOTE(emilio): This code isn't really executed in Gecko, but we don't want to
 // compile it out so that people remember it exists.
 
 use crate::bezier::Bezier;
 use crate::context::SharedStyleContext;
-use crate::dom::{OpaqueNode, TElement};
+use crate::dom::{OpaqueNode, TElement, TNode};
 use crate::font_metrics::FontMetricsProvider;
 use crate::properties::animated_properties::AnimatedProperty;
 use crate::properties::longhands::animation_direction::computed_value::single_value::T as AnimationDirection;
 use crate::properties::longhands::animation_play_state::computed_value::single_value::T as AnimationPlayState;
 use crate::properties::{self, CascadeMode, ComputedValues, LonghandId};
 #[cfg(feature = "servo")]
 use crate::properties::LonghandIdSet;
 use crate::stylesheets::keyframes_rule::{KeyframesAnimation, KeyframesStep, KeyframesStepValue};
@@ -67,33 +67,29 @@ pub struct KeyframesAnimationState {
     /// The current iteration state for the animation.
     pub iteration_state: KeyframesIterationState,
     /// Werther this animation is paused.
     pub running_state: KeyframesRunningState,
     /// The declared animation direction of this animation.
     pub direction: AnimationDirection,
     /// The current animation direction. This can only be `normal` or `reverse`.
     pub current_direction: AnimationDirection,
-    /// Werther this keyframe animation is outdated due to a restyle.
-    pub expired: bool,
     /// The original cascade style, needed to compute the generated keyframes of
     /// the animation.
     pub cascade_style: Arc<ComputedValues>,
 }
 
 impl KeyframesAnimationState {
     /// Performs a tick in the animation state, i.e., increments the counter of
     /// the current iteration count, updates times and then toggles the
     /// direction if appropriate.
     ///
     /// Returns true if the animation should keep running.
     pub fn tick(&mut self) -> bool {
         debug!("KeyframesAnimationState::tick");
-        debug_assert!(!self.expired);
-
         self.started_at += self.duration + self.delay;
         match self.running_state {
             // If it's paused, don't update direction or iteration count.
             KeyframesRunningState::Paused(_) => return true,
             KeyframesRunningState::Running => {},
         }
 
         if let KeyframesIterationState::Finite(ref mut current, ref max) = self.iteration_state {
@@ -184,17 +180,16 @@ impl fmt::Debug for KeyframesAnimationSt
         f.debug_struct("KeyframesAnimationState")
             .field("started_at", &self.started_at)
             .field("duration", &self.duration)
             .field("delay", &self.delay)
             .field("iteration_state", &self.iteration_state)
             .field("running_state", &self.running_state)
             .field("direction", &self.direction)
             .field("current_direction", &self.current_direction)
-            .field("expired", &self.expired)
             .field("cascade_style", &())
             .finish()
     }
 }
 
 /// State relating to an animation.
 #[derive(Clone, Debug)]
 pub enum Animation {
@@ -211,25 +206,16 @@ pub enum Animation {
         OpaqueNode,
         KeyframesAnimation,
         Atom,
         KeyframesAnimationState,
     ),
 }
 
 impl Animation {
-    /// Whether this animation is expired.
-    #[inline]
-    pub fn is_expired(&self) -> bool {
-        match *self {
-            Animation::Transition(..) => false,
-            Animation::Keyframes(_, _, _, ref state) => state.expired,
-        }
-    }
-
     /// The opaque node that owns the animation.
     #[inline]
     pub fn node(&self) -> &OpaqueNode {
         match *self {
             Animation::Transition(ref node, _, _) => node,
             Animation::Keyframes(ref node, _, _, _) => node,
         }
     }
@@ -240,24 +226,43 @@ impl Animation {
         match *self {
             Animation::Transition(..) => true,
             Animation::Keyframes(..) => false,
         }
     }
 
     /// Whether this animation has the same end value as another one.
     #[inline]
-    pub fn is_transition_with_same_end_value(&self, other_animation: &PropertyAnimation) -> bool {
+    fn is_transition_with_same_end_value(&self, other_animation: &PropertyAnimation) -> bool {
         match *self {
             Animation::Transition(_, _, ref animation) => {
                 animation.has_the_same_end_value_as(other_animation)
             },
             Animation::Keyframes(..) => false,
         }
     }
+
+    /// Whether or not this animation is cancelled by changes from a new style.
+    fn is_animation_cancelled_in_new_style(&self, new_style: &Arc<ComputedValues>) -> bool {
+        let name = match *self {
+            Animation::Transition(..) => return false,
+            Animation::Keyframes(_, _, ref name, _) => name,
+        };
+
+        let index = new_style
+            .get_box()
+            .animation_name_iter()
+            .position(|animation_name| Some(name) == animation_name.as_atom());
+        let index = match index {
+            Some(index) => index,
+            None => return true,
+        };
+
+        new_style.get_box().animation_duration_mod(index).seconds() == 0.
+    }
 }
 
 /// Represents an animation for a given property.
 #[derive(Clone, Debug)]
 pub struct PropertyAnimation {
     /// An `AnimatedProperty` that this `PropertyAnimation` corresponds to.
     property: AnimatedProperty,
 
@@ -397,19 +402,24 @@ impl ElementAnimationState {
     pub(crate) fn cancel_transitions_with_nontransitioning_properties(
         &mut self,
         properties_that_transition: &LonghandIdSet,
     ) {
         if self.running_animations.is_empty() {
             return;
         }
 
-        let previously_running_transitions =
-            std::mem::replace(&mut self.running_animations, Vec::new());
-        for running_animation in previously_running_transitions {
+        // TODO(mrobinson): We should make this more efficient perhaps by using
+        // a linked-list or by using something like `partition`.
+        let animation_count = self.running_animations.len();
+        let mut previously_running_animations = std::mem::replace(
+            &mut self.running_animations,
+            Vec::with_capacity(animation_count),
+        );
+        for running_animation in previously_running_animations.drain(..) {
             if let Animation::Transition(_, _, ref property_animation) = running_animation {
                 if !properties_that_transition.contains(property_animation.property_id()) {
                     self.cancelled_animations.push(running_animation);
                     continue;
                 }
             }
             self.running_animations.push(running_animation);
         }
@@ -464,27 +474,17 @@ impl ElementAnimationState {
     {
         // Return early so that we don't unnecessarily clone the style when making it mutable.
         if self.running_animations.is_empty() {
             return;
         }
 
         let style = Arc::make_mut(style);
         for animation in self.running_animations.iter_mut() {
-            let update = update_style_for_animation::<E>(context, animation, style, font_metrics);
-
-            match *animation {
-                Animation::Transition(..) => {},
-                Animation::Keyframes(_, _, _, ref mut state) => match update {
-                    AnimationUpdate::Regular => {},
-                    AnimationUpdate::AnimationCanceled => {
-                        state.expired = true;
-                    },
-                },
-            }
+            update_style_for_animation::<E>(context, animation, style, font_metrics);
         }
     }
 
     pub(crate) fn apply_new_animations<E>(
         &mut self,
         context: &SharedStyleContext,
         style: &mut Arc<ComputedValues>,
         font_metrics: &dyn crate::font_metrics::FontMetricsProvider,
@@ -527,25 +527,82 @@ impl ElementAnimationState {
                     },
                     _ => {},
                 }
             }
         }
         // Otherwise just add the new running animation.
         self.add_new_animation(new_animation);
     }
+
+    /// Update our animations given a new style, canceling or starting new animations
+    /// when appropriate.
+    pub fn update_animations_for_new_style<E>(
+        &mut self,
+        element: E,
+        context: &SharedStyleContext,
+        new_style: &Arc<ComputedValues>,
+    ) where
+        E: TElement,
+    {
+        // Cancel any animations that no longer exist in the style.
+        // TODO(mrobinson): We should make this more efficient perhaps by using
+        // a linked-list or by using something like `partition`.
+        if !self.running_animations.is_empty() {
+            let animation_count = self.running_animations.len();
+            let mut previously_running_animations = std::mem::replace(
+                &mut self.running_animations,
+                Vec::with_capacity(animation_count),
+            );
+            for animation in previously_running_animations.drain(..) {
+                if animation.is_animation_cancelled_in_new_style(new_style) {
+                    self.cancelled_animations.push(animation);
+                } else {
+                    self.running_animations.push(animation);
+                }
+            }
+        }
+
+        maybe_start_animations(element, &context, &new_style, self);
+    }
+
+    /// Update our transitions given a new style, canceling or starting new animations
+    /// when appropriate.
+    pub fn update_transitions_for_new_style(
+        &mut self,
+        context: &SharedStyleContext,
+        opaque_node: OpaqueNode,
+        before_change_style: Option<&Arc<ComputedValues>>,
+        new_style: &Arc<ComputedValues>,
+    ) {
+        // If this is the first style, we don't trigger any transitions and we assume
+        // there were no previously triggered transitions.
+        let before_change_style = match before_change_style {
+            Some(before_change_style) => before_change_style,
+            None => return,
+        };
+
+        let transitioning_properties = start_transitions_if_applicable(
+            context,
+            opaque_node,
+            before_change_style,
+            new_style,
+            self,
+        );
+        self.cancel_transitions_with_nontransitioning_properties(&transitioning_properties);
+    }
 }
 
 /// Kick off any new transitions for this node and return all of the properties that are
 /// transitioning. This is at the end of calculating style for a single node.
 pub fn start_transitions_if_applicable(
     context: &SharedStyleContext,
     opaque_node: OpaqueNode,
     old_style: &ComputedValues,
-    new_style: &mut Arc<ComputedValues>,
+    new_style: &Arc<ComputedValues>,
     animation_state: &mut ElementAnimationState,
 ) -> LonghandIdSet {
     // If the style of this element is display:none, then we don't start any transitions
     // and we cancel any currently running transitions by returning an empty LonghandIdSet.
     if new_style.get_box().clone_display().is_none() {
         return LonghandIdSet::new();
     }
 
@@ -645,32 +702,31 @@ where
     }
 }
 
 /// Triggers animations for a given node looking at the animation property
 /// values.
 pub fn maybe_start_animations<E>(
     element: E,
     context: &SharedStyleContext,
-    node: OpaqueNode,
     new_style: &Arc<ComputedValues>,
     animation_state: &mut ElementAnimationState,
 ) where
     E: TElement,
 {
     let box_style = new_style.get_box();
     for (i, name) in box_style.animation_name_iter().enumerate() {
         let name = match name.as_atom() {
             Some(atom) => atom,
             None => continue,
         };
 
         debug!("maybe_start_animations: name={}", name);
-        let total_duration = box_style.animation_duration_mod(i).seconds();
-        if total_duration == 0. {
+        let duration = box_style.animation_duration_mod(i).seconds();
+        if duration == 0. {
             continue;
         }
 
         let anim = match context.stylist.get_animation(name, element) {
             Some(animation) => animation,
             None => continue,
         };
 
@@ -682,17 +738,16 @@ pub fn maybe_start_animations<E>(
         // values.
         if anim.steps.is_empty() {
             continue;
         }
 
         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 {
@@ -707,105 +762,67 @@ pub fn maybe_start_animations<E>(
         let running_state = match box_style.animation_play_state_mod(i) {
             AnimationPlayState::Paused => KeyframesRunningState::Paused(0.),
             AnimationPlayState::Running => KeyframesRunningState::Running,
         };
 
         animation_state.add_or_update_new_animation(
             &context.timer,
             Animation::Keyframes(
-                node,
+                element.as_node().opaque(),
                 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(),
                 },
             ),
         );
     }
 }
 
-/// 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 ComputedValues,
     font_metrics_provider: &dyn FontMetricsProvider,
-) -> AnimationUpdate
-where
+) where
     E: TElement,
 {
     debug!("update_style_for_animation: {:?}", animation);
-    debug_assert!(!animation.is_expired());
-
     match *animation {
         Animation::Transition(_, start_time, ref property_animation) => {
             let now = context.timer.seconds();
             let progress = (now - start_time) / (property_animation.duration);
             let progress = progress.min(1.0);
             if progress >= 0.0 {
                 property_animation.update(style, progress);
             }
-            AnimationUpdate::Regular
         },
         Animation::Keyframes(_, ref animation, ref name, ref state) => {
             let duration = state.duration;
             let started_at = state.started_at;
 
             let now = match state.running_state {
                 KeyframesRunningState::Running => context.timer.seconds(),
                 KeyframesRunningState::Paused(progress) => started_at + duration * progress,
             };
 
             debug_assert!(!animation.steps.is_empty());
-
-            let maybe_index = style
-                .get_box()
-                .animation_name_iter()
-                .position(|animation_name| Some(name) == animation_name.as_atom());
-
-            let index = match maybe_index {
-                Some(index) => index,
-                None => return AnimationUpdate::AnimationCanceled,
-            };
-
-            let total_duration = style.get_box().animation_duration_mod(index).seconds() as f64;
-            if total_duration == 0. {
-                return AnimationUpdate::AnimationCanceled;
-            }
-
-            let mut total_progress = (now - started_at) / total_duration;
+            let mut total_progress = (now - started_at) / duration;
             if total_progress < 0. {
                 warn!("Negative progress found for animation {:?}", name);
-                return AnimationUpdate::Regular;
+                return;
             }
             if total_progress > 1. {
                 total_progress = 1.;
             }
 
             // Get the target and the last keyframe position.
             let last_keyframe_position;
             let target_keyframe_position;
@@ -843,53 +860,63 @@ where
 
             debug!(
                 "update_style_for_animation: keyframe from {:?} to {:?}",
                 last_keyframe_position, target_keyframe_position
             );
 
             let target_keyframe = match target_keyframe_position {
                 Some(target) => &animation.steps[target],
-                None => return AnimationUpdate::Regular,
+                None => return,
             };
 
             let last_keyframe = &animation.steps[last_keyframe_position];
 
             let relative_timespan =
                 (target_keyframe.start_percentage.0 - last_keyframe.start_percentage.0).abs();
             let relative_duration = relative_timespan as f64 * duration;
             let last_keyframe_ended_at = match state.current_direction {
                 AnimationDirection::Normal => {
-                    state.started_at + (total_duration * last_keyframe.start_percentage.0 as f64)
+                    state.started_at + (duration * last_keyframe.start_percentage.0 as f64)
                 },
                 AnimationDirection::Reverse => {
-                    state.started_at +
-                        (total_duration * (1. - last_keyframe.start_percentage.0 as f64))
+                    state.started_at + (duration * (1. - last_keyframe.start_percentage.0 as f64))
                 },
                 _ => unreachable!(),
             };
             let relative_progress = (now - last_keyframe_ended_at) / relative_duration;
 
             // TODO: How could we optimise it? Is it such a big deal?
             let from_style = compute_style_for_animation_step::<E>(
                 context,
                 last_keyframe,
                 style,
                 &state.cascade_style,
                 font_metrics_provider,
             );
 
             // NB: The spec says that the timing function can be overwritten
             // from the keyframe style.
-            let mut timing_function = style.get_box().animation_timing_function_mod(index);
-            if last_keyframe.declared_timing_function {
+            let timing_function = if last_keyframe.declared_timing_function {
                 // NB: animation_timing_function can never be empty, always has
                 // at least the default value (`ease`).
-                timing_function = from_style.get_box().animation_timing_function_at(0);
-            }
+                from_style.get_box().animation_timing_function_at(0)
+            } else {
+                // TODO(mrobinson): It isn't optimal to have to walk this list every
+                // time. Perhaps this should be stored in the animation.
+                let index = match style
+                    .get_box()
+                    .animation_name_iter()
+                    .position(|animation_name| Some(name) == animation_name.as_atom())
+                {
+                    Some(index) => index,
+                    None => return warn!("Tried to update a style with a cancelled animation."),
+                };
+                style.get_box().animation_timing_function_mod(index)
+            };
 
             let target_style = compute_style_for_animation_step::<E>(
                 context,
                 target_keyframe,
                 &from_style,
                 &state.cascade_style,
                 font_metrics_provider,
             );
@@ -927,12 +954,11 @@ where
                 }
             }
 
             debug!(
                 "update_style_for_animation: got style change in animation \"{}\"",
                 name
             );
             *style = new_style;
-            AnimationUpdate::Regular
         },
     }
 }
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -2,18 +2,16 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 //! High-level interface to CSS selector matching.
 
 #![allow(unsafe_code)]
 #![deny(missing_docs)]
 
-#[cfg(feature = "servo")]
-use crate::animation;
 use crate::computed_value_flags::ComputedValueFlags;
 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::TNode;
 use crate::invalidation::element::restyle_hints::RestyleHint;
@@ -450,39 +448,24 @@ trait PrivateMatchMethods: TElement {
             animation_state.apply_completed_animations(old_values);
             animation_state.apply_running_animations::<Self>(
                 shared_context,
                 old_values,
                 &context.thread_local.font_metrics_provider,
             );
         }
 
-        // Trigger any present animations if necessary.
-        animation::maybe_start_animations(
-            *self,
+        animation_state.update_animations_for_new_style(*self, &shared_context, &new_values);
+        animation_state.update_transitions_for_new_style(
             &shared_context,
             this_opaque,
-            &new_values,
-            &mut animation_state,
+            old_values.as_ref(),
+            new_values,
         );
 
-        // Trigger transitions if necessary. This will set `new_values` to
-        // the starting value of the transition if it did trigger a transition.
-        if let Some(ref old_values) = old_values {
-            let transitioning_properties = animation::start_transitions_if_applicable(
-                shared_context,
-                this_opaque,
-                old_values,
-                new_values,
-                &mut animation_state,
-            );
-            animation_state
-                .cancel_transitions_with_nontransitioning_properties(&transitioning_properties);
-        }
-
         animation_state.apply_running_animations::<Self>(
             shared_context,
             new_values,
             &context.thread_local.font_metrics_provider,
         );
         animation_state.apply_new_animations::<Self>(
             shared_context,
             new_values,