Bug 150328 – REGRESSION (r187121): Multiple-keyframe animations not honouring ' forwards' fill-mode

• Component: Animations
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

Created attachment 263477 [details]
Reduced test case

DESCRIPTION

In recent versions of WebKit, if a CSS animation is defined with animation-fill-mode = forwards AND at least 3 or more different keyframes **where at least one falls in the [51% - 100%] range**, the end result is some random state that does not match the values of neither the 0% nor the 100% keyframe.

Curiously enough, if all the keyframes but the 100% fall in the [0% - 50%] interval, then the animation is finished correctly, and the final result after it's ended is that the values of the 100% keyframe are kept.

I initially found this in WebKitGTK 2.10.1, but then I've tried in my Mac Mini with OS X Yosemite and I could reproduce it in the latest WebKit nightly, so it looks like a cross platform regression.

More specifically, in OS X:
  * Works OK with Safari 9.0 (10601.1.56.2)
  * FAILS with WebKit Nightly r191175

I'm attaching a test case I wrote, so that the bug can be easily reproduceable.


STEPS TO REPRODUCE

  1. Uncompress the contents of the attached file and load index.html in MiniBrowser / Safari + WebKit Nightly 
  2. You should a block of text with a blue-ish background moving around the horizontal axis:
    + Start at coordinates (400, 0), moving towards the left until reaching (0, 0) at the 25% keyframe
    + Move towards the right until reaching (250, 0) at the 51% keyframe
    + Move towards the left until reaching (100,0) at the 100% keyframe
    + Stay at (100, 0) once the animation has finished


EXPECTED OUTCOME

After the animation has finished, the moving block remains quiet at (100, 0) as per the 'forwards' value of animation-fill-mode, which ensures the values of the 100% keyframe are brought out of the animation.


ACTUAL OUTCOME

After the animation has finished, the moving block moves to some (apparently) random spot in the horizontal axis, which does not seem to much neither the 0% nor the 100% keyframes.

Also, note that if you modify the test case so that the definition of the 51% keyframe because the 50% keyframe, then the animation is processed correctly and the moving block is left at its right position once it's finished.

[Mario Sanchez Prada]

I found that the patch for WebKit bug 146996 ("Safari mis-applies "animation-fill-mode: forwards" when using fractional iteration count") is the one who broke this behaviour: http://trac.webkit.org/changeset/187121

I've double checked it myself and I can confirm that reverting that patch gets the test case running fine once again. Simon, any idea?

[Radar WebKit Bug Importer]

<rdar://problem/23175069>

[Simon Fraser (smfr)]

Created attachment 263896 [details]
Better testcase

[Simon Fraser (smfr)]

Created attachment 263899 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 263899 [details]
Patch

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

The patch looks good to me, although someone more knowledgeable of this code should better review it.

My only doubt is about the test case added, which I needed to modify slightly to be able reproduce the test locally, see my comments below...

> Source/WebCore/ChangeLog:7
> +        

Nit. Extra trailing spaces in empty line.

> LayoutTests/ChangeLog:5
> +

Nit. Extra trailing spaces in empty line.

> LayoutTests/ChangeLog:7
> +        

Ditto

> LayoutTests/animations/fill-forwards-end-state.html:14
> +      animation: anim1 1.5s 0.2s linear;

I tested this layout test in my laptop now I can't see any difference with or without the patch, even though it DOES make a difference for my original test case and your better test case.

After some fiddling, I found that making the animation a bit longer (2s instead of 1.5s) would make me reproduce the bug without your patch, so I was wondering if you could perhaps incorporate this change in the test?

In any case, that's strange and perhaps exposes another bug, but I'm not 100% sure yet. In any case, this is what I did to be able to reproduce the problem using your layout test without the rest of the patch:

-      animation: anim1 1.5s 0.2s linear;
+      animation: anim1 2s 0.2s linear;

> LayoutTests/animations/fill-forwards-end-state.html:28
> +      ["anim1", 2, "box1", "left", 100, 2],

This needs to be adjusted if you follow my suggestion on increasing the animation duration to something like this:

-      ["anim1", 2, "box1", "left", 100, 2],
+      ["anim1", 2.5, "box1", "left", 100, 2],

[Simon Fraser (smfr)]

Any duration > 1 should show the problem.

[Dean Jackson]

Comment on attachment 263899 [details]
Patch

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

>> LayoutTests/animations/fill-forwards-end-state.html:14
>> +      animation: anim1 1.5s 0.2s linear;
> 
> I tested this layout test in my laptop now I can't see any difference with or without the patch, even though it DOES make a difference for my original test case and your better test case.
> 
> After some fiddling, I found that making the animation a bit longer (2s instead of 1.5s) would make me reproduce the bug without your patch, so I was wondering if you could perhaps incorporate this change in the test?
> 
> In any case, that's strange and perhaps exposes another bug, but I'm not 100% sure yet. In any case, this is what I did to be able to reproduce the problem using your layout test without the rest of the patch:
> 
> -      animation: anim1 1.5s 0.2s linear;
> +      animation: anim1 2s 0.2s linear;

Sounds like a good suggestion.

[Simon Fraser (smfr)]

Comment on attachment 263899 [details]
Patch

Actually I'll fix the test as recommended.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/191502

[Alexey Proskuryakov]

animations/fill-forwards-end-state.html fails every time after r191502:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=animations%2Ffill-forwards-end-state.html

[Simon Fraser (smfr)]

The expected result just needs a number to be changed.

[Ryan Haddad]

Reopening to attach new patch.

[Ryan Haddad]

Created attachment 263949 [details]
Patch

Patch to update expected result.

[WebKit Commit Bot]

Comment on attachment 263949 [details]
Patch

Clearing flags on attachment: 263949

Committed r191516: <http://trac.webkit.org/changeset/191516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.