Bug 38963 - Animation keyframe timing functions are applying incorrectly
Summary: Animation keyframe timing functions are applying incorrectly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-05-11 18:56 PDT by Dean Jackson
Modified: 2010-06-12 15:17 PDT (History)
4 users (show)

See Also:


Attachments
manual test (2.88 KB, text/html)
2010-05-11 18:56 PDT, Dean Jackson
no flags Details
example with HW anim (2.99 KB, text/html)
2010-05-21 15:25 PDT, Dean Jackson
no flags Details
Example with SW anim (2.44 KB, text/html)
2010-05-21 15:26 PDT, Dean Jackson
no flags Details
patch (1.77 KB, patch)
2010-05-21 15:27 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Testcase (1.47 KB, text/html)
2010-06-06 10:50 PDT, Simon Fraser (smfr)
no flags Details
Testcase that should work, but doesn't (1.58 KB, text/html)
2010-06-06 10:57 PDT, Simon Fraser (smfr)
no flags Details
Patch (8.36 KB, patch)
2010-06-11 17:38 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2010-05-11 18:56:27 PDT
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
Comment 1 Dean Jackson 2010-05-12 03:40:11 PDT
<rdar://problem/7966984>
Comment 2 Dean Jackson 2010-05-12 17:50:57 PDT
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.
Comment 3 Dean Jackson 2010-05-12 17:55:01 PDT
This means part of my test case is wrong
Comment 4 Dean Jackson 2010-05-21 15:25:23 PDT
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.
Comment 5 Dean Jackson 2010-05-21 15:25:53 PDT
Created attachment 56753 [details]
example with HW anim
Comment 6 Dean Jackson 2010-05-21 15:26:18 PDT
Created attachment 56754 [details]
Example with SW anim
Comment 7 Dean Jackson 2010-05-21 15:27:02 PDT
Created attachment 56755 [details]
patch
Comment 8 Simon Fraser (smfr) 2010-05-21 16:02:54 PDT
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.
Comment 9 Dean Jackson 2010-05-23 05:16:36 PDT
> >  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.
Comment 10 Adele Peterson 2010-05-27 15:39:51 PDT
Dean, you should post this for review someday.
Comment 11 Simon Fraser (smfr) 2010-06-06 10:50:11 PDT
Created attachment 57977 [details]
Testcase
Comment 12 Simon Fraser (smfr) 2010-06-06 10:57:23 PDT
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.
Comment 13 Simon Fraser (smfr) 2010-06-11 17:38:12 PDT
Created attachment 58531 [details]
Patch
Comment 14 Simon Fraser (smfr) 2010-06-11 17:38:52 PDT
I was not able to reproduce the issue with the transform testcase.
Comment 15 Darin Adler 2010-06-11 18:49:49 PDT
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.
Comment 16 Simon Fraser (smfr) 2010-06-12 15:17:24 PDT
http://trac.webkit.org/changeset/61068