Bug 37352 - Animations are cleared when stylesheets are dynamically inserted
Summary: Animations are cleared when stylesheets are dynamically inserted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-09 13:29 PDT by Chris Marrin
Modified: 2010-04-09 18:00 PDT (History)
0 users

See Also:


Attachments
testcase showing the problem (2.97 KB, application/zip)
2010-04-09 13:29 PDT, Chris Marrin
no flags Details
Patch with testcase (7.70 KB, patch)
2010-04-09 17:23 PDT, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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