Web Animations: Trigger and update CSS Animations backed by the Web Animations model

Web Animations: Trigger and update CSS Animations backed by the Web Animations model

This patch introduces CSSAnimations which tracks the state of active CSS Animations for
a single Element. CSS Animation updates are calculated and effects applied during the 
style resolution process (rather than post resolution as in the current implementation)
to allow our implementation to move towards the behavior specified by the CSSWG -- that
is, animations should apply as part of the style cascade before !important rules
(see http://crbug.com/232273).

This patch creates Keyframes from raw, unresolved CSS Values. A change is in progress to
implement a factory for AnimatableValues which will allow reading of the appropriate
resolved values from the render style (as the current implementation achieves by stashing
away an entire RenderStyle. See StyleResolver::styleForKeyframe and CSSPropertyAnimation).

This patch changes the lifetime of active CSS Animations to be managed by the Element (as
long as it is attached). In the current implementation active animations are owned by the
renderer but this has the consequence that animations are restarted on reattachment
(see http://crbug.com/265302).

Note that changes introduced by this patch are not yet shipping, they are governed by the
WebAnimationsCSS runtime feature and --enable-web-animations-css flag in Chromium.

This change is covered by existing layout tests in LayoutTests/animations/ -- however at
this point these tests are still being run manually as there are several other dependencies
before it will be feasible to enable the virtual suite for CSS Animations backed by Web
Animations.

BUG=258896, 265302, 232273
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155386

[dstockwell, 2013-07-29 05:28:15]

pdr for core/animation and dglazkov for the intersection with StyleResolver and
Element.

[dglazkov, 2013-07-29 18:15:38]

I am <__< about animation state living on Element/Document. Where can I go read
about this? It seems like an odd place to hoist it. Those are DOM-ey things, not
style-ey things.

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:708: void
StyleResolver::keyframeStylesForAnimation(Element* element, const StringImpl*
name, KeyframeAnimationEffect::KeyframeVector& keyframes)
Not for this patch, but just curious: does this have to live on StyleResolver?
Could the logic that builds keyframes (and I am assuming later interpolates
styles between them) live in a separate class?

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1228: Element* element =
state.element();
Whoops. This seems wrong. We _just_ did a pass at trying to make Element
unaffected while resolving its style, and now we're mutating the element again?

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1274:
element->ensureActiveAnimations()->cssAnimations()->update(element, display,
this, animations);
Curious: why does a DOM element hold information about animations? Doesn't this
belong in style-land somewhere?

[dstockwell, 2013-07-30 01:12:47]

On 2013/07/29 18:15:38, Dimitri Glazkov wrote:
> I am <__< about animation state living on Element/Document. Where can I go
read
> about this? It seems like an odd place to hoist it. Those are DOM-ey things,
not
> style-ey things.

I have some comments on the state in Element below. Although I'm still happy to
move away from ElementRareData if we can find somewhere that's more sensible.

As for the Timeline being part of Document, in Web Animations the timing model
is detached from the effect model and is exposed via the dom. You can construct
an entire tree of timed items which are not connected to the style system, eg.
Groups, Custom animation effects (callbacks), and (although deferred to a
separate spec) media elements.

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:708: void
StyleResolver::keyframeStylesForAnimation(Element* element, const StringImpl*
name, KeyframeAnimationEffect::KeyframeVector& keyframes)
On 2013/07/29 18:15:39, Dimitri Glazkov wrote:
> Not for this patch, but just curious: does this have to live on StyleResolver?
> Could the logic that builds keyframes (and I am assuming later interpolates
> styles between them) live in a separate class?

I think most of the logic can move out and I can work on that later. We'll just
need matchScopedKeyframesRule.

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1228: Element* element =
state.element();
On 2013/07/29 18:15:39, Dimitri Glazkov wrote:
> Whoops. This seems wrong. We _just_ did a pass at trying to make Element
> unaffected while resolving its style, and now we're mutating the element
again?

The only mutation here is the call to ensureActiveAnimations. Perhaps making
that method const, or a cast on #1274 would be more palatable?

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1274:
element->ensureActiveAnimations()->cssAnimations()->update(element, display,
this, animations);
On 2013/07/29 18:15:39, Dimitri Glazkov wrote:
> Curious: why does a DOM element hold information about animations? Doesn't
this
> belong in style-land somewhere?

I was going to store it in animation-land in a Map, but Elliott had concerns
(see https://codereview.chromium.org/19266007/)

For lifetime reasons Element seems like the next best place. CSS Animations are
started during style resolution, but end based on time, or element
detach/destruction.

In the current animation system this state lives in a Map from RenderObject, but
that fails to handle reattachment correctly.

ActiveAnimations currently only stores animations that are applied to the
RenderStyle, however it's likely that we will use it to store other DOM related
animation data (see
http://dev.w3.org/fxtf/web-animations/#extensions-to-the-element-interface).

Note also that in Web Animations, animations created by script (not CSS
Animations) will progress/fire events regardless of whether they have any effect
on the style of the element or even when the element is display:none (see
http://dev.w3.org/fxtf/web-animations/#timing-events). Animations or rather
Timed Items are not simply part of the style system.

[dstockwell, 2013-07-30 01:28:28]

On 2013/07/30 01:12:47, dstockwell wrote:
> In the current animation system this state lives in a Map from RenderObject,
but
> that fails to handle reattachment correctly.

I remembered another problem that I ran into while trying to use the
RenderObject.

We can be in style resolution for an element that does not yet have a
RenderObject -- but we need to store this state somewhere. The current system
avoids this by not starting animations until the style has been resolved and the
render object constructed, but we can't do that if we want to resolve
http://crbug.com/232273 (as described in the description for this change).

[dglazkov, 2013-07-30 22:07:27]

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21012002/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.cpp:1274:
element->ensureActiveAnimations()->cssAnimations()->update(element, display,
this, animations);
I just spoke with shans@ about this, and I think we can fix this quite easily.
Let's break this into two phases:

1) constructing a new animation
2) adding this animation to element->activeAnimations.

If we do the first one at the applyProperties time, then queue it up, then pour
the queue into element->activeAnimations.

Queue would be stored on StyleResolverState

This would allow us to:
1) retain Element constness in applyProperties
2) clearly inform style machinery about effects of property application (a
similar thing happens for images, for example).
3) provide a path for future refactorings that may include multiple threads,
etc.

