Bug 187839 – Hardware fill-forwards animation and transitions don't interact correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Simon Fraser (smfr)]

Created attachment 345415 [details]
Testcase.html

If you have a fill-forwards transform animation, and a transform transition on the same element, then we'll show the transition when we should not. See attached test case.

Does not happen with software animations.

[Radar WebKit Bug Importer]

<rdar://problem/42410412>

[Antoine Quint]

Our behavior with the old engine differs from what Firefox and Chrome do where hovering the moving element does not alter its left-to-right animation. This is not fixed with the new engine either.

[Antoine Quint]

Created attachment 389497 [details]
Another non-interactive test case

[Antoine Quint]

The issue here is that we apply accelerated effects regardless of whether they are at the top of the effect stack for a given property. I think the easiest way to fix this is to only run an accelerated effect if it is at the top of the effect stack for all of its target properties.

[Antoine Quint]

There may however be another orthogonal issue: whether the transition should be started at all. Currently, this test runs differently in Safari, Chrome and Firefox.

In Safari, this starts a transition from `transform: none` to `transform: translate(100px,100px)`. Safari is only considering unanimated style values.

In Firefox, this starts a transition from `transform: none` to `transform: translateX(25px)`. Firefox is considering the animated value.

In Chrome, no transition starts, I’m not sure why because the current style is definitely different from the style during the previous style change, animated or otherwise.

Selectively quoting from the CSS Transitions spec (https://www.w3.org/TR/css-transitions-1/#starting):

"Otherwise, define the before-change style as the computed values of all properties on the element as of the previous style change event, except with any styles derived from declarative animations such as CSS Transitions, CSS Animations, and SMIL Animations updated to the current time."

My reading of this is that the before-change style at ~500ms should be the value set by the currently-running CSS Animation at ~500ms, so `transform: translateX(25px)`.

"Likewise, define the after-change style as the computed values of all properties on the element based on the information known at the start of that style change event, but using the computed values of the animation-* properties from the before-change style, excluding any styles from CSS Transitions in the computation, and inheriting from the after-change style of the parent."

This sentence really did my head in :) I think it means that the after-change style is also set by the currently-running CSS Animation at ~500ms, so `transform: translateX(25px)` as well.

This would lead me to believe that Chrome has the correct behavior here and that there is no CSS Transition started since both the before-change and after-change styles have the same value for `transform`.

[Antoine Quint]

Created attachment 400202 [details]
Patch

[Antoine Quint]

Created attachment 400204 [details]
Patch

[Antoine Quint]

So it seems only the new test fails with this patch. I’ll find a way to fix this and send a patch for review tomorrow.

[Antoine Quint]

Created attachment 400256 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 400256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400256&action=review

> Source/WebCore/animation/AnimationTimeline.cpp:389
> +        if (animation && animation->isRelevant()) {

isRelevant() is so vague. Relevant to whom?

> Source/WebCore/animation/ElementAnimationRareData.h:55
> +    void setLastStyleChangeEventStyle(std::unique_ptr<const RenderStyle> style) { m_lastStyleChangeEventStyle = WTFMove(style); }

std::unique_ptr<const RenderStyle>&&

> Source/WebCore/dom/Element.h:506
> +    void setLastStyleChangeEventStyle(std::unique_ptr<const RenderStyle>);

std::unique_ptr<const RenderStyle>&&

> LayoutTests/webanimations/updating-property-targeted-by-css-transition-during-css-animation.html:4
> +<style type="text/css" media="screen">

Just <style>

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 400256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400256&action=review
> 
> > Source/WebCore/animation/AnimationTimeline.cpp:389
> > +        if (animation && animation->isRelevant()) {
> 
> isRelevant() is so vague. Relevant to whom?

This is a term defined by the Web Animations spec, see https://drafts.csswg.org/web-animations-1/#relevant-animation.

I'll fix all of the other comments in the commit.

[Antoine Quint]

Committed r262154: <https://trac.webkit.org/changeset/262154>