Bug 43452 – SVG animation beginElement() does not restart a stopped animation.

• Component: SVG
• Status: RESOLVED FIXED

[mg]

The scenario is:

1) An animation is started on document load using "animate" or "animateTransform". 

2) If the animation is running, "beginElement()" will restart the animation.

3) If the animation is running, "endElement()" will stop the animation.

4) If the animation is stopped, "beginElement()" will NOT restart the animation. This is the problem. 

5) As an additional problem, with "animateTransform" neither "beginElement()" nor "endElement()" seem to have any effect. This seems to be a closely related problem and so is included in this report.


This behaviour is consistent across all versions of Webkit browsers tested, including the following:

On Linux:
- Chromium 6.0.453.0 (51332) Built on Ubuntu 9.04, running on Ubuntu 10.04
- Epiphany Web Browser 2.30.2
- Midori 0.2.2 GTK+ 2.19.6, WebKitGTK+ 1.1.21

On MS windows:
- Google Chrome 5.0.375.125
- Safari 5.0.1 (7533.17.8)


This feature *does* work as expected (stopping and restarting) on the following browsers:
On Linux (64 bit Ubuntu 10.04):
- Firefox 4 beta (4.03b3pre)
- Opera 10.60 Build 6386

On MS Windows XP:
- Opera 10.60 Build 3445
- (Firefox was not tested on this platform). 


The application area for this is starting and stopping animations on demand. This would be required for any applications where the animations were used for any purpose other than page load decorations. Animations are specified as ordinary SVG XML in the web page rather than through DOM scripting to allow the easy creation of complex drawings using clip art via a drawing editor such as Inkscape. "beginElement()" and "endElement()" are the only practical means of re-starting and stopping animations in this situation. I have planned applications for these features now that all the modern browsers support animation (Firefox has just added animation in version 4). However, this is not a critical bug for me as I can tell users to either not use the new animation features, or else to use Firefox or Opera until the bug is fixed in Chrome and/or Safari. 


An SVG file with six test cases is provided. Each test case is labelled. The animations start on load and use Javascript to start or stop. Click on a green area to stop an animation. Click on a red area to re-start an animation. If you click on a running animation you will see that it will restart (some test cases are easier to see than others for this). The exception of course is the "animateMotion" test which will not stop or restart.

All tests work correctly in Opera or Firefox 4 (beta version). If you are using one of these browsers, a stopped animation will restart, and "animateMotion" behaves the same as "animate" and "animateTransform".

This report does not appear to be directly related to Bug 34301 - "Creating <animateMotion> elements via javascript do not execute". The test case provided in that report appears to run correctly when I tested it on Chromium 6.0.453.0 on Linux, whereas the test cases provided here do not run on the browsers listed above. (Bug 34301 is still marked as "new", but the test case seems to work Ok for me on Chromium 6.0.453.0 (51332)).

[mg]

Created attachment 63395 [details]
SVG file with 6 test cases.

The attachment didn't appear to go through on the first attempt for some reason.

[Felician Marton]

Created attachment 94602 [details]
Bugfix for SVG animation restarting.

[Rob Buis]

Comment on attachment 94602 [details]
Bugfix for SVG animation restarting.

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

Great that you are working on this! I am r- because of my suggestions and questions.

> LayoutTests/ChangeLog:11
> +        Added test for animation beginElement. It should restart the animation even if the animation is stopped by endElement() previously.

This line should move up to be below the 2 bugfix lines

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:56
> +button.setAttribute("onclick", "executeTest()");

Can you explain why button is needed? If not, can you remove it and put the onclick on the rect?

> Source/WebCore/ChangeLog:11
> +        (WebCore::SVGSMILElement::findInstanceTime): changed the return value when we search in the endTimes vector and there is no proper match.

s/changed/Changed

[Felician Marton]

Created attachment 95170 [details]
Bugfix for SVG animation restarting. Updated Patch.

[Nikolas Zimmermann]

Comment on attachment 95170 [details]
Bugfix for SVG animation restarting. Updated Patch.

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

Test is still flakey, needs another iteration: r-:

> LayoutTests/ChangeLog:7
> +        https://bugs.webkit.org/show_bug.cgi?id=43452
> +        Added test for animation beginElement. It should restart the animation even if the animation is stopped by endElement() previously.

You should leave a blank line below the bug link.

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:10
> +rect.setAttribute("fill", "pink");

Please use green here not pink ;-)

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:43
> +    //Timeout needed, because te SVG animation mechanism is fully time based.
> +    //If animation actions performed in a very short time, the actions will not behave as expected.
> +    setTimeout("animateX.beginElement()",30);
> +    setTimeout("animateX.endElement()",60);
> +    setTimeout("animateX.beginElement(), checkPosition()",90);

This hack is not acceptable, it makes the test timing dependant, and likely flakey. What you want to use is:

function beginAnimation() {
    animateX.beginElement();
    setTimeout(restartAnimation, 0);
}

function restartAnimation() {
    animateX.endElement();
    setTimeout(function() {
        animateX.beginElement();
        checkPosition();
    }, 0);
}

function executeTest() {
    setTimeout(beginElement, 0);
}

That looks cleaner, right?

> Source/WebCore/ChangeLog:5
> +        Bugfix: SVG animation beginElement() does not restart the animation after endElement().

