Restyle should reflect animations and transitions

components/layout/animation.rs

@@ -15,6 +15,7 @@ use script_traits::UntrustedNodeAddress;
 use script_traits::{
     AnimationState, ConstellationControlMsg, LayoutMsg as ConstellationMsg, TransitionEventType,
 };
+use servo_arc::Arc;
 use style::animation::{
     update_style_for_animation, Animation, ElementAnimationState, PropertyAnimation,
 };
@@ -223,7 +224,7 @@ fn do_recalc_style_for_animations<E>(
             update_style_for_animation::<E>(
                 &context.style_context,
                 animation,
-                &mut fragment.style,
+                Arc::make_mut(&mut fragment.style),
                 &ServoMetricsProvider,
             );
             let difference = RestyleDamage::compute_style_difference(&old_style, &fragment.style);

components/style/animation.rs

@@ -442,39 +442,36 @@ impl ElementAnimationState {
         false
     }
 
-    /// Compute before-change-style given an existing ElementAnimationState,
-    /// information from the StyleContext, and the values of the previous style
-    /// computation.
-    ///
-    /// TODO(mrobinson): This is not a correct computation of before-change-style.
-    /// For starters it's unclear why we aren't using the running transitions to
-    /// transform this style into before-change-style.
-    pub(crate) fn compute_before_change_style<E>(
+    pub(crate) fn apply_completed_animations(&mut self, style: &mut Arc<ComputedValues>) {
+        for animation in self.finished_animations.iter() {
+            // TODO: Make this a bit more general and add support animation-fill-mode.
+            // Without support for that property though, animations that have ended should
+            // not affect property values. This is why we only process transitions here.
+            if let Animation::Transition(_, _, property_animation) = animation {
+                property_animation.update(Arc::make_mut(style), 1.0);
+            }
+        }
+    }
+
+    pub(crate) fn apply_running_animations<E>(
         &mut self,
         context: &SharedStyleContext,
         style: &mut Arc<ComputedValues>,
         font_metrics: &dyn crate::font_metrics::FontMetricsProvider,
     ) where
         E: TElement,
     {
-        for animation in self.finished_animations.iter() {
-            debug!("Updating style for finished animation {:?}", animation);
-            // TODO: support animation-fill-mode
-            if let Animation::Transition(_, _, property_animation) = animation {
-                property_animation.update(Arc::make_mut(style), 1.0);
-            }
+        // Return early so that we don't unnecessarily clone the style when making it mutable.
+        if self.running_animations.is_empty() {
+            return;
         }
 
-        for running_animation in self.running_animations.iter_mut() {
-            let update = match *running_animation {
-                Animation::Transition(..) => continue,
-                Animation::Keyframes(..) => {
-                    update_style_for_animation::<E>(context, running_animation, style, font_metrics)
-                },
-            };
+        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 *running_animation {
-                Animation::Transition(..) => unreachable!(),
+            match *animation {
+                Animation::Transition(..) => {},
                 Animation::Keyframes(_, _, _, ref mut state) => match update {
                     AnimationUpdate::Regular => {},
                     AnimationUpdate::AnimationCanceled => {
@@ -485,6 +482,25 @@ impl ElementAnimationState {
         }
     }
 
+    pub(crate) fn apply_new_animations<E>(
+        &mut self,
+        context: &SharedStyleContext,
+        style: &mut Arc<ComputedValues>,
+        font_metrics: &dyn crate::font_metrics::FontMetricsProvider,
+    ) where
+        E: TElement,
+    {
+        // Return early so that we don't unnecessarily clone the style when making it mutable.
+        if self.new_animations.is_empty() {
+            return;
+        }
+
+        let style = Arc::make_mut(style);
+        for animation in self.new_animations.iter_mut() {
+            update_style_for_animation::<E>(context, animation, style, font_metrics);
+        }
+    }
+
     /// Whether this `ElementAnimationState` is empty, which means it doesn't
     /// hold any animations in any state.
     pub fn is_empty(&self) -> bool {
@@ -526,21 +542,19 @@ pub fn start_transitions_if_applicable(
     new_style: &mut Arc<ComputedValues>,
     animation_state: &mut ElementAnimationState,
 ) -> LonghandIdSet {
-    use crate::properties::animated_properties::TransitionPropertyIteration;
-
     // 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();
     }
 
     let mut properties_that_transition = LonghandIdSet::new();
-    let transitions: Vec<TransitionPropertyIteration> = new_style.transition_properties().collect();
-    for transition in &transitions {
-        if properties_that_transition.contains(transition.longhand_id) {
+    for transition in new_style.transition_properties() {
+        let physical_property = transition.longhand_id.to_physical(new_style.writing_mode);
+        if properties_that_transition.contains(physical_property) {
             continue;
         } else {
-            properties_that_transition.insert(transition.longhand_id);
+            properties_that_transition.insert(physical_property);
         }
 
         let property_animation = match PropertyAnimation::from_longhand(
@@ -552,18 +566,12 @@ pub fn start_transitions_if_applicable(
                 .get_box()
                 .transition_duration_mod(transition.index),
             old_style,
-            Arc::make_mut(new_style),
+            new_style,
         ) {
             Some(property_animation) => property_animation,
             None => continue,
         };
 
-        // Set the property to the initial value.
-        //
-        // NB: get_mut is guaranteed to succeed since we called make_mut()
-        // above.
-        property_animation.update(Arc::get_mut(new_style).unwrap(), 0.0);
-
         // 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 running or
         // completed.
@@ -747,7 +755,7 @@ pub enum AnimationUpdate {
 pub fn update_style_for_animation<E>(
     context: &SharedStyleContext,
     animation: &Animation,
-    style: &mut Arc<ComputedValues>,
+    style: &mut ComputedValues,
     font_metrics_provider: &dyn FontMetricsProvider,
 ) -> AnimationUpdate
 where
@@ -762,7 +770,7 @@ where
             let progress = (now - start_time) / (property_animation.duration);
             let progress = progress.min(1.0);
             if progress >= 0.0 {
-                property_animation.update(Arc::make_mut(style), progress);
+                property_animation.update(style, progress);
             }
             AnimationUpdate::Regular
         },
@@ -867,7 +875,7 @@ where
             let from_style = compute_style_for_animation_step::<E>(
                 context,
                 last_keyframe,
-                &**style,
+                style,
                 &state.cascade_style,
                 font_metrics_provider,
             );
@@ -911,7 +919,7 @@ where
                             property
                         );
                         debug!("update_style_for_animation: {:?}", property_animation);
-                        property_animation.update(Arc::make_mut(&mut new_style), relative_progress);
+                        property_animation.update(&mut new_style, relative_progress);
                     },
                     None => {
                         debug!(

components/style/matching.rs

@@ -445,7 +445,10 @@ trait PrivateMatchMethods: TElement {
         let mut animation_state = animation_states.remove(&this_opaque).unwrap_or_default();
 
         if let Some(ref mut old_values) = *old_values {
-            animation_state.compute_before_change_style::<Self>(
+            // We convert old values into `before-change-style` here.
+            // https://drafts.csswg.org/css-transitions/#starting
+            animation_state.apply_completed_animations(old_values);
+            animation_state.apply_running_animations::<Self>(
                 shared_context,
                 old_values,
                 &context.thread_local.font_metrics_provider,
@@ -475,10 +478,20 @@ trait PrivateMatchMethods: TElement {
                 .cancel_transitions_with_nontransitioning_properties(&transitioning_properties);
         }
 
-        animation_state.finished_animations.clear();
+        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,
+            &context.thread_local.font_metrics_provider,
+        );
 
-        // If the ElementAnimationState is empty, don't push it to save
-        // memory and to avoid extra processing later.
+        // If the ElementAnimationState is empty, and don't store it in order to
+        // save memory and to avoid extra processing later.
+        animation_state.finished_animations.clear();
         if !animation_state.is_empty() {
             animation_states.insert(this_opaque, animation_state);
         }

tests/wpt/include.ini

@@ -19,20 +19,34 @@ skip: true
     skip: false
   [CSS2]
     skip: false
+    [linebox]
+      skip:false
+      [animations]
+        skip: true
   [css-align]
     skip: false
+    [animation]
+      skip: true
   [css-animations]
     skip: false
   [css-backgrounds]
     skip: false
+    [animations]
+      skip: true
   [css-color]
     skip: false
+    [animation]
+      skip: true
   [css-conditional]
     skip: false
   [css-flexbox]
     skip: false
+    [animation]
+      skip: true
   [css-fonts]
     skip: false
+    [animations]
+      skip: true
   [css-images]
     skip: false
   [css-paint-api]
@@ -41,18 +55,28 @@ skip: true
     skip: false
   [css-text]
     skip: false
+    [animations]
+      skip: true
     [i18n]
       skip: true
   [css-text-decor]
     skip: false
   [css-transforms]
     skip: false
+    [animation]
+      skip: true
   [css-transitions]
     skip: false
+    [animations]
+      skip: true
   [css-ui]
     skip: false
+    [animation]
+      skip: true
   [css-values]
     skip: false
+    [animations]
+      skip: true
   [css-variables]
     skip: false
   [cssom]
@@ -61,6 +85,8 @@ skip: true
     skip: false
   [filter-effects]
     skip: false
+    [animation]
+      skip: true
   [geometry]
     skip: false
   [mediaqueries]