Bug 34301 - Creating <animateMotion> elements via javascript do not execute
: Creating <animateMotion> elements via javascript do not execute
Status: RESOLVED FIXED
: WebKit
SVG
: 420+
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 17043
: 41761
  Show dependency treegraph
 
Reported: 2010-01-28 20:03 PST by
Modified: 2011-06-03 08:45 PST (History)


Attachments
Patch (10.77 KB, patch)
2011-05-30 15:11 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.88 KB, patch)
2011-05-31 09:14 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.50 KB, patch)
2011-05-31 11:56 PST, Rob Buis
no flags Review Patch | Details | Formatted Diff | Diff
Patch (23.22 KB, patch)
2011-06-02 14:16 PST, Rob Buis
zimmermann: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 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 From 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 From 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 From 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 From 2010-05-10 01:33:51 PST -------
*** Bug 38114 has been marked as a duplicate of this bug. ***
------- Comment #5 From 2010-05-10 01:34:45 PST -------
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 From 2011-05-30 11:10:00 PST -------
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 From 2011-05-30 11:15:14 PST -------
(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 From 2011-05-30 11:29:06 PST -------
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 From 2011-05-30 15:11:58 PST -------
Created an attachment (id=95375) [details]
Patch
------- Comment #10 From 2011-05-30 21:57:40 PST -------
(From update of attachment 95375 [details])
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 From 2011-05-31 09:14:19 PST -------
Created an attachment (id=95441) [details]
Patch
------- Comment #12 From 2011-05-31 10:08:57 PST -------
(From update of attachment 95441 [details])
You did not address my comment about passIfCloseEnough
------- Comment #13 From 2011-05-31 10:16:11 PST -------
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.
------- Comment #14 From 2011-05-31 10:17:52 PST -------
(In reply to comment #13)
> Hi Dirk,
> 
> (In reply to comment #12)
> > (From update of attachment 95441 [details] [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 From 2011-05-31 11:56:35 PST -------
Created an attachment (id=95457) [details]
Patch
------- Comment #16 From 2011-05-31 12:28:53 PST -------
(From update of attachment 95457 [details])
r=me. LGTM.
------- Comment #17 From 2011-05-31 12:38:32 PST -------
Committed r87746: <http://trac.webkit.org/changeset/87746>
------- Comment #18 From 2011-05-31 23:14:22 PST -------
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 From 2011-06-02 14:16:38 PST -------
Created an attachment (id=95808) [details]
Patch
------- Comment #20 From 2011-06-03 00:56:51 PST -------
(From update of attachment 95808 [details])
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 From 2011-06-03 08:45:56 PST -------
Committed r88020: <http://trac.webkit.org/changeset/88020>