Bug 150328 - REGRESSION (r187121): Multiple-keyframe animations not honouring ' forwards' fill-mode
Summary: REGRESSION (r187121): Multiple-keyframe animations not honouring ' forwards' ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified All
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-10-19 06:23 PDT by Mario Sanchez Prada
Modified: 2015-10-24 16:58 PDT (History)
11 users (show)

See Also:


Attachments
Reduced test case (908 bytes, text/html)
2015-10-19 06:23 PDT, Mario Sanchez Prada
no flags Details
Better testcase (658 bytes, text/html)
2015-10-22 21:44 PDT, Simon Fraser (smfr)
no flags Details
Patch (5.52 KB, patch)
2015-10-22 22:10 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (1.24 KB, patch)
2015-10-23 15:12 PDT, Ryan Haddad
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2015-10-19 06:23:33 PDT
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.
Comment 1 Mario Sanchez Prada 2015-10-19 16:16:42 PDT
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?
Comment 2 Radar WebKit Bug Importer 2015-10-19 16:17:34 PDT
<rdar://problem/23175069>
Comment 3 Simon Fraser (smfr) 2015-10-22 21:44:36 PDT
Created attachment 263896 [details]
Better testcase
Comment 4 Simon Fraser (smfr) 2015-10-22 22:10:10 PDT
Created attachment 263899 [details]
Patch
Comment 5 Mario Sanchez Prada 2015-10-23 08:14:01 PDT
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],
Comment 6 Simon Fraser (smfr) 2015-10-23 11:19:40 PDT
Any duration > 1 should show the problem.
Comment 7 Dean Jackson 2015-10-23 11:24:06 PDT
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.
Comment 8 Simon Fraser (smfr) 2015-10-23 11:26:21 PDT
Comment on attachment 263899 [details]
Patch

Actually I'll fix the test as recommended.
Comment 9 Simon Fraser (smfr) 2015-10-23 11:32:34 PDT
https://trac.webkit.org/changeset/191502
Comment 10 Alexey Proskuryakov 2015-10-23 13:36:16 PDT
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
Comment 11 Simon Fraser (smfr) 2015-10-23 14:37:01 PDT
The expected result just needs a number to be changed.
Comment 12 Ryan Haddad 2015-10-23 15:12:15 PDT
Reopening to attach new patch.
Comment 13 Ryan Haddad 2015-10-23 15:12:19 PDT
Created attachment 263949 [details]
Patch

Patch to update expected result.
Comment 14 WebKit Commit Bot 2015-10-23 16:08:01 PDT
Comment on attachment 263949 [details]
Patch

Clearing flags on attachment: 263949

Committed r191516: <http://trac.webkit.org/changeset/191516>
Comment 15 WebKit Commit Bot 2015-10-23 16:08:09 PDT
All reviewed patches have been landed.  Closing bug.