google / ioweb2015

I/O Web App 2015
https://events.google.com/io2015/
Apache License 2.0
687 stars 122 forks source link

Animations: remove animations with fill: 'forwards' #674

Open samthor opened 9 years ago

samthor commented 9 years ago

Every Web Animation that is being played with a timing option fill: 'forwards' and then later forgotten about is essentially leaking a new AnimationPlayer.

For instance, navigating to many different pages is going to leave a big trail of each animation transition used to get there. Ostensibly the benefit here is that you can pop the animations to get back to the previous state.

We should either-

1) remove fill: 'forwards' (or 'both') where it doesn't make sense 2) have a way to pop/clear players once we're confidently done with them

ebidel commented 9 years ago

cancel()? http://w3c.github.io/web-animations/#dom-animationplayer-cancel

samthor commented 9 years ago

Yup, that would be fine too, just some effort in finding where to call that. We've got to keep track of the players we want to later remove.

Some notes after chatting to Chrome team-

  1. they played with IOWA, and found ~1500 leaked animations after about 10-15 minutes of random use
  2. concern after about 10k-100k+ animations in terms of memory
  3. but there's also a linear CPU cost if animations are on the same element, e.g. all the animations on IOWA.Elements.Nav.
ebidel commented 9 years ago

This is interesting to say the least! I'd like to better understand the intended usage pattern(s) of the player stuff, and whether that changes between native an polyfill world. TBH, I consider IOWA's usage of the API fairly straightforward and what may developers will also attemp. If developers are expected to do a ton of bookkeeping in a SPA, that seems like razors and knives everywhere.

addyosmani commented 9 years ago

I spent some time with @devnook and @paullewis memory profiling IOWA for leaks this morning. We discovered a lot of instances of detached DOM nodes (sourced from template stamping) that weren't getting correctly cleaned up, but nothing in our traces suggested it was an issue with the animation polyfills or player. I'd be super interested in how we arrived at the animation leaks data from above :)

devnook commented 9 years ago

I tried to rewrite page animations to avoid using fill: "forward", but it seems not possible at the moment. It works well in Chrome, but in FF and Safari the animated target looses its animated state at the end of animation (but before onfinish callback is called), making it impossible to properly finish an animation without flickering (e.g. hide content, update colors etc).