trurl-master / jsdom-testing-mocks

A set of tools for emulating browser behavior in jsdom environment
MIT License
116 stars 7 forks source link

animation.id, animation.cancel #42

Closed adjsky closed 1 year ago

adjsky commented 1 year ago

Hi! Using Web Animations API mock i have noticed two moments:

  1. passing id to options does nothing, animation.id returns null
  2. calling animation.cancel doesn't delete the animation from element.getAnimations()

These moment are easy to fix so i decided to fix it by myself in this PR

What i've done:

animation.id https://developer.mozilla.org/en-US/docs/Web/API/Element/animate As mentioned in MDN options should accept an optional id

  1. Change options type to KeyframeAnimationOptions
  2. If options is an object and options.id is defined, attach options.id to animation.id

animation.cancel I haven't found a description of this behavior in the specification (either in MDN docs) but chrome and FF delete cancelled animations from element.getAnimations()

There is an example: https://codesandbox.io/s/competent-golick-0qwqd1?file=/src/App.js

  1. Add cancel event listener to an animation
  2. Remove the animation from animations array

Also i've added tests to cover the new functionality

trurl-master commented 1 year ago

@adjsky Hi! Thank you so much for taking the time to do that!

animation.id

Nicely noticed, I completely missed that

animation.cancel

Yeah, you're right. I wanted also to refactor this part a tiny bit, and move the logic responsible for associating animations with elements inside the Animation class itself, because currently it won't work if you manually create an animation. If you're up to the task, it would be great, if not, I'll look into it some time this or next week.

I thought of extracting the elementAnimations map from index.ts into a separate module and move the logic responsible for adding and removing animations to the Animation class (probably inside #resolvers.ready.resolve for adding and #resolvers.finished.resolve/reject for removing, not sure yet)

adjsky commented 1 year ago

Thank you for reviewing the PR!

If you're up to the task, it would be great, if not, I'll look into it some time this or next week.

I won't have access to my PC for the next 6 days, so i won't be able to until a week later 😅

adjsky commented 1 year ago

@trurl-master Hi! I moved the adding/removing logic to the Animation class

What's changed:

  1. Moved the elementAnimations map and methods for adding/removing/getting animations from index.ts to elementAnimations.ts module
  2. Added new methods to the Animation class, #addToTarget and #removeFromTarget to add and remove animations from effect.target, #removeFromTarget is called in #resolvers.finished.resolve/reject, #addToTarget in #getNewReadyPromise

I didn't put #addToTarget in #resolvers.ready.resolve because this resolver is resolved in queueMicrotask. If #addToTarget was in #resolvers.ready.resolve there would be no way to see animation in element.getAnimations() after calling animation.play(). But for me it seems a bad place to put #addToTarget inside #getNewReadyPromise, if that's okay for you - i'll leave it as it is

trurl-master commented 1 year ago

@adjsky it looks reasonable to me, I'm not sure what can be done better at this time. Check it out if it works for you.

Thank you so much for the contribution! ❤️