Summary: | SVG animation beginElement() does not restart a stopped animation. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mg <griffin_m82> | ||||||||||||
Component: | SVG | Assignee: | Felician Marton <felician> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abecsi, commit-queue, felician, krit, rhodovan.u-szeged, zimmermann | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 41761 | ||||||||||||||
Attachments: |
|
Description
mg
2010-08-03 17:24:05 PDT
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.
Created attachment 94602 [details]
Bugfix for SVG animation restarting.
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 Created attachment 95170 [details]
Bugfix for SVG animation restarting. Updated Patch.
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. (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. Created attachment 95751 [details]
Bugfix for SVG animation restarting with test
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? 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?
(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? Created attachment 96233 [details]
Bugfix for SVG animation restarting with test
You convinced me, now is use setTimeout(foo, 0).
Comment on attachment 96233 [details]
Bugfix for SVG animation restarting with test
Looks good, r=me.
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> All reviewed patches have been landed. Closing bug. |