Bug 22870 – Animations and Transitions started in the same recalcStyle() cycle should have identical start times

• Component: CSS
• Status: RESOLVED FIXED

[Chris Marrin]

Currently the start times animations get is set by a call to currentTime() at the moment the animation is beng activated. So animations that should be started at the same time have slightly different start times. It's possible for some types of animations (rotations for instance) to therefore be slightly off from each other. It also makes animation events not fire at quite the same time. 

This can be fixed by registering an animation time value at the start of recalcStyle() and using that for all animations and transitions that start in that cycle.

[Chris Marrin]

Created attachment 26035 [details]
Patch, including LayoutTest files

        Fixed https://bugs.webkit.org/show_bug.cgi?id=22870
 
        I added a recalcStyleTime value to Document. This is updated
        at the start of the document's recalcStyle cycle and also
        when the document starts loading, since animations are started
        then as well. Then I always use that value when setting animation
        start times, so all animations started in the same cycle have the 
        same start time.

        The test cases checked in test this, but in the case of the 'left'
        test it actually doesn't make any difference in most cases. This is
        because values are clamped to whole pixels, so the start times would 
        have to be pretty far off for the test to fail using the old
        currentTime() model. Still, under really heavy load, it's possible for
        the test to fail without these changes.

        The 'transform' test is another story. It animates to the full resolution
        of a floating point number, so the test fails miserably without this
        fix.

[Dave Hyatt]

Comment on attachment 26035 [details]
Patch, including LayoutTest files

+    m_styleRecalcStartTime = -1;

A named constant would be nice here instead of -1.

+    if (m_styleRecalcStartTime < 0)
+        const_cast<Document*>(this)->m_styleRecalcStartTime = currentTime();

Just make m_styleRecalcStartTime mutable.  I  think 

if (m_styleRecalcStartTime == cRecalcStyleTimeNotSet) 

will be more readable also (once you add the named constant).

+                if (m_startTime <= 0) {
                     m_startTime = param;
+                }

These brackets shouldn't be there.

I do not understand why you would reset the m_styleRecalcStartTime when parsing is finished.  That seems unnecessary.  Finishing parsing is a pretty irrelevant milestone as far as the page rendering (the user may have been interacting with the page for a long time while it's still parsing in fact).  I think you should just remove that code.

[Chris Marrin]

Created attachment 26161 [details]
Replacement patch

This fixes the issues identified and does the implementation a bit differently. Rather than maintaining a time value in Document, it is now in AnimationController. Document now makes begin/end calls to AnimationController at the start and end of recalcStyle. Right now, the time value is reset in the begin call and the end call does nothing. But it's there to keep things symmetrical. And I will be needing the end call in the future for some of the sync logic yet to do.

[Dave Hyatt]

Comment on attachment 26161 [details]
Replacement patch

I like this patch better than the previous one.  Nice.

r=me

[Chris Marrin]

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/animations/animation-test-helpers.js
Adding         LayoutTests/animations/simultaneous-start-left-expected.txt
Adding         LayoutTests/animations/simultaneous-start-left.html
Adding         LayoutTests/animations/simultaneous-start-transform-expected.txt
Adding         LayoutTests/animations/simultaneous-start-transform.html
Sending        WebCore/ChangeLog
Sending        WebCore/dom/Document.cpp
Sending        WebCore/page/animation/AnimationBase.cpp
Sending        WebCore/page/animation/AnimationBase.h
Sending        WebCore/page/animation/AnimationController.cpp
Sending        WebCore/page/animation/AnimationController.h
Sending        WebCore/page/animation/KeyframeAnimation.cpp
Transmitting file data .............
Committed revision 39667.