Auto merge of #29035 - yvt:patch/process-pending-animation-events, r=mrobinson

Don't hold back the delivery of pending animation events

The script event loop can block for an indefinite period despite having pending animation-related events that it needs to deliver immediately.

The bug can cause a test timeout in `/css/css-transitions/events-001.html` but (to my knowledge) never manifested in `master` because any irrelevant messages received by the script thread afterward would unblock the event loop and cause it to process any pending animation-related events. We can reproduce the bug in `master` by removing [the `maybe_observe_paint_time` call][2] in `LayoutThread::compute_abs_pos_and_build_display_list`. This also explains the test failure [seen in #28708][1] because the same call is [commented out][3].

[1]: https://github.com/servo/servo/pull/28708#issuecomment-1080003778
[2]: 5448528279/components/layout_thread/lib.rs (L1156-L1157)
[3]: 03a41ffe32/components/layout_thread/lib.rs (L1186-L1187)

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

---
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

components/script/animations.rs

@@ -40,7 +40,7 @@ pub(crate) struct Animations {
     pub sets: DocumentAnimationSet,
 
     /// Whether or not we have animations that are running.
-    have_running_animations: Cell<bool>,
+    has_running_animations: Cell<bool>,
 
     /// A list of nodes with in-progress CSS transitions or pending events.
     rooted_nodes: DomRefCell<FxHashMap<OpaqueNode, Dom<Node>>>,
@@ -53,7 +53,7 @@ impl Animations {
     pub(crate) fn new() -> Self {
         Animations {
             sets: Default::default(),
-            have_running_animations: Cell::new(false),
+            has_running_animations: Cell::new(false),
             rooted_nodes: Default::default(),
             pending_events: Default::default(),
         }
@@ -97,6 +97,7 @@ impl Animations {
         }
 
         self.unroot_unused_nodes(&sets);
+        //self.update_running_animations_presence(window);
     }
 
     /// Cancel animations for the given node, if any exist.
@@ -142,17 +143,25 @@ impl Animations {
     }
 
     fn update_running_animations_presence(&self, window: &Window, new_value: bool) {
-        let have_running_animations = self.have_running_animations.get();
+        let had_running_animations = self.has_running_animations.get();
-        if new_value == have_running_animations {
+        if new_value == had_running_animations {
             return;
         }
 
-        self.have_running_animations.set(new_value);
+        self.has_running_animations.set(new_value);
-        let state = match new_value {
+        self.handle_animation_presence_or_pending_events_change(window);
+    }
+
+    fn handle_animation_presence_or_pending_events_change(&self, window: &Window) {
+        let has_running_animations = self.has_running_animations.get();
+        let has_pending_events = !self.pending_events.borrow().is_empty();
+
+        // Do not send the NoAnimationCallbacksPresent state until all pending
+        // animation events are delivered.
+        let state = match has_running_animations || has_pending_events {
             true => AnimationsPresentState::AnimationsPresent,
             false => AnimationsPresentState::NoAnimationsPresent,
         };
-
         window.send_to_constellation(ScriptMsg::ChangeRunningAnimationsState(state));
     }
 
@@ -425,10 +434,13 @@ impl Animations {
             });
     }
 
-    pub(crate) fn send_pending_events(&self) {
+    pub(crate) fn send_pending_events(&self, window: &Window) {
         // Take all of the events here, in case sending one of these events
         // triggers adding new events by forcing a layout.
         let events = std::mem::replace(&mut *self.pending_events.borrow_mut(), Vec::new());
+        if events.is_empty() {
+            return;
+        }
 
         for event in events.into_iter() {
             // We root the node here to ensure that sending this event doesn't
@@ -488,6 +500,10 @@ impl Animations {
                     .fire(node.upcast());
             }
         }
+
+        if self.pending_events.borrow().is_empty() {
+            self.handle_animation_presence_or_pending_events_change(window);
+        }
     }
 }
 

components/script/script_thread.rs

@@ -1705,11 +1705,11 @@ impl ScriptThread {
     // Perform step 11.10 from https://html.spec.whatwg.org/multipage/#event-loops.
     fn update_animations_and_send_events(&self) {
         for (_, document) in self.documents.borrow().iter() {
-            document.animations().send_pending_events();
+            document.update_animation_timeline();
         }
 
         for (_, document) in self.documents.borrow().iter() {
-            document.update_animation_timeline();
+            document.animations().send_pending_events(document.window());
         }
     }
 

tests/wpt/mozilla/meta/MANIFEST.json

@@ -12980,6 +12980,13 @@
       ]
      ]
     },
+    "css-transition-cancel-event.html": [
+     "23400c556d58bb21b78a9cbed3b56028c7d299c3",
+     [
+      null,
+      {}
+     ]
+    ],
     "empty-keyframes.html": [
      "9f8935fb7f51219bb3ee07335e208a63c9edde81",
      [

tests/wpt/mozilla/tests/css/css-transition-cancel-event.html

@@ -0,0 +1,68 @@
+<!doctype html>
+<html>
+<head>
+<meta charset=utf-8>
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+</head>
+<body>
+
+<script>
+const loaded = new Promise(resolve => {
+  addEventListener('load', () => {
+    setTimeout(resolve, 100);
+  });
+});
+
+promise_test(async t => {
+  await loaded;
+
+  const div1 = document.createElement('div');
+  const div2 = document.createElement('div');
+
+  div1.setAttribute('style', 'transition: all .01s linear; ' +
+                             'padding-left: 1px');
+  div2.setAttribute('style', 'transition: all 50s linear; ' +
+                             'padding-left: 1px');
+  document.body.appendChild(div1);
+  document.body.appendChild(div2);
+
+  getComputedStyle(div1).paddingLeft;
+  getComputedStyle(div2).paddingLeft;
+  div1.style.paddingLeft = '10px';
+  div2.style.paddingLeft = '10px';
+
+  div1.ontransitionend = () => {
+    // Cancel `div2`'s transition
+    div2.remove();
+  };
+
+  const watcher = new EventWatcher(t, div2, [ 'transitioncancel' ]);
+
+  // Check that we receive `transitioncancel` very soon.
+  //
+  // This test was written to catch a specific bug where the event loop blocks
+  // waiting for new incoming messages despite still having pending animation-
+  // related events (a `transitioncancel` event in this case) that need to be
+  // processed on a next "rendering opportunity".
+  //
+  // We want to check that the `transitioncancel` event is delivered soon
+  // enough, but we can't rely on test timeout because the timeout timer would
+  // unblock the event loop, resulting in a terribly belate delivery of the
+  // `transitioncancel` event. No matter how short the timeout is, it would
+  // look like the event was delivered just in time.
+  //
+  // Therefore, we are going to check the time taken to receive the event. If
+  // the test times out, we will probably receive the event anyway, but the
+  // duration will be some large value like 9800.
+  const start = Date.now();
+  await watcher.wait_for('transitioncancel');
+
+  assert_less_than(Date.now() - start, 1000);
+}, 'transitioncancel is delivered promptly');
+
+</script>
+</body>
+</html>