Auto merge of #26407 - mrobinson:animation-restyle-model, r=jdm

Have animations more closely match the HTML spec

These two commits do two major things:

**Have animations ticks trigger a restyle**: This corrects synchronization issues with animations,
where the values used in layout are out of sync with what is returned by `getComputedStyle`.

**Tick the animation timer in script according to spec**: This greatly reduces the flakiness of
animation and transitions tests.

Fixes #13865.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #13865

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

components/script/animation_timeline.rs

@@ -0,0 +1,49 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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/. */
+
+#![deny(missing_docs)]
+
+//! A timeline module, used to specify an `AnimationTimeline` which determines
+//! the time used for synchronizing animations in the script thread.
+
+use time;
+
+/// A `AnimationTimeline` which is used to synchronize animations during the script
+/// event loop.
+#[derive(Clone, Copy, Debug, JSTraceable, MallocSizeOf)]
+pub struct AnimationTimeline {
+    current_value: f64,
+}
+
+impl AnimationTimeline {
+    /// Creates a new "normal" timeline, i.e., a "Current" mode timer.
+    #[inline]
+    pub fn new() -> Self {
+        Self {
+            current_value: time::precise_time_s(),
+        }
+    }
+
+    /// Creates a new "test mode" timeline, with initial time 0.
+    #[inline]
+    pub fn new_for_testing() -> Self {
+        Self { current_value: 0. }
+    }
+
+    /// Returns the current value of the timeline in seconds.
+    pub fn current_value(&self) -> f64 {
+        self.current_value
+    }
+
+    /// Updates the value of the `AnimationTimeline` to the current clock time.
+    pub fn update(&mut self) {
+        self.current_value = time::precise_time_s();
+    }
+
+    /// Increments the current value of the timeline by a specific number of seconds.
+    /// This is used for testing.
+    pub fn advance_specific(&mut self, by: f64) {
+        self.current_value += by;
+    }
+}

components/script/dom/document.rs

@@ -2,6 +2,7 @@
  * 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/. */
 
+use crate::animation_timeline::AnimationTimeline;
 use crate::document_loader::{DocumentLoader, LoadType};
 use crate::dom::attr::Attr;
 use crate::dom::beforeunloadevent::BeforeUnloadEvent;
@@ -380,6 +381,9 @@ pub struct Document {
     csp_list: DomRefCell<Option<CspList>>,
     /// https://w3c.github.io/slection-api/#dfn-selection
     selection: MutNullableDom<Selection>,
+    /// A timeline for animations which is used for synchronizing animations.
+    /// https://drafts.csswg.org/web-animations/#timeline
+    animation_timeline: DomRefCell<AnimationTimeline>,
 }
 
 #[derive(JSTraceable, MallocSizeOf)]
@@ -2904,6 +2908,11 @@ impl Document {
             dirty_webgl_contexts: DomRefCell::new(HashMap::new()),
             csp_list: DomRefCell::new(None),
             selection: MutNullableDom::new(None),
+            animation_timeline: if pref!(layout.animations.test.enabled) {
+                DomRefCell::new(AnimationTimeline::new_for_testing())
+            } else {
+                DomRefCell::new(AnimationTimeline::new())
+            },
         }
     }
 
@@ -3605,6 +3614,18 @@ impl Document {
             })
             .collect()
     }