WDYT?

[dstockwell, 2013-07-30 23:21:52]

On 2013/07/30 22:07:27, Dimitri Glazkov wrote:
> Let's break this into two phases:
> 
> 1) constructing a new animation
> 2) adding this animation to element->activeAnimations.
> 
> If we do the first one at the applyProperties time, then queue it up, then
pour
> the queue into element->activeAnimations.
> 
> Queue would be stored on StyleResolverState

I had already started working on a change to split out the starting of new
animations (for other reasons), so I think this should mostly be fine.

[dstockwell, 2013-08-01 07:51:05]

On 2013/07/30 22:07:27, Dimitri Glazkov wrote:
> WDYT?

Done, PTAL!

Your spidey sense was spot on and I now absolutely agree that we don't want to
be mutating animation state during style resolution.

This patch now only computes the update that is required during style
resolution. The updates aren't made until the style is applied to the render
object (which matches the existing implementation). In Patch Set #2 I tried to
thread a 'delta' summary of the update out through the RenderStyle but it didn't
really make sense, and is only an optimization that may not be necessary (we can
revisit if needed). Instead the animation updates are recalculated as needed.

The update state that we compute and use during style resolution consists of the
animations that will be stopped (so that we can filter their animated values
from being applied) and the styles that need to be applied for animations that
will be started (these are trivially calculated without starting the
animations).

I also took the opportunity to move the logic that builds keyframes out of
StyleResolver and into CSSAnimations. The logic for the existing implementation
remains and could be moved to eg. CSSPropertyAnimation, but either way I intend
to delete it after enabling web-animations-css.

[dglazkov, 2013-08-01 16:12:46]

