Bug 38963 – Animation keyframe timing functions are applying incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dean Jackson]

Created attachment 55795 [details]
manual test

The timing functions within animations are not applying correctly. See the testcase - I'm noticing a few errors:

- the tf rule on a 0%/from keyframe is applying to the whole animation, if no other keyframes specify tfs
- the tf rule on a 0%/from keyframe is NOT applying to that keyframe when the animation is iterating backwards

[Dean Jackson]

<rdar://problem/7966984>

[Dean Jackson]

Just making notes to myself here.

KeyframeAnimation::getKeyframeAnimationInterval()

const TimingFunction* timingFunction = 0;
    if (fromStyle->animations() && fromStyle->animations()->size() > 0) {

This means that:
eg. 
0% { animation-timing-function: linear; }
20% { animation-timing-function: ease-in; }

and you are currently at 10%, going forwards, then it should use the linear function (sort-of opposite of what transitions do)

and when you are at the same position going backwards, the same timing function (linear) applies. Even though technically the from and to have swapped, it's just that the animation is playing backwards, not that different rules apply.

[Dean Jackson]

This means part of my test case is wrong

[Dean Jackson]

The problem is that when we clone the RenderStyle for each keyframe, the AnimationList is new, but it still points to the same animation elements. This means that when a later keyframe sets the timing function, it overwrites the timing function for all previous keyframes.

I've attached two samples which show this in action.

Also, here is a patch to fix it. My answer was to make sure that there was a real copy done on AnimationList, rather than a shallow one. I'm not sure this is the best way - I couldn't find other places that trigger this code (only when applyDeclarations is called from within CSSStyleSelector::keyframeStylesForAnimation), so this isn't a review request. If it looks ok, I'll write a real regression test.

[Dean Jackson]

Created attachment 56753 [details]
example with HW anim

[Dean Jackson]

Created attachment 56754 [details]
Example with SW anim

[Dean Jackson]

Created attachment 56755 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 56755 [details]
patch

>  class AnimationList : public FastAllocBase {
>  public:
> +
> +    AnimationList() {}

Not sure if you need that.

Otherwise I think this patch is basically correct. I do wonder if m_animations should be a DataRef in StyleRareNonInheritedData though.

[Dean Jackson]

> >  class AnimationList : public FastAllocBase {
> >  public:
> > +
> > +    AnimationList() {}

> Not sure if you need that.

Yeah, I did. There was somewhere (can't remember) that used the default constructor. Once I added the copy constructor the compile failed because it wasn't generating the parameterless version.

[Adele Peterson]

Dean, you should post this for review someday.

[Simon Fraser (smfr)]

Created attachment 57977 [details]
Testcase

[Simon Fraser (smfr)]

Created attachment 57978 [details]
Testcase that should work, but doesn't

For some reason this testcase doesn't work with or without the fix. Weird.

[Simon Fraser (smfr)]

Created attachment 58531 [details]
Patch

[Simon Fraser (smfr)]

I was not able to reproduce the issue with the transform testcase.

[Darin Adler]

Comment on attachment 58531 [details]
Patch

> +    AnimationList() {}
> +    AnimationList(const AnimationList& o);

We normally put a space between the braces in "{}".

There’s no need for the argument name "o". (In the definition we prefer names to letters for arguments, so there other would be better then o.)

It’s best practice to also defined an assignment operator if you define a copy constructor. There are three ways you can deal with this:

    1) Declare a private assignment operator and do not define it.

    2) Use Noncopyable to do the above, but this will require a change to the copy constructor.

    3) Define a public assignment operator that uses swap (see <http://www.gotw.ca/gotw/059.htm>).

        AnimationList& operator=(const AnimationList& other)
        {
            AnimationList copy(other);
            swap(copy);
            return *this;
        }

If you do (3) we also need to write the swap function, but that is typically simple. I suggest doing either (1) or (3) rather than checking this in as-is.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/61068