Replace proxied animations in getAnimations reusing proxied CSS animations #89
Conversation
…mations This makes document.getAnimations and Element.getAnimations correctly return proxied instances of the animation so as to behave as expected. Also by getting the proxied animations we can intelligently reuse the existing proxied CSS animation when only timing details have changed which implicitly fixes the performance issue of #84.
|
|
||
| // Swap the original animation with the proxied one | ||
| if (proxyAnimation !== null) { | ||
| const result = createScrollTimeline(anim, anim.animationName, evt.target); |
There was a problem hiding this comment.
result may be null.
There was a problem hiding this comment.
Canceling and restarting a CSS animation will unconditionally create a new ScrollTimeline. Possibly not a big performance hit, but it looked like this was one of the things the previous cache PR was trying to avoid.
There was a problem hiding this comment.
+1 on result possibly being null
There was a problem hiding this comment.
result may be null.
Thanks, done!
Canceling and restarting a CSS animation will unconditionally create a new ScrollTimeline. Possibly not a big performance hit, but it looked like this was one of the things the previous cache PR was trying to avoid.
I think createScrollTimeline is pretty inexpensive. Scroll driven animations themselves will generally start once and run forever. Non-scroll driven animations should quickly be determined not to match any of the rules in getAnimationTimeline (and we didn't used to cache misses anyways). There are other easy optimizations we could do here if it became a bottleneck.
src/scroll-timeline-css.js
Outdated
| } else { | ||
| // If the timeline was removed or the animation was already an instance of a proxy animation, | ||
| // invoke the set the timeline procedure on the existing animation. | ||
| anim.timeline = result.timeline; |
There was a problem hiding this comment.
Don’t see why this needs to be set every time. If it’s not needed, then we can return earlier in this block, after doing a simple anim instanceof ProxyAnimation check, thereby preventing excessive calls to createScrollTimeline.
There was a problem hiding this comment.
If the style changed such that the underlying CSS animation was restarted there's a good chance that the timeline may have changed as well. We could try removing this but I don't expect this to occur frequently. Scroll driven animations tend to start once and run for the lifetime of their element / the page.
| PASS /scroll-animations/css/animation-timeline-ignored.tentative.html animation-timeline ignored after setting timeline with JS (document timeline) | ||
| PASS /scroll-animations/css/animation-timeline-ignored.tentative.html animation-timeline ignored after setting timeline with JS (null) | ||
| FAIL /scroll-animations/css/animation-timeline-ignored.tentative.html animation-timeline ignored after setting timeline with JS (document timeline) | ||
| FAIL /scroll-animations/css/animation-timeline-ignored.tentative.html animation-timeline ignored after setting timeline with JS (null) |
There was a problem hiding this comment.
I checked and these two new failures are really just exposing existing issues. The first is failing because the animated position is ever so slightly off from the position we had with the scroll timeline - perhaps we need to allow for some skew? The second is that we simply don't currently support setting timeline = null, but before this change we were setting timeline = null on the real animation rather than the proxy animation so it didn't exercise the code at all.
|
I'm a bit surprised there's not more new passing tests, as there are quite a few wpt/scroll-animation tests which rely on getAnimations to get the scroll driven animation, but this will certainly be a prereq for getting any of those tests working. |
flackr
force-pushed
the
getAnimations
branch
from
912d293 to
c6daadc
Compare
This makes document.getAnimations and Element.getAnimations correctly return proxied instances of the animation so as to behave as expected. Also by getting the proxied animations we can intelligently reuse the existing proxied CSS animation when only timing details have changed which implicitly fixes the performance issue of #84.