diff --git a/components/layout/animation.rs b/components/layout/animation.rs index b42933bb400..6b78d8cb8b8 100644 --- a/components/layout/animation.rs +++ b/components/layout/animation.rs @@ -79,7 +79,7 @@ pub fn update_animation_state( 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( }, }; + 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 { - script_chan - .send(ConstellationControlMsg::TransitionEnd( - node.to_untrusted_node_address(), - frame.property_animation.property_name().into(), - frame.duration, - )).unwrap(); + if let Animation::Transition(node, _, ref frame) = running_animation { + script_chan.send(ConstellationControlMsg::TransitionEnd( + node.to_untrusted_node_address(), + frame.property_animation.property_name().into(), + frame.duration, + )).unwrap(); } expired_animations diff --git a/components/style/animation.rs b/components/style/animation.rs index 5c0a7103426..17e8b0981fb 100644 --- a/components/style/animation.rs +++ b/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()`. - /// - /// The `bool` field is werther this animation should no longer run. - Transition(OpaqueNode, f64, AnimationFrame, bool), + Transition(OpaqueNode, f64, AnimationFrame), /// 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, - duration: duration, + timing_function, + 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() { - atom - } else { - continue; + let name = match name.as_atom() { + Some(atom) => atom, + None => continue, }; debug!("maybe_start_animations: name={}", name); @@ -556,61 +549,65 @@ where continue; } - if let Some(anim) = context.stylist.get_animation(name, element) { - debug!("maybe_start_animations: animation {} found", name); + let anim = match context.stylist.get_animation(name, element) { + Some(animation) => animation, + None => continue, + }; - // If this animation doesn't have any keyframe, we can just continue - // 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; - } + debug!("maybe_start_animations: animation {} found", name); - 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: iteration_state, - running_state: running_state, - direction: animation_direction, - current_direction: initial_direction, - expired: false, - cascade_style: new_style.clone(), - }, - )).unwrap(); - had_animations = true; + // If this animation doesn't have any keyframe, we can just continue + // 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(); + 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( context: &SharedStyleContext, animation: &Animation, style: &mut Arc, 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, _) => { - debug!("update_style_for_animation: transition found"); + Animation::Transition(_, start_time, ref frame) => { let now = context.timer.seconds(); let mut new_style = (*style).clone(); let updated_style = @@ -662,12 +676,14 @@ pub fn update_style_for_animation( 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( 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 - ); - // 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( 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]; @@ -846,6 +843,7 @@ pub fn update_style_for_animation( 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); } } diff --git a/components/style/matching.rs b/components/style/matching.rs index 922ecad9eb4..24c9e9bbf11 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -331,7 +331,7 @@ trait PrivateMatchMethods: TElement { fn process_animations( &self, context: &mut StyleContext, - old_values: &mut Option>, + old_values: Option<&Arc>, new_values: &mut Arc, restyle_hint: RestyleHint, important_rules_changed: bool, @@ -413,7 +413,7 @@ trait PrivateMatchMethods: TElement { fn process_animations( &self, context: &mut StyleContext, - old_values: &mut Option>, + old_values: Option<&Arc>, new_values: &mut Arc, _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 { - // FIXME(emilio, #20116): This makes no sense. + if old_values.is_some() { 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() { - // 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() { + for mut running_animation in all_running_animations.get_mut(&this_opaque).unwrap() { + if let Animation::Transition(_, _, ref frame) = *running_animation { + possibly_expired_animations.push(frame.property_animation.clone()); continue; } - animation::update_style_for_animation::( + let update = animation::update_style_for_animation::( 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(..) => 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, diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 437232d386a..98a74854b8d 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -127,17 +127,22 @@ pub enum AnimatedProperty { } impl AnimatedProperty { - /// Get the name of this property. - pub fn name(&self) -> &'static str { + /// Get the id of the property we're animating. + pub fn id(&self) -> LonghandId { match *self { % for prop in data.longhands: - % if prop.animatable and not prop.logical: - AnimatedProperty::${prop.camel_case}(..) => "${prop.name}", - % endif + % if prop.animatable and not prop.logical: + AnimatedProperty::${prop.camel_case}(..) => LonghandId::${prop.camel_case}, + % 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 { diff --git a/tests/wpt/metadata/css/css-fonts/variations/font-style-interpolation.html.ini b/tests/wpt/metadata/css/css-fonts/variations/font-style-interpolation.html.ini index 21acb456b9f..2e7944f8327 100644 --- a/tests/wpt/metadata/css/css-fonts/variations/font-style-interpolation.html.ini +++ b/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 diff --git a/tests/wpt/metadata/css/css-variables/variable-transitions-transition-property-all-before-value.html.ini b/tests/wpt/metadata/css/css-variables/variable-transitions-transition-property-all-before-value.html.ini new file mode 100644 index 00000000000..282cefe525a --- /dev/null +++ b/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 diff --git a/tests/wpt/metadata/css/css-variables/variable-transitions-value-before-transition-property-all.html.ini b/tests/wpt/metadata/css/css-variables/variable-transitions-value-before-transition-property-all.html.ini new file mode 100644 index 00000000000..e1fc8c9c968 --- /dev/null +++ b/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