Bug 45325 – SVG Animate / Animate transform not animating after change via JS API

• Component: SVG
• Status: RESOLVED FIXED

[marek]

Created attachment 66774 [details]
Test case demonstarting what I wrote

Current Webkit (or at least latest official releases of both Chrome/Safari) suffers from issue,
that when we want to restart some SMIL animation again and again via JS API  elem.beginElement(),
it works only as long we're not touching/modifying animation attributes.
If we do so, it is somewhat invalidated and refuses to start again (under new conditions).
The only workaround to this is to remove original element, create fresh new one and append it instead.
But I consider it bug and prepared testcase showing it - both elements should be animated again and again,
and it works so in Presto/Gecko, but is broken in Webkit and only not changed element (circle) continues in its restarted animations.

Versions: Chrome 6.0.472.53
Safari: 5.0 7533.16

[Robin Qiu]

I think I have found the root cause.

When set attr, this will be called: 
void SVGAnimationElement::attributeChanged(Attribute* attr, bool preserveDecls)
{
    // Assumptions may not hold after an attribute change.
    m_animationValid = false;
    SVGSMILElement::attributeChanged(attr, preserveDecls);
}

m_animationValid is only changed in void SVGAnimationElement::startedActiveInterval(), and this function is only called once at animation start.

So, if any attrubite is changed after an animation get started, m_animationValid will always be false, as a result, SVGAnimationElement::updateAnimation() will return immediately.

I changed 
    m_animationValid = false;
to
    // m_animationValid = false;
and then, this test case can pass. Certainly, this is just a test.

I would like to listen to some comments on how to fix this bug. Add a flag such as m_attrChanged and re-calculate the m_animationValid in updateAnimation()? or modify any code in SVGSMILElement::progress()? 

Any comment is welcome.

[Robin Qiu]

Created attachment 72801 [details]
Patch v1

Tested, no regression. The behavior similar to Opera now.

But I don't know how to write a test case for this patch, because the "animVal" always returns orginal value even after attribute is changed.

[Nikolas Zimmermann]

(In reply to comment #2)
> Created an attachment (id=72801) [details]
> Patch v1
> 
> Tested, no regression. The behavior similar to Opera now.
> 
> But I don't know how to write a test case for this patch, because the "animVal" always returns orginal value even after attribute is changed.

We have no animVal support, so you might want to use the animation snapshot API to test this.
See the existing testcases in svg/animations/ that make us of it.

I think you want to declare a SMIL animation element, then change an animation attribute dynamically, then call beginElement. Ideally you'd test eg. a color transition with dur="1s" from red to green, and snapshot the animation values at 0s, 0.5s and 1s.

Without your patch the animation wouldn't run, and no change in baseVal would be visible.
(Note: that we're currently changing the baseVal, not the animVal, so this is going to work, as long as we have no animVal support, and then we'll need to revisit the animation testcases again anyway).

Can you give that a try?

[Robin Qiu]

Created attachment 72802 [details]
The test case which doesn't work.

I copied and modified LayoutTests/svg/animations/script-tests/animVal-basics.js to test this patch, but it doesn't work.
Changes only are:
17c16
< animate.setAttribute("to", "100");
---
> animate.setAttribute("to", "200");
72a72
> window.setTimeout('animate.setAttribute("to", "100");', 1000);

[Robin Qiu]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=72801) [details] [details]
> > Patch v1
> > 
> > Tested, no regression. The behavior similar to Opera now.
> > 
> > But I don't know how to write a test case for this patch, because the "animVal" always returns orginal value even after attribute is changed.
> 
> We have no animVal support, so you might want to use the animation snapshot API to test this.
> See the existing testcases in svg/animations/ that make us of it.
> 
> I think you want to declare a SMIL animation element, then change an animation attribute dynamically, then call beginElement. Ideally you'd test eg. a color transition with dur="1s" from red to green, and snapshot the animation values at 0s, 0.5s and 1s.
> 
> Without your patch the animation wouldn't run, and no change in baseVal would be visible.
> (Note: that we're currently changing the baseVal, not the animVal, so this is going to work, as long as we have no animVal support, and then we'll need to revisit the animation testcases again anyway).
> 
> Can you give that a try?

