internals: refactor stop animations resolve mechanism

[Matin297]

This pull request is associated with this issue that refactors stopAnimations function.

Problem

In the current implementation of stopAnimations, the event listeners for cancel and finish are not doing anything particular regarding promise resolution and are just returning a variable named handleAnimationEvent that is the result of requestAnimationFrame call, which based on the documentation is an ID that we can use to cancel animation frame request.

Solution

The changes made will make sure that if the animation is canceled or finished before the requestAnimationFrame callback is called then it will resolve the promise and will call cancelAnimationFrame with the requestId.

Or when the browser is about to repaint and none of the cancel or finish events were fired then the requestAnimationFrame callback is called, the event listeners are removed and the promise will get resolved before the repaint.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
shoelace	✅ Ready (Inspect)	Visit Preview	Dec 12, 2023 6:18pm

[claviska]

Thanks for catching this! Indeed, the handlers from the original source aren't being executed:

return new Promise(resolve => {
  const handleAnimationEvent = requestAnimationFrame(resolve);

  animation.addEventListener('cancel', () => handleAnimationEvent, { once: true });
  animation.addEventListener('finish', () => handleAnimationEvent, { once: true });
  animation.cancel();
});

The idea behind this was to wait one frame to make sure the canceled/completed animation was fully painted before resolving. I'm not sure that we need to call cancelAnimationFrame() in the animation's cancel and finish handlers. At this point, the animation has already been canceled or completed, respectively, so we just need to wait for a repaint before resolving.

Fixing the original code's handler so it actually runs is probably sufficient.

return new Promise(resolve => {
  const handleAnimationEvent = requestAnimationFrame(resolve);

  animation.addEventListener('cancel', handleAnimationEvent, { once: true });
  animation.addEventListener('finish', handleAnimationEvent, { once: true });
  animation.cancel();
});

[claviska]

What's the purpose of setting these properties to null?

[Matin297]

Here I am trying to remove the event listeners for we no longer need to run their handlers since we're resolving the promise using requestAnimationFrame callback. (in the case where this callback is called before the event handlers);

[claviska]

I'm not following why we're canceling the requestAnimationFrame() created above. I don't see how it's related to the animation. At this point, we just need to wait for one more frame before resolving.

[Matin297]

Here I am trying to cancel requested animation frame for we no longer need to run the callback passed to it since we're resolving the promise using this event handler. (in the case where handler is called before the end of current frame);

[Matin297]

What I was trying to do was to make use of the event listeners besides requestAnimationFrame to resolve the promise as soon as possible.

Though based on what you said "The idea behind this was to wait one frame to make sure the canceled/completed animation was fully painted before resolving" I think we can leave the requestAnimationFrame callback as it was before But I don't think we need to add event listeners for cancel or finish events since we're not doing anything particular in their handlers.

return new Promise(resolve => {
  requestAnimationFrame(resolve);
  animation.cancel();
}); 

Also here below, I think the problem with the changes that you suggested is, as I mentioned in the description, the value returned from requestAnimationFrame is a number and not a function and is not callable. The variable name "handleAnimationEvent " is a little bit misleading.

return new Promise(resolve => {
  const handleAnimationEvent = requestAnimationFrame(resolve);

  animation.addEventListener('cancel', handleAnimationEvent, { once: true });
  animation.addEventListener('finish', handleAnimationEvent, { once: true });
  animation.cancel();
}); 

[claviska]

I took a closer look at this and I think you're correct that we can get rid of the listeners in this case. The docs on MDN aren't really clear on this, but I tested it out and it seems like this will suffice.

export function stopAnimations(el: HTMLElement) {
  return Promise.all(
    el.getAnimations().map(animation => {
      return new Promise(resolve => {
        animation.cancel();
        requestAnimationFrame(resolve);
      });
    })
  );
}

Calling animation.cancel() stops the animation and emits the cancel event. The requestAnimationFrame() ensures that the event is already emitted and repainting has occurred for all animations before resolving the call to stopAnimations().

What do you think?

[Matin297]

It looks great 👌