Html: cancelAnimationFrame should also cancel pending callbacks

[egirard]

requestAnimationFrame callbacks cannot reliably cancel other requestAnimationFrame callbacks.

Calls to requestAnimationFrame will not add callbacks to the current callback loop, but instead adds them to the next callback loop. This allows callbacks to re-register themselves if they need to continue operating, which is a very common pattern for developers.

cancelAnimationFrame follows the same pattern, and as a result requestAnimationFrame callbacks have somewhat unpredictable results if they make calls to cancelAnimationFrame.

For example, assume a page has called requestAnimationFrame with rAF-callback1 and rAF-callback2, each of which loops by calling rAF on themselves at the end of the callback.

• if rAF-callback2 calls cancelAnimationFrame on rAF-callback1, then callback1 will be removed from future callbacks. The cancelAnimationFrame call succeeds.
• if rAF-callback1 calls cancelAnimationFrame on rAF-callback2, the current (pending) call to callback2 continues, and the cancelAnimationFrame call has no effect.

Recommend that we amend the spec so that calls to cancelAnimationFrame also cancel pending callbacks within the current callback loop.

Many browsers (including Chromium, Edge, Safari) already follow this implementation. Previous discussion on this issue can be found in mozilla and chromium

[annevk]

cc @birtles @jakearchibald @rocallahan

[jakearchibald]

To summarise:

function one() {
  console.log('one');
  cancelAnimationFrame(twoHandle);
}

function two() {
  console.log('two');
}

const oneHandle = requestAnimationFrame(one);
const twoHandle = requestAnimationFrame(two);

Should "two" be logged? Demo.

Chrome, Edge, Safari say no. Firefox and the spec say yes.

---

In the spec cancelAnimationFrame removes items from the map of animation frame callbacks. However, when the event loop processes animation frames, it clones the map of callbacks, so it's out-of-scope for cancelAnimationFrame's removals.

This feels unintuitive for me. For instance:

let animOneHandle;
let animTwoHandle;

function animOne() {
  // …
  animOneHandle = requestAnimationFrame(animOne);
}

function animTwo() {
  // …
  animTwoHandle = requestAnimationFrame(animTwo);
}

function startAnims() {
  animOneHandle = requestAnimationFrame(animOne);
  animTwoHandle = requestAnimationFrame(animTwo);
}

function stopAnims() {
  cancelAnimationFrame(animOneHandle);
  cancelAnimationFrame(animTwoHandle);
}

With this pattern, stopAnims will stop the next call to animOne and animTwo sometimes, depending on when it's called.

I also find Flackr's comparison to event listeners compelling.

In terms of updating the spec, instead of cloning the map, it should clone the keys. Then, it can get each callback from the original map, skipping it if it's missing. Items are removed from the original map after each callback.

[jakearchibald]

I asked Twitter, and I'm really surprised at how close the vote is https://twitter.com/jaffathecake/status/1095662236230733824.

However, it seems like there's a lot of confusion around how animation callbacks are queued. A lot of folks think the order is random, or they happen in parallel somehow. Oh, and a few folks seem to think it's a scoping issue.

[rocallahan]

I think the first comment here is a compelling argument for changing the spec.

Originally I didn't have cancelAnimationFrame, instead people who wanted to cancel could set up their own boolean cancellation flag and just bail out of the callback if that flag is set. Would have avoided this problem :-).

[domenic]

It sounds like we should change this (great find!) but I'd love to hear from Firefox folks before changing the spec out from under them. Let's hope @birtles can chime in soon.

[flackr]

Another case for the proposed behavior is consistency with setTimeout, which has the same semantics in this demo. I believe both a and b's time has come up when a is called, but b is still canceled by a.

[jakearchibald]

Each setTimeout callback is another turn of the event loop, so the event listener example seems a lot closer.

[wanderview]

Each setTimeout callback is another turn of the event loop, so the event listener example seems a lot closer.

FWIW setTimeout callbacks can be executed in batch within a single event loop turn. I don't think the spec explicitly calls this out, but it vaguely allows it by having a separate timer task source.

[birtles]

I agree that the first comment is particularly compelling. I'm ok with this change.

[annevk]

@wanderview how does that allow for it? What you're suggesting would mean a Promise.resolve().then(...) would not necessarily run at the end of a timeout, which is not something the specification allows for currently. File a new issue if that's indeed the case?

[jakearchibald]

You always get a microtask checkpoint when the stack empties, so if a browser were running multiple timeout callbacks in a single task it wouldn't change promise behaviour.

But yeah the spec doesn't seem to allow this grouping.

[annevk]

In that case (executing two in a single task vs each in their own task not being observable) @wanderview is correct as they do share a task source and the user agent is allowed to prioritize that task source over others.

[wanderview]

FWIW, I tried to removing this batching to improve interactivity and ran into web compat issues. See this post for more details.

(Sorry for the thread hijack regarding this.)

[domenic]

Sounds like we have agreement. Is anyone willing to work on a spec change and tests PR? I wasn't able to gather the exact model for the spec change proposed; I thought maybe it was just not doing the clone, but Chrome at least seems to have two callback collections (callbacks_ and callbacks_to_invoke_), which is more complicated.

[birtles]

Yes, we don't want to drop the cloning, otherwise calls to requestAnimationFrame from within a rAF callback would end up running in the same frame. @jakearchibald proposed the spec behavior earlier:

In terms of updating the spec, instead of cloning the map, it should clone the keys. Then, it can get each callback from the original map, skipping it if it's missing. Items are removed from the original map after each callback.

It looks like getting the keys of an ordered map already creates a new ordered set so if we want to avoid having two callback collections like in Chrome, I guess we could do something like:

1. Let _callbacks_ be _target_'s map of animation frame callbacks.
2. Let _callback handles_ be the result of getting the keys of _callbacks_.
3. For each _handle_ in _callback handles_, if _callbacks_ contains _handle_, perform the following steps:
   
   
   
   1. Let _callback_ be _callbacks_[_handle_].
   
   
   2. Remove _callbacks_[_handle_].
   
   
   3. Invoke _callback_, passing _now_ as the only argument, and if an exception is thrown, report the exception.
   
   

Does that seem right?

[annevk]

Looks good to me.

[egirard]

@birtles suggestion looks good to me.

On a side note, the existing blink implementation uses a cancelled flag (similar to Event.cancelled) and tests that flag before calling each callback. This avoids duplicating the collection.

[birtles]

Thanks for the feedback. I'll work on a PR and WPT for this.