+
+    pub fn advance_animation_timeline_for_testing(&self, delta: f64) {
+        self.animation_timeline.borrow_mut().advance_specific(delta);
+    }
+
+    pub fn update_animation_timeline(&self) {
+        self.animation_timeline.borrow_mut().update();
+    }
+
+    pub fn current_animation_timeline_value(&self) -> f64 {
+        self.animation_timeline.borrow().current_value()
+    }
 }
 
 impl Element {

components/script/dom/htmlimageelement.rs

@@ -60,8 +60,8 @@ use mime::{self, Mime};
 use msg::constellation_msg::PipelineId;
 use net_traits::image::base::{Image, ImageMetadata};
 use net_traits::image_cache::{
-    CanRequestImages, CorsStatus, ImageCache, ImageCacheResult, ImageOrMetadataAvailable,
-    ImageResponse, PendingImageId, PendingImageResponse, UsePlaceholder,
+    CorsStatus, ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponse,
+    PendingImageId, PendingImageResponse, UsePlaceholder,
 };
 use net_traits::request::{CorsSettings, Destination, Initiator, RequestBuilder};
 use net_traits::{FetchMetadata, FetchResponseListener, FetchResponseMsg, NetworkError};
@@ -326,7 +326,6 @@ impl HTMLImageElement {
             cors_setting_for_element(self.upcast()),
             sender,
             UsePlaceholder::Yes,
-            CanRequestImages::Yes,
         );
 
         match cache_result {
@@ -1107,7 +1106,6 @@ impl HTMLImageElement {
             cors_setting_for_element(self.upcast()),
             sender,
             UsePlaceholder::No,
-            CanRequestImages::Yes,
         );
 
         match cache_result {

components/script/dom/htmlvideoelement.rs

@@ -27,8 +27,8 @@ use html5ever::{LocalName, Prefix};
 use ipc_channel::ipc;
 use ipc_channel::router::ROUTER;
 use net_traits::image_cache::{
-    CanRequestImages, ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponse,
-    PendingImageId, UsePlaceholder,
+    ImageCache, ImageCacheResult, ImageOrMetadataAvailable, ImageResponse, PendingImageId,
+    UsePlaceholder,
 };
 use net_traits::request::{CredentialsMode, Destination, RequestBuilder};
 use net_traits::{
@@ -163,7 +163,6 @@ impl HTMLVideoElement {
             None,
             sender,
             UsePlaceholder::No,
-            CanRequestImages::Yes,
         );
 
         match cache_result {

components/script/dom/testbinding.rs

@@ -1054,8 +1054,8 @@ impl TestBindingMethods for TestBinding {
         }
     }
 
-    fn AdvanceClock(&self, ms: i32, tick: bool) {
-        self.global().as_window().advance_animation_clock(ms, tick);
+    fn AdvanceClock(&self, ms: i32) {
+        self.global().as_window().advance_animation_clock(ms);
     }
 
     fn Panic(&self) {

components/script/dom/webidls/TestBinding.webidl

@@ -517,7 +517,7 @@ interface TestBinding {
   [Pref="dom.testbinding.prefcontrolled.enabled"]
   const unsigned short prefControlledConstDisabled = 0;
   [Pref="layout.animations.test.enabled"]
-  void advanceClock(long millis, optional boolean forceLayoutTick = true);
+  void advanceClock(long millis);
 
   [Pref="dom.testbinding.prefcontrolled2.enabled"]
   readonly attribute boolean prefControlledAttributeEnabled;

components/script/dom/window.rs

@@ -173,6 +173,7 @@ pub enum ReflowReason {
     IFrameLoadEvent,
     MissingExplicitReflow,
     ElementStateChanged,
+    PendingReflow,
 }
 
 #[dom_struct]
@@ -1544,16 +1545,13 @@ impl Window {
         )
     }
 
-    /// Advances the layout animation clock by `delta` milliseconds, and then
-    /// forces a reflow if `tick` is true.
-    pub fn advance_animation_clock(&self, delta: i32, tick: bool) {
-        self.layout_chan
-            .send(Msg::AdvanceClockMs(
-                delta,
-                tick,
-                self.origin().immutable().clone(),
-            ))
-            .unwrap();
+    /// Prepares to tick animations and then does a reflow which also advances the
+    /// layout animation clock.
+    pub fn advance_animation_clock(&self, delta: i32) {
+        let pipeline_id = self.upcast::<GlobalScope>().pipeline_id();
+        self.Document()
+            .advance_animation_timeline_for_testing(delta as f64 / 1000.);
+        ScriptThread::handle_tick_all_animations_for_testing(pipeline_id);
     }
 
     /// Reflows the page unconditionally if possible and not suppressed. This
@@ -1626,9 +1624,8 @@ impl Window {
 
         // If this reflow is for display, ensure webgl canvases are composited with
         // up-to-date contents.
-        match reflow_goal {
-            ReflowGoal::Full => document.flush_dirty_canvases(),
-            ReflowGoal::TickAnimations | ReflowGoal::LayoutQuery(..) => {},
+        if for_display {
+            document.flush_dirty_canvases();
         }
 
         // Send new document and relevant styles to layout.
@@ -1645,6 +1642,7 @@ impl Window {
             script_join_chan: join_chan,
             dom_count: document.dom_count(),
             pending_restyles: document.drain_pending_restyles(),
+            animation_timeline_value: document.current_animation_timeline_value(),
         };
 
         self.layout_chan
@@ -2446,8 +2444,7 @@ fn should_move_clip_rect(clip_rect: UntypedRect<Au>, new_viewport: UntypedRect<f
 }
 
 fn debug_reflow_events(id: PipelineId, reflow_goal: &ReflowGoal, reason: &ReflowReason) {
-    let mut debug_msg = format!("**** pipeline={}", id);
-    debug_msg.push_str(match *reflow_goal {
+    let goal_string = match *reflow_goal {
         ReflowGoal::Full => "\tFull",
         ReflowGoal::TickAnimations => "\tTickAnimations",
         ReflowGoal::LayoutQuery(ref query_msg, _) => match query_msg {
@@ -2464,32 +2461,9 @@ fn debug_reflow_events(id: PipelineId, reflow_goal: &ReflowGoal, reason: &Reflow
             &QueryMsg::ElementInnerTextQuery(_) => "\tElementInnerTextQuery",
             &QueryMsg::InnerWindowDimensionsQuery(_) => "\tInnerWindowDimensionsQuery",
         },
-    });
+    };
 
-    debug_msg.push_str(match *reason {
-        ReflowReason::CachedPageNeededReflow => "\tCachedPageNeededReflow",
-        ReflowReason::RefreshTick => "\tRefreshTick",
-        ReflowReason::FirstLoad => "\tFirstLoad",
-        ReflowReason::KeyEvent => "\tKeyEvent",
-        ReflowReason::MouseEvent => "\tMouseEvent",
-        ReflowReason::Query => "\tQuery",
-        ReflowReason::Timer => "\tTimer",
-        ReflowReason::Viewport => "\tViewport",
-        ReflowReason::WindowResize => "\tWindowResize",
-        ReflowReason::DOMContentLoaded => "\tDOMContentLoaded",
-        ReflowReason::DocumentLoaded => "\tDocumentLoaded",
-        ReflowReason::StylesheetLoaded => "\tStylesheetLoaded",
-        ReflowReason::ImageLoaded => "\tImageLoaded",
-        ReflowReason::RequestAnimationFrame => "\tRequestAnimationFrame",
-        ReflowReason::WebFontLoaded => "\tWebFontLoaded",
-        ReflowReason::WorkletLoaded => "\tWorkletLoaded",
-        ReflowReason::FramedContentChanged => "\tFramedContentChanged",
-        ReflowReason::IFrameLoadEvent => "\tIFrameLoadEvent",
-        ReflowReason::MissingExplicitReflow => "\tMissingExplicitReflow",
-        ReflowReason::ElementStateChanged => "\tElementStateChanged",
-    });
-
-    println!("{}", debug_msg);
+    println!("**** pipeline={}\t{}\t{:?}", id, goal_string, reason);
 }
 
 impl Window {

components/script/lib.rs

@@ -47,6 +47,7 @@ extern crate servo_atoms;
 #[macro_use]
 extern crate style;
 
+mod animation_timeline;
 #[warn(deprecated)]
 #[macro_use]
 mod task;

components/script/script_thread.rs

@@ -136,16 +136,17 @@ use script_traits::CompositorEvent::{
     WheelEvent,
 };
 use script_traits::{
-    CompositorEvent, ConstellationControlMsg, DiscardBrowsingContext, DocumentActivity,
-    EventResult, HistoryEntryReplacement, InitialScriptState, JsEvalResult, LayoutMsg, LoadData,
-    LoadOrigin, MediaSessionActionType, MouseButton, MouseEventType, NewLayoutInfo, Painter,
-    ProgressiveWebMetricType, ScriptMsg, ScriptThreadFactory, ScriptToConstellationChan,
-    StructuredSerializedData, TimerSchedulerMsg, TouchEventType, TouchId,
-    TransitionOrAnimationEventType, UntrustedNodeAddress, UpdatePipelineIdReason,
-    WebrenderIpcSender, WheelDelta, WindowSizeData, WindowSizeType,
+    AnimationTickType, CompositorEvent, ConstellationControlMsg, DiscardBrowsingContext,
+    DocumentActivity, EventResult, HistoryEntryReplacement, InitialScriptState, JsEvalResult,
+    LayoutMsg, LoadData, LoadOrigin, MediaSessionActionType, MouseButton, MouseEventType,
+    NewLayoutInfo, Painter, ProgressiveWebMetricType, ScriptMsg, ScriptThreadFactory,
+    ScriptToConstellationChan, StructuredSerializedData, TimerSchedulerMsg, TouchEventType,
+    TouchId, TransitionOrAnimationEvent, TransitionOrAnimationEventType, UntrustedNodeAddress,
+    UpdatePipelineIdReason, WebrenderIpcSender, WheelDelta, WindowSizeData, WindowSizeType,
 };
 use servo_atoms::Atom;
 use servo_config::opts;
+use servo_config::pref;
 use servo_url::{ImmutableOrigin, MutableOrigin, ServoUrl};
 use std::borrow::Cow;
 use std::cell::Cell;
@@ -1497,8 +1498,7 @@ impl ScriptThread {
                         self.handle_set_scroll_state(id, &scroll_state);
                     })
                 },
-                FromConstellation(ConstellationControlMsg::TickAllAnimations(pipeline_id)) => {
-                    // step 7.8
+                FromConstellation(ConstellationControlMsg::TickAllAnimations(pipeline_id, _)) => {
                     if !animation_ticks.contains(&pipeline_id) {
                         animation_ticks.insert(pipeline_id);
                         sequential.push(event);
@@ -1544,6 +1544,9 @@ impl ScriptThread {
             }
         }
 
+        // Step 11.10 from https://html.spec.whatwg.org/multipage/#event-loops.
+        self.update_animations_and_send_events();
+
         // Process the gathered events.
         debug!("Processing events.");
         for msg in sequential {
@@ -1603,7 +1606,7 @@ impl ScriptThread {
 
             let pending_reflows = window.get_pending_reflow_count();
             if pending_reflows > 0 {
-                window.reflow(ReflowGoal::Full, ReflowReason::ImageLoaded);
+                window.reflow(ReflowGoal::Full, ReflowReason::PendingReflow);
             } else {
                 // Reflow currently happens when explicitly invoked by code that
                 // knows the document could have been modified. This should really
@@ -1616,6 +1619,19 @@ impl ScriptThread {
         true
     }
 
+    // Perform step 11.10 from https://html.spec.whatwg.org/multipage/#event-loops.
+    // TODO(mrobinson): This should also update the current animations and send events
+    // to conform to the HTML specification. This might mean having events for rooting
+    // DOM nodes and ultimately all animations living in script.
+    fn update_animations_and_send_events(&self) {
+        for (_, document) in self.documents.borrow().iter() {
+            // Only update the time if it isn't being managed by a test.
+            if !pref!(layout.animations.test.enabled) {
+                document.update_animation_timeline();
+            }
+        }
+    }
+
     fn categorize_msg(&self, msg: &MixedMessage) -> ScriptThreadEventCategory {
         match *msg {
             MixedMessage::FromConstellation(ref inner_msg) => match *inner_msg {
@@ -1703,7 +1719,7 @@ impl ScriptThread {
                 RemoveHistoryStates(id, ..) => Some(id),
                 FocusIFrame(id, ..) => Some(id),
                 WebDriverScriptCommand(id, ..) => Some(id),
-                TickAllAnimations(id) => Some(id),
+                TickAllAnimations(id, ..) => Some(id),
                 TransitionOrAnimationEvent { .. } => None,
                 WebFontLoaded(id) => Some(id),
                 DispatchIFrameLoadEvent {
@@ -1902,23 +1918,11 @@ impl ScriptThread {
             ConstellationControlMsg::WebDriverScriptCommand(pipeline_id, msg) => {
                 self.handle_webdriver_msg(pipeline_id, msg)
             },
-            ConstellationControlMsg::TickAllAnimations(pipeline_id) => {
-                self.handle_tick_all_animations(pipeline_id)
+            ConstellationControlMsg::TickAllAnimations(pipeline_id, tick_type) => {
+                self.handle_tick_all_animations(pipeline_id, tick_type)
             },
-            ConstellationControlMsg::TransitionOrAnimationEvent {
-                pipeline_id,
-                event_type,
-                node,
-                property_or_animation_name,
-                elapsed_time,
-            } => {
-                self.handle_transition_or_animation_event(
-                    pipeline_id,
-                    event_type,
-                    node,
-                    property_or_animation_name,
-                    elapsed_time,
-                );
+            ConstellationControlMsg::TransitionOrAnimationEvent(ref event) => {
+                self.handle_transition_or_animation_event(event);
             },
             ConstellationControlMsg::WebFontLoaded(pipeline_id) => {
                 self.handle_web_font_loaded(pipeline_id)
@@ -2914,35 +2918,51 @@ impl ScriptThread {
         debug!("Exited script thread.");
     }
 
+    /// Handles animation tick requested during testing.
+    pub fn handle_tick_all_animations_for_testing(id: PipelineId) {
+        SCRIPT_THREAD_ROOT.with(|root| {
+            let script_thread = unsafe { &*root.get().unwrap() };
+            script_thread
+                .handle_tick_all_animations(id, AnimationTickType::CSS_ANIMATIONS_AND_TRANSITIONS);
+        });
+    }
+
     /// Handles when layout thread finishes all animation in one tick
-    fn handle_tick_all_animations(&self, id: PipelineId) {
+    fn handle_tick_all_animations(&self, id: PipelineId, tick_type: AnimationTickType) {
         let document = match self.documents.borrow().find_document(id) {
             Some(document) => document,
             None => return warn!("Message sent to closed pipeline {}.", id),
         };
-        document.run_the_animation_frame_callbacks();
+        if tick_type.contains(AnimationTickType::REQUEST_ANIMATION_FRAME) {
+            document.run_the_animation_frame_callbacks();
+        }
+        if tick_type.contains(AnimationTickType::CSS_ANIMATIONS_AND_TRANSITIONS) {
+            match self.animating_nodes.borrow().get(&id) {
+                Some(nodes) => {
+                    for node in nodes.iter() {
+                        node.dirty(NodeDamage::NodeStyleDamaged);
+                    }
+                },
+                None => return,
+            }
+
+            document.window().add_pending_reflow();
+        }
     }
 
     /// Handles firing of transition-related events.
     ///
     /// TODO(mrobinson): Add support for more events.
-    fn handle_transition_or_animation_event(
-        &self,
-        pipeline_id: PipelineId,
-        event_type: TransitionOrAnimationEventType,
-        unsafe_node: UntrustedNodeAddress,
-        property_or_animation_name: String,
-        elapsed_time: f64,
-    ) {
+    fn handle_transition_or_animation_event(&self, event: &TransitionOrAnimationEvent) {
         let js_runtime = self.js_runtime.rt();
-        let node = unsafe { from_untrusted_node_address(js_runtime, unsafe_node) };
+        let node = unsafe { from_untrusted_node_address(js_runtime, event.node) };
 
         // We limit the scope of the borrow here, so that we don't maintain this borrow
         // and then incidentally trigger another layout. That might result in a double
         // mutable borrow of `animating_nodes`.
         {
             let mut animating_nodes = self.animating_nodes.borrow_mut();
-            let nodes = match animating_nodes.get_mut(&pipeline_id) {
+            let nodes = match animating_nodes.get_mut(&event.pipeline_id) {
                 Some(nodes) => nodes,
                 None => {
                     return warn!(
@@ -2964,17 +2984,12 @@ impl ScriptThread {
                 },
             };
 
-            if event_type.finalizes_transition_or_animation() {
+            if event.event_type.finalizes_transition_or_animation() {
                 nodes.remove(node_index);
             }
         }
 
-        // Not quite the right thing - see #13865.
-        if event_type.finalizes_transition_or_animation() {
-            node.dirty(NodeDamage::NodeStyleDamaged);
-        }
-
-        let event_atom = match event_type {
+        let event_atom = match event.event_type {
             TransitionOrAnimationEventType::AnimationEnd => atom!("animationend"),
             TransitionOrAnimationEventType::TransitionCancel => atom!("transitioncancel"),
             TransitionOrAnimationEventType::TransitionEnd => atom!("transitionend"),
@@ -2986,11 +3001,11 @@ impl ScriptThread {
         };
 
         // TODO: Handle pseudo-elements properly
-        let property_or_animation_name = DOMString::from(property_or_animation_name);
-        let elapsed_time = Finite::new(elapsed_time as f32).unwrap();
+        let property_or_animation_name = DOMString::from(event.property_or_animation_name.clone());
+        let elapsed_time = Finite::new(event.elapsed_time as f32).unwrap();
         let window = window_from_node(&*node);
 
-        if event_type.is_transition_event() {
+        if event.event_type.is_transition_event() {
             let event_init = TransitionEventInit {
                 parent,
                 propertyName: property_or_animation_name,