Bug 37352

Summary: Animations are cleared when stylesheets are dynamically inserted
Product: WebKit Reporter: Chris Marrin <cmarrin>
Component: CSSAssignee: Chris Marrin <cmarrin>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
testcase showing the problem
none
Patch with testcase simon.fraser: review+

Description Chris Marrin 2010-04-09 13:29:29 PDT
Created attachment 52983 [details]
testcase showing the problem

We have a testcase where animations fail to start when you load a dynamic style sheet. Attached testcase shows the problem. It is timing related, so it rarely happens on the desktop, but it happens more often on embedded devices.

I've tracked this down to the fact that the animation is getting destroyed early, when style gets recalculated after loading the new style sheet. It's very timing sensitive and adding printfs to track it down makes it stop failing. But so far it looks like the keyframes are getting destroyed, so it decides to delete the animation since there are no keyframes.
Comment 1 Chris Marrin 2010-04-09 17:23:06 PDT
Created attachment 53018 [details]
Patch with testcase
Comment 2 Simon Fraser (smfr) 2010-04-09 17:30:11 PDT
Comment on attachment 53018 [details]
Patch with testcase

> Index: WebCore/dom/Document.cpp
> ===================================================================

>      recalcStyleSelector();
> +    // This recalcStyle initialtes a new recalc cycle. We need to bracket it to
> +    // make sure animations get the correct update time
> +    if (m_frame)
> +        m_frame->animation()->beginAnimationUpdate();
>      recalcStyle(Force);
> +    if (m_frame)
> +        m_frame->animation()->endAnimationUpdate();

I don't think the comment is necessary; the code speaks for itself.

> Index: WebCore/page/animation/KeyframeAnimation.cpp
> ===================================================================
> --- WebCore/page/animation/KeyframeAnimation.cpp	(revision 57355)
> +++ WebCore/page/animation/KeyframeAnimation.cpp	(working copy)
> @@ -68,6 +68,11 @@ void KeyframeAnimation::getKeyframeAnima
>      double elapsedTime = getElapsedTime();
>  
>      double t = m_animation->duration() ? (elapsedTime / m_animation->duration()) : 1;
> +
> +    ASSERT(t >= 0);
> +    if (t < 0)
> +        t = 0;

Or t = max(t, 0);

> Index: LayoutTests/animations/dynamic-stylesheet-loading.html
> ===================================================================
> +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> +  <meta name="viewport" content="width=device-width, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no">

No need for these meta tags.

> +  <title>Test of dynamic stylesheet loading</title>
> +  <link rel="stylesheet" href="resources/dynamic-stylesheet-insertion-main.css">
> +    <script src="animation-test-helpers.js" type="text/javascript" charset="utf-8"></script>
> +    <script type="text/javascript" charset="utf-8">
> +    
> +    const expectedValues = [
> +      // [animation-name, time, element-id, property, expected-value, tolerance]
> +      ["splashdown", 0.3, "splash", "webkitTransform.5", 100, 0.1],
> +    ];
> +    
> +    var controller = {};
> +
> +    controller.init = function () {
> +        // put a listener on the initial splash animation
> +        this.splash = document.getElementById('splash');
> +        this.splash.addEventListener('webkitAnimationStart', this, false);
> +        this.splash.addEventListener('webkitAnimationEnd', this, false);
> +
> +        runAnimationTest(expectedValues, undefined, undefined, true);
> +    };
> +
> +    controller.handleEvent = function (event) {
> +        if (event.type == 'webkitAnimationStart') {
> +            // pre-load the stylesheet
> +            var link = document.createElement('link');
> +            link.rel = 'stylesheet';
> +            link.href = 'resources/dynamic-stylesheet-insertion-inserted.css';
> +            document.head.appendChild(link);
> +        }
> +    };

I'd simplify this JS to not be objecty, and to make the names like 'splash' more generic.

> Index: LayoutTests/animations/resources/dynamic-stylesheet-insertion-inserted.css
> ===================================================================

Maybe add a comment saying "/* This file deliberately left blank */ ?

> Index: LayoutTests/animations/resources/dynamic-stylesheet-insertion-main.css
> ===================================================================
> --- LayoutTests/animations/resources/dynamic-stylesheet-insertion-main.css	(revision 0)
> +++ LayoutTests/animations/resources/dynamic-stylesheet-insertion-main.css	(revision 0)
> @@ -0,0 +1,37 @@
> +body {
> +  width: 320px;
> +  height: 480px;
> +}
> +
> +#splash {
> +  position: absolute;
> +  left: 20px;
> +  left: 60px;
> +  width: 100px;
> +  height: 100px;
> +  background-color: blue;
> +  -webkit-transform: translate3d(0,80px,0);
> +  -webkit-animation-name: splashdown;
> +  -webkit-animation-duration: 0.6s;
> +  -webkit-animation-delay: 0.1s;
> +}

Should remove rules that don't pertain to the issue, and pref. avoid rules that kick us into compositing mode.

r=me with more simplified testcase.
Comment 3 Chris Marrin 2010-04-09 18:00:45 PDT
Landed in r57390
Comment 4 Chris Marrin 2010-04-09 18:00:45 PDT
Landed in r57390