Bug 37352 – Animations are cleared when stylesheets are dynamically inserted

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Marrin]

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.

[Chris Marrin]

Created attachment 53018 [details]
Patch with testcase

[Simon Fraser (smfr)]

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.

[Chris Marrin]

Landed in r57390

[Chris Marrin]

Landed in r57390