fix(script): request animation ticks if Animations::pending_events is not empty

Fixes the test case `/_mozilla/css/css-transition-cancel-event
.html`, which was failing under a specific circumstance.

The observed sequence of events during the failing test run looks like
this:

1. Transitions start in `div1` and `div2`.

2. `div1` generates a `transitionend` event.

3. The `transitionend` event handler removes `div2` from DOM, cancelling
   its ongoing transition.

4. `div2` is supposed to generate a `transitioncancel` event in a timely
   manner, which it does not. The test fails as a result.

What is going on here? Here's a possible explaination:

1. During one invocation of `ScriptThread::handle_msgs`...

2. In step 2, `ScriptThread::update_animations_send_events` -> `Document
   ::update_for_new_timeline_value` detects the completion of the
   transition, and in response, pends the `transitionend` event.

3. In step 3, `ScriptThread::update_animations_send_events` ->
   `Animations::send_pending_events` calls the `transitionend` handler.

4. The `transitionend` event handler removes `div2`, thereby cancelling
   its ongoing transition and triggering a reflow.

5. Reflow takes place. During this, `Animations::do_post_reflow_update`
   -> `Animations::handle_canceled_animations` pends the
   `transitioncancel` event (precursor to step 4).

6. Having discovering that there was no running animation, `Animations::
   do_post_reflow_update` calls `self.update_running_animation_presence
   (_, false)`, which sends `AnimationState::NoAnimationsPresent`.

7. The invocation of `ScriptThread::handle_msgs` ends, and another
   starts. It blocks waiting for events.

8. Meanwhile, the compositor receives `AnimationState::
   NoAnimationsPresent` and stops further generation of animation ticks.

9. With no events to wake it up, the script thread is stuck waiting
   despite having the pending `transitioncancel` event (step 4).

The HTML specification [says][1] that "an event loop must continually
run [...] as long as it exists" and does not say it can block if there
is nothing to do. Blocking is merely optimization in a user agent
implementation. Pending animation-related events must be processed every
time a "rendering opportunity" arises unless the user agent has a reason
to believe that it "would have no visible effect".

Skipping the processing of animation-related events would have visible
effect if such events are indeed present. The correct implementation in
Servo, therefore, would be to request more animation ticks so that such
events are processed in a subsequent tick.

[1]: https://html.spec.whatwg.org/multipage/#event-loop-processing-model

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

@@ -1709,7 +1709,7 @@ impl ScriptThread {
         }
 
         for (_, document) in self.documents.borrow().iter() {
-            document.animations().send_pending_events();
+            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>