Bug 34301

Summary:

Creating <animateMotion> elements via javascript do not execute

Product:

WebKit

Reporter:

dj.am.juicebox

Component:

SVG

Assignee:

Rob Buis <rwlbuis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

krit, marek.raida, rwlbuis, zimmermann

Priority:

Version:

420+

Hardware:

OS:

OS X 10.5

Bug Depends on:

17043

Bug Blocks:

41761

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	zimmermann: review+

dj.am.juicebox

Reported 2010-01-28 20:03:07 PST

Adding <animateMotion> elements via javascript fail to execute. This may be related to this bug 17043 "SVG missing some .idl files": https://bugs.webkit.org/show_bug.cgi?id=17043 The following example should animate a rectangle moving from left to right when the page loads. It does nothing. The same page works ok in opera. The same animation definition in the body of the document executes ok: <html> <head> <script> window.onload = function() { var svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); svg.setAttribute('xmlns:xlink', 'http://www.w3.org/1999/xlink'); svg.setAttribute('version', '1.1'); svg.setAttribute('width', '800px'); svg.setAttribute('height', '400px'); document.body.appendChild(svg); var rect = document.createElementNS('http://www.w3.org/2000/svg', 'rect'); rect.setAttribute("id", "myrect"); rect.setAttribute("fill","red"); rect.setAttribute("stroke","black"); rect.setAttribute("stroke-width","5"); rect.setAttribute("x", "100"); rect.setAttribute("y", "100"); rect.setAttribute("width", "100"); rect.setAttribute("height", "50"); svg.appendChild(rect); var anim = document.createElementNS('http://www.w3.org/2000/svg','animate'); anim.setAttribute("attributeName", "width"); anim.setAttribute("from", "100"); anim.setAttribute("to", "400"); anim.setAttribute("dur", "10s"); anim.setAttribute("begin", "0s"); anim.setAttribute("fill", "freeze"); rect.appendChild(anim); anim.beginElement(); } </script> </head> <body> </body> </html>

Attachments
Patch (10.77 KB, patch) 2011-05-30 15:11 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (14.88 KB, patch) 2011-05-31 09:14 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (14.50 KB, patch) 2011-05-31 11:56 PDT, Rob Buis	no flags	Details Formatted Diff Diff
Patch (23.22 KB, patch) 2011-06-02 14:16 PDT, Rob Buis	zimmermann: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Nikolas Zimmermann

Comment 1 2010-01-29 11:01:40 PST

Bug confirmed, SVGAnimateMotion.idl is missing. Grab another SVGAnimate*.idl file as template, copy it to SVGAnimateMotion.idl, adjust it according to the W3C IDL files (what methods/attributes are exported). Adding to the build is a bit more complex, if you attach a complete SVGAnimateMotion.idl here, I'm happy to either guide you through adding it to the build or I'll do it myselves. Thanks for the bug report, you're welcome to help with SVG! :-)

dj.am.juicebox

Comment 2 2010-01-29 15:24:20 PST

Thanks Nikolas, I'm going to dive into webkit tonight, looks like this should not be too bad to fix, thanks.

Nikolas Zimmermann

Comment 3 2010-01-29 15:30:02 PST

