Bug 34301

Summary: Creating <animateMotion> elements via javascript do not execute
Product: WebKit Reporter: dj.am.juicebox
Component: SVGAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: krit, marek.raida, rwlbuis, zimmermann
Priority: P2    
Version: 420+   
Hardware: PC   
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
DetailsFormatted DiffDiff
Patch (14.88 KB, patch)
2011-05-31 09:14 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (14.50 KB, patch)
2011-05-31 11:56 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (23.22 KB, patch)
2011-06-02 14:16 PDT, Rob Buis 		zimmermann: review+
DetailsFormatted DiffDiff
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.