search

PolymerElements / neon-animation

Polymer + Web Animations
https://www.webcomponents.org/element/PolymerElements/neon-animation
143 stars 98 forks source link

Ensure only a single instance of an animation is running #182

Closed dstoc closed 8 years ago

dstoc commented 8 years ago

Fixes https://github.com/PolymerElements/neon-animation/issues/81

samuelli commented 8 years ago

Strange API with cancelAnimation(), but sure.

LGTM.

atotic commented 8 years ago

@dstockwell I assume this fixes the bug, it has been a while since I looked at that code. Just curious, why didn't you fix web-animations and use the cancel event? Anyone else using web-animations will need the cancel event too.

dstoc commented 8 years ago

@dstockwell I assume this fixes the bug, it has been a while since I looked at that code. Just curious, why didn't you fix web-animations and use the cancel event? Anyone else using web-animations will need the cancel event too.

@atotic, the cancel event in web animations has been fixed: https://github.com/web-animations/web-animations-next/issues/368 & https://github.com/web-animations/web-animations-next/pull/416

We don't need to listen to the cancel event in neon as no one else should be calling cancel() directly on these animations.

atotic commented 8 years ago

Having a bit of a hard time following the code (always happens when I am reading neon-animation). If I understand correctly, the reason this fixes the bug is because:

Runner.playAnimation() calls animation.cancel() on previous animations with the same name as new animation.

What would happen if new animation's name was different from name of previous animation. I think that cancel would never get called for previous animation, which could trigger a bug.

In the declarative demo, none of animations have a name, (ie: they all have the same name: "undefined"), so things work. What would happen if animations had different names....

dstoc commented 8 years ago

What would happen if new animation's name was different from name of previous animation. I think that cancel would never get called for previous animation, which could trigger a bug.

When the animations finish their finish event will cancel/complete. But it's true there are cases where not cancelling other animations immediately could cause issues. Unfortunately the API was never really clear what should happen here, and the original behavior was that you could run any number of animations concurrently.

I'll think a bit more on this, but right now I am leaning back towards only allowing a single animation at a time.