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+

Description dj.am.juicebox 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>
Comment 1 Nikolas Zimmermann 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! :-)
Comment 2 dj.am.juicebox 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.
Comment 3 Nikolas Zimmermann 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 :-)
Comment 4 Nikolas Zimmermann 2010-05-10 01:33:51 PDT
*** Bug 38114 has been marked as a duplicate of this bug. ***
Comment 5 Nikolas Zimmermann 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...
Comment 6 Rob Buis 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.
Comment 7 Nikolas Zimmermann 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.?
Comment 8 Rob Buis 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.
Comment 9 Rob Buis 2011-05-30 15:11:58 PDT
Created attachment 95375 [details]
Patch
Comment 10 Dirk Schulze 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?
Comment 11 Rob Buis 2011-05-31 09:14:19 PDT
Created attachment 95441 [details]
Patch
Comment 12 Dirk Schulze 2011-05-31 10:08:57 PDT
Comment on attachment 95441 [details]
Patch

You did not address my comment about passIfCloseEnough
Comment 13 Rob Buis 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.
Comment 14 Dirk Schulze 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)
Comment 15 Rob Buis 2011-05-31 11:56:35 PDT
Created attachment 95457 [details]
Patch
Comment 16 Dirk Schulze 2011-05-31 12:28:53 PDT
Comment on attachment 95457 [details]
Patch

r=me. LGTM.
Comment 17 Rob Buis 2011-05-31 12:38:32 PDT
Committed r87746: <http://trac.webkit.org/changeset/87746>
Comment 18 Nikolas Zimmermann 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.
Comment 19 Rob Buis 2011-06-02 14:16:38 PDT
Created attachment 95808 [details]
Patch
Comment 20 Nikolas Zimmermann 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?
Comment 21 Rob Buis 2011-06-03 08:45:56 PDT
Committed r88020: <http://trac.webkit.org/changeset/88020>