Bug 1500260 - Expire keyframes animations when no longer referenced by the style. r=emilio
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 05 May 2018 19:15:59 +0200
changeset 490346 7037075929099a8d88ef702257e88d15cbe3ab59
parent 490345 6d0c84abdfa1d1b5e64e78a1826762e110c7ea57
child 490347 99cc3c6f76ef81913aea6d0a7d785e02e55f4648
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersemilio
bugs1500260, 20731, 20757
milestone64.0a1
Bug 1500260 - Expire keyframes animations when no longer referenced by the style. r=emilio It's a long way to make this sound in general... Fixes #20731 This cherry-picks part of servo/servo#20757.
servo/components/style/animation.rs
servo/components/style/matching.rs
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -625,38 +625,62 @@ pub fn update_style_for_animation_frame(
 
     frame
         .property_animation
         .update(Arc::make_mut(&mut new_style), progress);
 
     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_assert!(!animation.is_expired());
 
     match *animation {
         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 =
                 update_style_for_animation_frame(&mut new_style, now, start_time, frame);
             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;
@@ -670,38 +694,28 @@ pub fn update_style_for_animation<E>(
 
             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 => {
-                    warn!(
-                        "update_style_for_animation: Animation {:?} not found in style",
-                        name
-                    );
-                    return;
-                },
+                None => return AnimationUpdate::AnimationCanceled,
             };
 
             let total_duration = style.get_box().animation_duration_mod(index).seconds() as f64;
             if total_duration == 0. {
-                debug!(
-                    "update_style_for_animation: zero duration for animation {:?}",
-                    name
-                );
-                return;
+                return AnimationUpdate::AnimationCanceled;
             }
 
             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
@@ -743,21 +757,17 @@ pub fn update_style_for_animation<E>(
 
             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 => {
-                    warn!("update_style_for_animation: No current keyframe found for animation \"{}\" at progress {}",
-                          name, total_progress);
-                    return;
-                },
+                None => return AnimationUpdate::Regular,
             };
 
             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 {
@@ -831,16 +841,17 @@ pub fn update_style_for_animation<E>(
                 }
             }
 
             debug!(
                 "update_style_for_animation: got style change in animation \"{}\"",
                 name
             );
             *style = new_style;
+            AnimationUpdate::Regular
         },
     }
 }
 
 /// Update the style in the node when it finishes.
 #[cfg(feature = "servo")]
 pub fn complete_expired_transitions(
     node: OpaqueNode,
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -580,17 +580,17 @@ trait PrivateMatchMethods: TElement {
     #[cfg(feature = "servo")]
     fn update_animations_for_cascade(
         &self,
         context: &SharedStyleContext,
         style: &mut Arc<ComputedValues>,
         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.
         let this_opaque = self.as_node().opaque();
         animation::complete_expired_transitions(this_opaque, style, context);
 
         // Merge any running animations into the current style, and cancel them.
         let had_running_animations = context
@@ -598,40 +598,50 @@ trait PrivateMatchMethods: TElement {
             .read()
             .get(&this_opaque)
             .is_some();
         if !had_running_animations {
             return;
         }
 
         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
             // mainly huge load or debug builds, the constellation might be
             // 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 {
-                possibly_expired_animations.push(frame.property_animation.clone())
+            match *running_animation {
+                Animation::Transition(_, _, ref frame) => {
+                    possibly_expired_animations.push(frame.property_animation.clone())
+                }
+                Animation::Keyframes(_, _, _, ref mut state) => {
+                    match update {
+                        AnimationUpdate::Regular => {},
+                        AnimationUpdate::AnimationCanceled => {
+                            state.expired = true;
+                        }
+                    }
+                }
             }
         }
     }
 }
 
 impl<E: TElement> PrivateMatchMethods for E {}
 
 /// The public API that elements expose for selector matching.