This is not the real bug title, it should always match the bugzilla title, so remove the "Bugfix: ".

> Source/WebCore/ChangeLog:9
> +

You should describe here what actually changed. Sth like:
Calling beginElement() after calling endElement() does not restart the animation.

[Nikolas Zimmermann]

(In reply to comment #5)
> This is not the real bug title, it should always match the bugzilla title, so remove the "Bugfix: ".
Please use "SVG animation beginElement() does not restart a stopped animation", which is the original bug title.

[Felician Marton]

Created attachment 95751 [details]
Bugfix for SVG animation restarting with test

[Nikolas Zimmermann]

Comment on attachment 95751 [details]
Bugfix for SVG animation restarting with test

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

Hm, I leave the r? unchanged, as I have a question:

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:32
> +    setTimeout(end,1);

s/,1/, 0/

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:37
> +    setTimeout(begin2,1);

s/,1/, 0/

> LayoutTests/svg/animations/script-tests/animate-endElement-beginElement.js:51
> +    //BeginElement-endElement-beginElement musn't execute in zero time, because in the current
> +    //implemetation of the svg animation will loose the commands order!

This is not true, we're clamping the setTimeout value anyway to a lower boundary, so if you pass 0 or 1, it doesn't matter.
See Source/WebCore/page/DOMWindow.cpp - grep for setTimeout, it uses DOMTimer::install, and in DOMTimer.cpp you'll find the constructor does:

DOMTimer::DOMTimer(ScriptExecutionContext* context, PassOwnPtr<ScheduledAction> action, int interval, bool singleShot)
...
    , m_originalInterval(interval)
...
{
    scriptExecutionContext()->addTimeout(m_timeoutId, this);

    double intervalMilliseconds = intervalClampedToMinimum(interval, context->minimumTimerInterval());
    if (singleShot)
        startOneShot(intervalMilliseconds);
    else
        startRepeating(intervalMilliseconds);
}

The clamping routine does:

static const double oneMillisecond = 0.001;
double DOMTimer::intervalClampedToMinimum(int timeout, double minimumTimerInterval) const
{
    double intervalMilliseconds = max(oneMillisecond, timeout * oneMillisecond);

    if (intervalMilliseconds < minimumTimerInterval && m_nestingLevel >= maxTimerNestingLevel)
        intervalMilliseconds = minimumTimerInterval;
    return intervalMilliseconds;
}

So all timers are clamped to 1ms anyway. Can you explain what kind of trouble you ran into when using 0 as interval?

[Felician Marton]

First of all I think you misunderstood my sentence:
"BeginElement-endElement-beginElement musn't execute in zero time, because in the current implemetation of the svg animation will loose the commands order"

I mean the case when I would write into the js the following code:

animateX.beginElement();
animateX.endElement();
animateX.beginElement();

And _this_ code could run in zero time (faster than one tick in the JS timer), not the timeout interval 0.
--

> So all timers are clamped to 1ms anyway. Can you explain what kind of trouble you ran into when using 0 as interval?

Ok now is see setTimeout(foo,0) is absulately the same as setTimeot(foo,1) by syntax by syntax/working. But semantically not the same.
0 means I don't want to wait, but I _had to_ for some reason. 1 means I _want_!
So now that's why I still want to use 1.

Am I wrong?

[Nikolas Zimmermann]

(In reply to comment #9)
> First of all I think you misunderstood my sentence:
> "BeginElement-endElement-beginElement musn't execute in zero time, because in the current implemetation of the svg animation will loose the commands order"
> 
> I mean the case when I would write into the js the following code:
> 
> animateX.beginElement();
> animateX.endElement();
> animateX.beginElement();
> 
> And _this_ code could run in zero time (faster than one tick in the JS timer), not the timeout interval 0.
Aha, yeah I misunderstoof the sentence.

> --
> 
> > So all timers are clamped to 1ms anyway. Can you explain what kind of trouble you ran into when using 0 as interval?
> 
> Ok now is see setTimeout(foo,0) is absulately the same as setTimeot(foo,1) by syntax by syntax/working. But semantically not the same.
> 0 means I don't want to wait, but I _had to_ for some reason. 1 means I _want_!
> So now that's why I still want to use 1.
> 
> Am I wrong?
You are not wrong, though for consistency you should use 0.
When we usually do "setTimeout(doIt, 0)" we want to indicate: do _one_ cycle of layout/paint/style recalaculations on the document, before calling the "doIt()" function.
It's basically a way to guarantee the document eg. has repainted after you did any JS operation, that might affect rendering.

That's why setTimeout(foo, 0) doesn"t mean "> 0 means I don't want to wait, but I _had to_ .. " for me but rather, do the next function call after we're sure the page has been renderered completly.

Does that make sense for you?

[Felician Marton]

Created attachment 96233 [details]
Bugfix for SVG animation restarting with test

You convinced me, now is use setTimeout(foo, 0).

[Nikolas Zimmermann]

Comment on attachment 96233 [details]
Bugfix for SVG animation restarting with test

Looks good, r=me.

[WebKit Commit Bot]

Comment on attachment 96233 [details]
Bugfix for SVG animation restarting with test

Clearing flags on attachment: 96233

Committed r88234: <http://trac.webkit.org/changeset/88234>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.