This came together nicely. lgtm on StyleResolver parts with comments. I tried to
look over CSSAnimation code and it looks legit, but you might want another pair
of eyes to double-check.

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.cpp:137: const StylePropertySet*
firstKeyframeStyles(const Element* element, const StringImpl* animationName,
StyleResolver* resolver)
We should probably move this to StyleResolver, since all keyframe style
resolution-related code will need to be refactored out of it, and keeping all of
it together would help future us.

https://codereview.chromium.org/21012002/diff/15001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21012002/diff/15001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1203: PassOwnPtr<CSSAnimationUpdate>
StyleResolver::calculateCSSAnimationUpdate(StyleResolverState& state)
This guy sits awkwardly here. Ideally, it would take const element, const style,
and const ScopedStyleTree. It needs neither StyleResolverState nor
StyleResolver. Then, it could be a factory function on CSSAnimationUpdate.

But this can only happen after we move keyframe style resolution code out of
StyleResolver, so maybe add a fixme to help future us remember?

https://codereview.chromium.org/21012002/diff/15001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1211: if
(CSSAnimations::needsUpdate(element, state.style())) {
early return?

[shans, 2013-08-01 23:54:15]

LGTM on animations part.

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/Act...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/Act...
Source/core/animation/ActiveAnimations.h:45: CSSAnimations* cssAnimations() {
return &m_cssAnimations; }
Add a comment describing what goes in defaultStack and what goes in
cssAnimations. e.g. it's not obvious that all of the animations referenced in
cssAnimations will also be in the defaultStack (right?), but that CSS
Transitions is non-intersecting.

[dstockwell, 2013-08-02 00:11:01]

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/Act...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/Act...
Source/core/animation/ActiveAnimations.h:45: CSSAnimations* cssAnimations() {
return &m_cssAnimations; }
On 2013/08/01 23:54:16, shans wrote:
> Add a comment describing what goes in defaultStack and what goes in
> cssAnimations. e.g. it's not obvious that all of the animations referenced in
> cssAnimations will also be in the defaultStack (right?), but that CSS
> Transitions is non-intersecting.

Done.

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/21012002/diff/15001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.cpp:137: const StylePropertySet*
firstKeyframeStyles(const Element* element, const StringImpl* animationName,
StyleResolver* resolver)
On 2013/08/01 16:12:46, Dimitri Glazkov wrote:
> We should probably move this to StyleResolver, since all keyframe style
> resolution-related code will need to be refactored out of it, and keeping all
of
> it together would help future us.

Done.

https://codereview.chromium.org/21012002/diff/15001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21012002/diff/15001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1203: PassOwnPtr<CSSAnimationUpdate>
StyleResolver::calculateCSSAnimationUpdate(StyleResolverState& state)
On 2013/08/01 16:12:46, Dimitri Glazkov wrote:
> This guy sits awkwardly here. Ideally, it would take const element, const
style,
> and const ScopedStyleTree. It needs neither StyleResolverState nor
> StyleResolver. Then, it could be a factory function on CSSAnimationUpdate.
> 
> But this can only happen after we move keyframe style resolution code out of
> StyleResolver, so maybe add a fixme to help future us remember?

Done.

https://codereview.chromium.org/21012002/diff/15001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1211: if
(CSSAnimations::needsUpdate(element, state.style())) {
On 2013/08/01 16:12:46, Dimitri Glazkov wrote:
> early return?

Done.

[commit-bot: I haz the power, 2013-08-02 00:11:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/21012002/23001

[commit-bot: I haz the power, 2013-08-02 00:53:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-02 01:22:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/21012002/35001

[commit-bot: I haz the power, 2013-08-02 04:45:40]

Change committed as 155386

[Steve Block, 2013-08-06 03:57:00]

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/Act...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/Act...
Source/core/animation/ActiveAnimations.h:47: // Tracks the state of active CSS
Animations. When active the individual animations will
Not sure I understand this comment. If this tracks _active_ CSS animations,
won't they all appear in the default stack?

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.cpp:139: if (animationData->isDelaySet()
&& animationData->delay())
Do we check/force anywhere that delay() is non-negative?

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.h:55: bool isFiltered(const Player*
player) const { return m_filtered.contains(player); }
isFiltered() seems like a rather generic name. Can you make it more explicit or
add a comment?

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:765: {
Can we add an ASSERT to make clear that this is used only with WebAnimations?

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:779: keyframe->setOffset(offsets[j]);
It looks like we don't handle multiple keyframes at the same offset. CSS
requires us to use the last specified keyframe for that offset, whereas
KeyframeAnimationEffect allows multiple keyframes at the same offset and uses a
'step' behavior. So we need to remove duplicates here I think.

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:796: {
Can we add an ASSERT to make clear that this is used only with WebAnimations?

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1039: {
Can we add an ASSERT to make clear that this is used only with WebAnimations?

https://codereview.chromium.org/21012002/diff/35001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:1736:
setStyle(animation()->updateAnimations(this, style.get()));
Is this 'else' correct? Surely we never want to do this if
webAnimationsCSSEnabled() is true?

[Steve Block, 2013-08-06 03:59:37]

Was just reading through this now I'm back and think I've spotted a minor of
bugs (or at least the need for a FIXME).

[dstockwell, 2013-08-06 05:43:25]

Fixes in https://codereview.chromium.org/22364002/

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/Act...
File Source/core/animation/ActiveAnimations.h (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/Act...
Source/core/animation/ActiveAnimations.h:47: // Tracks the state of active CSS
Animations. When active the individual animations will
On 2013/08/06 03:57:01, Steve Block wrote:
> Not sure I understand this comment. If this tracks _active_ CSS animations,
> won't they all appear in the default stack?

Yes, they are always active. I'll simplify the comment.

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.cpp:139: if (animationData->isDelaySet()
&& animationData->delay())
On 2013/08/06 03:57:01, Steve Block wrote:
> Do we check/force anywhere that delay() is non-negative?

Good catch, this can be negative and will be hard to deal with. Added an assert
for now.

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.h:55: bool isFiltered(const Player*
player) const { return m_filtered.contains(player); }
On 2013/08/06 03:57:01, Steve Block wrote:
> isFiltered() seems like a rather generic name. Can you make it more explicit
or
> add a comment?

Don't have a better name, added a comment.

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:765: {
On 2013/08/06 03:57:01, Steve Block wrote:
> Can we add an ASSERT to make clear that this is used only with WebAnimations?

Done.

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:779: keyframe->setOffset(offsets[j]);
On 2013/08/06 03:57:01, Steve Block wrote:
> It looks like we don't handle multiple keyframes at the same offset. CSS
> requires us to use the last specified keyframe for that offset, whereas
> KeyframeAnimationEffect allows multiple keyframes at the same offset and uses
a
> 'step' behavior. So we need to remove duplicates here I think.

I was addressing this in: https://codereview.chromium.org/21779003/

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:796: {
On 2013/08/06 03:57:01, Steve Block wrote:
> Can we add an ASSERT to make clear that this is used only with WebAnimations?

Done.

https://codereview.chromium.org/21012002/diff/35001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1039: {
On 2013/08/06 03:57:01, Steve Block wrote:
> Can we add an ASSERT to make clear that this is used only with WebAnimations?

Done.

https://codereview.chromium.org/21012002/diff/35001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:1736:
setStyle(animation()->updateAnimations(this, style.get()));
On 2013/08/06 03:57:01, Steve Block wrote:
> Is this 'else' correct? Surely we never want to do this if
> webAnimationsCSSEnabled() is true?

Fixed.

[Mike Lawther (Google), 2013-08-06 06:01:10]

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/21012002/diff/35001/Source/core/animation/css...
Source/core/animation/css/CSSAnimations.cpp:139: if (animationData->isDelaySet()
&& animationData->delay())
On 2013/08/06 05:43:25, dstockwell wrote:
> On 2013/08/06 03:57:01, Steve Block wrote:
> > Do we check/force anywhere that delay() is non-negative?
> 
> Good catch, this can be negative and will be hard to deal with. Added an
assert
> for now.

It sounds like we'll want to deal with negative eventually? I usually like to
file a bug and link to it in a comment for
TODOs like this.