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>
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! :-)
Thanks Nikolas, I'm going to dive into webkit tonight, looks like this should not be too bad to fix, thanks.
(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 :-)
*** Bug 38114 has been marked as a duplicate of this bug. ***
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...
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.
(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.?
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.
Created attachment 95375 [details] Patch
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?
Created attachment 95441 [details] Patch
Comment on attachment 95441 [details] Patch You did not address my comment about passIfCloseEnough
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.
(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)
Created attachment 95457 [details] Patch
Comment on attachment 95457 [details] Patch r=me. LGTM.
Committed r87746: <http://trac.webkit.org/changeset/87746>
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.
Created attachment 95808 [details] Patch
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?
Committed r88020: <http://trac.webkit.org/changeset/88020>