Bug 85929 - regression(111639): Issue with simultaneous CSS animations
Summary: regression(111639): Issue with simultaneous CSS animations
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Nobody
URL: http://samvermette.s3.amazonaws.com/s...
Keywords: InRadar, Regression
Depends on:
Reported: 2012-05-08 16:33 PDT by Simon Fraser (smfr)
Modified: 2012-05-15 21:48 PDT (History)
6 users (show)

See Also:

Testcase (2.47 KB, text/html)
2012-05-08 16:39 PDT, Dean Jackson
no flags Details
Patch V1. (8.77 KB, patch)
2012-05-14 17:37 PDT, Igor Trindade Oliveira
dino: review+
Details | Formatted Diff | Diff
Final patch (8.67 KB, patch)
2012-05-15 13:07 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2012-05-08 16:33:54 PDT
When there's an ongoing CSS animation with its iteration loop set to "infinite", adding a second CSS animation with "webkit-animation-fill-mode" set to "forwards" will always wait for the first animation's cycle to finish before starting to animate.

Steps to Reproduce
I have created a page that reproduces the issue here: http://samvermette.s3.amazonaws.com/safari.html

Expected Results
The blue square should always animate as soon as I hover the link.

Actual results
Except for the first time you hover the link, the blue square will always wait for the red square to finish its animation cycle before starting to animate.

Removing the "webkit-animation-fill-mode" property seems to fix the issue.
Comment 1 Simon Fraser (smfr) 2012-05-08 16:34:04 PDT
Comment 2 Simon Fraser (smfr) 2012-05-08 16:34:38 PDT
Igor, please take a look.
Comment 3 Dean Jackson 2012-05-08 16:37:39 PDT
Regressed with http://trac.webkit.org/changeset/111639/
Comment 4 Dean Jackson 2012-05-08 16:39:37 PDT
Created attachment 140814 [details]
Comment 5 Igor Trindade Oliveira 2012-05-09 08:02:18 PDT
Comment 6 Igor Trindade Oliveira 2012-05-11 19:12:23 PDT
After some time debugging i am removing the regression title because:

* I reverted http://trac.webkit.org/changeset/111639/ and the bug still there
* The test works when i run the test *without* accelerated composite;
* I tried to find one changeset where the bug does not happen, so i reverted my local branch for 3 months ago, and the bug already was there.
Comment 7 Igor Trindade Oliveira 2012-05-14 17:36:59 PDT
My bad. It is a regression, however in Qt and GTK, AC had a bug(fixed in the changeset http://trac.webkit.org/changeset/116899) and reverting the changeset http://trac.webkit.org/changeset/111639/, the bug still was there.
Comment 8 Igor Trindade Oliveira 2012-05-14 17:37:25 PDT
Created attachment 141827 [details]
Patch V1.

Proposed patch.
Comment 9 Dean Jackson 2012-05-14 18:27:50 PDT
Comment on attachment 141827 [details]
Patch V1.

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

> LayoutTests/animations/fill-mode-forwards.html:12
> +    function addPopAnimation () {

Space between function name and ()

> LayoutTests/animations/fill-mode-forwards.html:15
> +        popAnimationCounter = popAnimationCounter + 1;

Just popAnimationCounter++

> LayoutTests/animations/fill-mode-forwards.html:18
> +    function removePopAnimation () {

Ditto on the space.

> LayoutTests/animations/fill-mode-forwards.html:93
> +  <div id="pulsing-square" onwebkitanimationiteration="setTimeout(addPopAnimation(), 50);"
> +       onwebkitanimationend="pulsingSquareEnded()"
> +       ></div>

I think you can move the ></div> to the previous line.

> Source/WebCore/ChangeLog:10
> +        Currently, previousTimeToNextService is just saving the previous CompositeAnimation::timeToNextService
> +        for AnimationControllerPrivate::updateAnimationTimerForRenderer, however CompositeAnimation::timeToNextService
> +        is also called and used by AnimationControllerPrivate::updateAnimationTimer and we
> +        are not saving the previousTimeToNextService for AnimationControllerPrivate::updateAnimationTimer
> +        doing AnimationController have unexpected behavior.

This last line is not correct English. I think you could probably just change the bit after "used by AnimationControllerPrivate::updateAnimationTimer" to be "updateAnimationTimer. Make sure we save the existing timeToNextService from both places, updateAnimationTimerForRenderer and updateAnimationTimer." I don't think you need the class names (e.g. AnimationControllerPrivate::) here.
Comment 10 Igor Trindade Oliveira 2012-05-15 13:07:18 PDT
Created attachment 142038 [details]
Final patch
Comment 11 WebKit Review Bot 2012-05-15 21:48:33 PDT
Comment on attachment 142038 [details]
Final patch

Clearing flags on attachment: 142038

Committed r117216: <http://trac.webkit.org/changeset/117216>
Comment 12 WebKit Review Bot 2012-05-15 21:48:38 PDT
All reviewed patches have been landed.  Closing bug.