(In reply to comment #2) > Thanks Nikolas, I'm going to dive into webkit tonight, looks like this should > not be too bad to fix, thanks. If you need further assistance, Dirk & me usually hang around on IRC #webkit / #ksvg, just meet us there :-)

Nikolas Zimmermann

Comment 4 2010-05-10 01:33:51 PDT

*** Bug 38114 has been marked as a duplicate of this bug. ***

Nikolas Zimmermann

Comment 5 2010-05-10 01:34:45 PDT

Bug 17403, covers that SVGAnimationMotionElement.idl & SVGMPathElement.idl are missing. It should be trivial to expose them, it's just a bit cumbersome to add JS IDL bindings...

Rob Buis

Comment 6 2011-05-30 11:10:00 PDT

I don't the SVGAnimationMotionElement.idl is needed to fix this bug, this dynamic test works with trunk: http://www.kevlindev.com/tutorials/basics/animation/js_dom_smil/index.htm So I think this bug can be closed. OTOH it could be used to combine with bug 17043 and add a similar test as above but with <mpath> inserted as well. It is on my TODO list :) Cheers, Rob.

Nikolas Zimmermann

Comment 7 2011-05-30 11:15:14 PDT

(In reply to comment #6) > I don't the SVGAnimationMotionElement.idl is needed to fix this bug, this dynamic test works with trunk: > > http://www.kevlindev.com/tutorials/basics/animation/js_dom_smil/index.htm > > So I think this bug can be closed. OTOH it could be used to combine with bug 17043 and add a similar test as above but with <mpath> inserted as well. It is on my TODO list :) > Cheers, Well if you use document.createElementNS('http://www.w3.org/2000/svg','animateMotion'); it won't create an SVGAnimateMotionElement but a SVGElement, so you can't create those elements from DOM at the moment? Am I missing sth.?

Rob Buis

Comment 8 2011-05-30 11:29:06 PDT

Hi Niko, (In reply to comment #7) > (In reply to comment #6) > > I don't the SVGAnimationMotionElement.idl is needed to fix this bug, this dynamic test works with trunk: > > > > http://www.kevlindev.com/tutorials/basics/animation/js_dom_smil/index.htm > > > > So I think this bug can be closed. OTOH it could be used to combine with bug 17043 and add a similar test as above but with <mpath> inserted as well. It is on my TODO list :) > > Cheers, > > Well if you use document.createElementNS('http://www.w3.org/2000/svg','animateMotion'); > it won't create an SVGAnimateMotionElement but a SVGElement, so you can't create those elements from DOM at the moment? > > Am I missing sth.? Jein :) You are right, but the way it is used does not matter, as only setAttribute is used as API. Nevertheless we should add these .idl files, I have them ready. I am writing a test for inserting <mpath> dynamically, so that could go in with a patch to fix this bug. But it looks like this and bug 17032 should be fixed in one patch though. Cheers, Rob.

Rob Buis

Comment 9 2011-05-30 15:11:58 PDT

Created attachment 95375 [details] Patch

Dirk Schulze

Comment 10 2011-05-30 21:57:40 PDT

Comment on attachment 95375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95375&action=review Some snippets before I can give r+. > Source/WebCore/ChangeLog:17 > + * CMakeLists.txt: > + * CodeGenerators.pri: > + * WebCore.gypi: I wonder that you did not add it to Xcode!? Can you do that as well please? > LayoutTests/svg/animations/script-tests/animate-mpath-insert.js:42 > + passed = isCloseEnough(eval(name), value, error); > + if (passed) { > + testPassed(name + " is almost " + value + " (within " + error + ")"); > + } else { > + testFailed(name + " is " + eval(name) + " but should be within " + error + " of " + value); > + } We already have a function like passIfCloseEnough. Can you use this method please? It is in the animation lib. > LayoutTests/svg/animations/script-tests/animate-mpath-insert.js:52 > + passIfCloseEnough("rootSVGElement.getBBox().x", 100, 20); > + passIfCloseEnough("rootSVGElement.getBBox().y", 250, 20); > +} > + > +function endSample() { > + passIfCloseEnough("rootSVGElement.getBBox().x", 400, 20); > + passIfCloseEnough("rootSVGElement.getBBox().y", 250, 20); 20 is a high tolerance. Can you decrease it a bit?

Rob Buis

Comment 11 2011-05-31 09:14:19 PDT

Created attachment 95441 [details] Patch

Dirk Schulze

Comment 12 2011-05-31 10:08:57 PDT

Comment on attachment 95441 [details] Patch You did not address my comment about passIfCloseEnough

Rob Buis

Comment 13 2011-05-31 10:16:11 PDT

Hi Dirk, (In reply to comment #12) > (From update of attachment 95441 [details]) > You did not address my comment about passIfCloseEnough Sorry, completely forgot! I think you mean that we have isCloseEnough? passIfCloseEnough wraps it, otherwise I think you have to "inline" it and it ends up with a lot more code. Let me know if I am missing something. Cheers, Rob.

Dirk Schulze

Comment 14 2011-05-31 10:17:52 PDT

(In reply to comment #13) > Hi Dirk, > > (In reply to comment #12) > > (From update of attachment 95441 [details] [details]) > > You did not address my comment about passIfCloseEnough > > Sorry, completely forgot! > I think you mean that we have isCloseEnough? passIfCloseEnough wraps it, otherwise I think you have to "inline" it and it ends up with a lot more code. Let me know if I am missing something. > Cheers, > > Rob. No, I mean shouldBeCloseEnough! function shouldBeCloseEnough(_a, _b, tolerance)

Rob Buis

Comment 15 2011-05-31 11:56:35 PDT

Created attachment 95457 [details] Patch

Dirk Schulze

Comment 16 2011-05-31 12:28:53 PDT

Comment on attachment 95457 [details] Patch r=me. LGTM.

Rob Buis

Comment 17 2011-05-31 12:38:32 PDT

Committed r87746: <http://trac.webkit.org/changeset/87746>

Nikolas Zimmermann

Comment 18 2011-05-31 23:14:22 PDT

Rob, you forgot to update Source/WebCore/page/DOMWindow.idl, you have to enable the new constructors for mpath/animateMotiion, and update test expecations (there are some files that'll change, can't remember which ones, but we have tests that dump all contructors of DOMWindow). Reopening bug, as it needs a follow-up patch.

Rob Buis

Comment 19 2011-06-02 14:16:38 PDT

Created attachment 95808 [details] Patch

Nikolas Zimmermann

Comment 20 2011-06-03 00:56:51 PDT

Comment on attachment 95808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95808&action=review This is fine as-is, though some comments: > Source/WebCore/page/DOMWindow.idl:719 > -// attribute SVGAnimateMotionElementConstructor SVGAnimateMotionElement; > + attribute SVGAnimateMotionElementConstructor SVGAnimateMotionElement; The alignment doesn't match here. > Source/WebCore/page/DOMWindow.idl:721 > + attribute SVGMPathElementConstructor SVGMPathElement; Ditto. > LayoutTests/svg/custom/global-constructors-expected.txt:143 > FAIL SVGAnimationElement.toString() should be [object SVGAnimationElementConstructor]. Threw exception ReferenceError: Can't find variable: SVGAnimationElement Just noticed this, seems we have another constructor missing? Can you grep that file for other missing SVG constructors?

Rob Buis

Comment 21 2011-06-03 08:45:56 PDT

Committed r88020: <http://trac.webkit.org/changeset/88020>

Note You need to log in before you can comment on or make changes to this bug.