Thanks for your so quick reply!
I'll do that right now. :)

[Robin Qiu]

Created attachment 72809 [details]
Add test case. V2.

Add a test case.

[Nikolas Zimmermann]

Comment on attachment 72809 [details]
Add test case. V2.

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

> LayoutTests/svg/animations/script-tests/animate-set-attribute-frozen.js:1
> +description("Animation should not be frozen after setAttrubite() and the new attribute should be applied.");

Typo: setAttribute.

> LayoutTests/svg/animations/script-tests/animate-set-attribute-frozen.js:30
> +// Setup animation test
> +function expectDarkBlue() {
> +    // Check half-time conditions
> +    shouldBeFalse("rect.style.fill == '#0000FF'");
> +    shouldBe("rect.style.fill.toString().substring(0,5)", "'#0000'");
> +    completeTest();
> +}
> +
> +window.setTimeout("triggerUpdate(50, 50)", 0);
> +window.setTimeout('animate.setAttribute("to", "#000000");', 1000);
> +window.setTimeout('expectDarkBlue()', 1500);

This is not correct.
You should use the snapshot API, which is capable of giving you the current DOM at a specified time.
See LayoutTests/svg/animations/script-tests/animate-color-transparent.js as example.

I'd add an "prepareTest()" method, which does:
function prepareTest() {
    animate.setAttribute("to", "#00000");
    window.setTimeout(executeTest, 0);
}
and use
// Begin test async
rect.setAttribute("onclick", "prepareTest()");

instead of

// Begin test async
rect.setAttribute("onclick", "executeTest()");

That should give you a reliable way of testing this, which is not timing dependant.

[Robin Qiu]

Created attachment 73048 [details]
thistest case doesn't work either

Both original code and patched code can pass this test case.
I debugged webkit:

(gdb) bt
#0  WebCore::SMILTimeContainer::updateAnimations (this=0x81748e8, elapsed=...) at ../../../WebCore/svg/animation/SMILTimeContainer.cpp:313                                                                                                                                    
#1  0xb65b5486 in WebCore::SMILTimeContainer::sampleAnimationAtTime (this=0x81748e8, elementId=..., newTime=1.5) at ../../../WebCore/svg/animation/SMILTimeContainer.cpp:222                                                                                                  
#2  0xb65179fb in WebCore::SVGDocumentExtensions::sampleAnimationAtTime (this=0x820bc38, elementId=..., element=0x820d1d8, time=1.5) at ../../../WebCore/svg/SVGDocumentExtensions.cpp:129                                                                                    
#3  0xb638ce15 in DumpRenderTreeSupportQt::pauseSVGAnimation (frame=0x819d350, animationId=..., time=1.5, elementId=...) at ../../../WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:253                                                                                 
#4  0x08076ab8 in LayoutTestController::sampleSVGAnimationForElementAtTime (this=0x81cc170, animationId=..., time=1.5, elementId=...) at /home/robin/projects/WebKit/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:505    

in WebCore::SMILTimeContainer::updateAnimations():
Line 260:           for (unsigned n = 0; n < toAnimate.size(); ++n) {
toAnimate is always empty, so  
Line 285:         animation->progress(elapsed, resultElement); 
never be called.

[Nikolas Zimmermann]

Created attachment 73049 [details]
This testcase doesn't work in Safari but in Opera

Robin, I think this testcase demonstrates the problem. I commened what I changed compared to your last testcase.
Can you verify that it works with your patch?

[Robin Qiu]

This bug is fixed in ffa063e516cc9bccc1ca0dab620e0304291f3d9d, bug 26019.

[Nikolas Zimmermann]

Closing bug, its fixed and we have test coverage for it.