Bug 41421 - Node.cloneNode does not work on SVG nodes
Summary: Node.cloneNode does not work on SVG nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-30 10:53 PDT by Fady Samuel
Modified: 2010-07-05 05:50 PDT (History)
6 users (show)

See Also:


Attachments
SVG Clone Test case with Ellipse (1.14 KB, text/html)
2010-06-30 11:22 PDT, Fady Samuel
no flags Details
SVG Clone Test - Two Ellipses (1.17 KB, text/html)
2010-06-30 11:35 PDT, Fady Samuel
no flags Details
Initial patch (35.60 KB, patch)
2010-07-03 04:33 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (35.46 KB, patch)
2010-07-05 05:32 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 2010-06-30 10:53:32 PDT
The html file contains a javascript file. The javascript file creates an SVG ellipse. Then it clones the ellipse using Node.cloneNode(true) and changes the x coordinate of the resulting ellipse. Finally, the two ellipses are attached to the DOM tree. However only the first ellipse is displayed. This is because none of the first ellipse's attributes have been correctly cloned by Node.cloneNode(). The second ellipse has blank attributes.
Comment 1 Simon Fraser (smfr) 2010-06-30 11:19:17 PDT
There is no attachment (yet)?
Comment 2 Fady Samuel 2010-06-30 11:22:20 PDT
Created attachment 60135 [details]
SVG Clone Test case with Ellipse
Comment 3 Fady Samuel 2010-06-30 11:22:42 PDT
Oops, sorry, attached. Thanks.
Comment 4 Simon Fraser (smfr) 2010-06-30 11:33:25 PDT
A good test is self-documenting. It should say something like "you should see two ellipses below".
Comment 5 Fady Samuel 2010-06-30 11:35:48 PDT
Created attachment 60137 [details]
SVG Clone Test - Two Ellipses
Comment 6 Fady Samuel 2010-06-30 12:38:50 PDT
I have an approach to get this to work without modifying a zillion files but it'll be a bit hacky as it'll involve a bit of pointer arithmetic...
Comment 7 Nikolas Zimmermann 2010-06-30 23:56:26 PDT
Thanks for the bug report, good catch Fady!
I think it's not hard to fix. The SVG animated properties are synchronized with their corresponding XML attributes, whenever you use getAttribute/hasAttribute(s). We just need to assure they're synchronized when cloning the node, that's all.

If you grep for "updateAnimatedSVGAttribute" in WebCore/dom, you should see how it works, and how to hook that logic into cloneNode. Do you want to give it a try?
Comment 8 Darin Adler 2010-07-01 11:07:51 PDT
(In reply to comment #7)
> If you grep for "updateAnimatedSVGAttribute" in WebCore/dom, you should see how it works, and how to hook that logic into cloneNode. Do you want to give it a try?

Niko, the way this should work is that Element::cloneElementWithoutChildren calls attributes(true), then Element::attributes calls updateAnimatedSVGAttribute. Why isn’t that working?
Comment 9 Nikolas Zimmermann 2010-07-03 00:38:46 PDT
N(In reply to comment #8)
> (In reply to comment #7)
> > If you grep for "updateAnimatedSVGAttribute" in WebCore/dom, you should see how it works, and how to hook that logic into cloneNode. Do you want to give it a try?
> 
> Niko, the way this should work is that Element::cloneElementWithoutChildren calls attributes(true), then Element::attributes calls updateAnimatedSVGAttribute. Why isn’t that working?

Didn't investigate yet. I only know a workaround, that proves it's related to the synchronization:
If you insert " alert(ellipse1.getAttribute("cx"));" before cloning the ellipse, it works as expected.

I'll check soon what's going on.
Comment 10 Nikolas Zimmermann 2010-07-03 00:45:10 PDT
(In reply to comment #9)
> N(In reply to comment #8)
> > (In reply to comment #7)
> > > If you grep for "updateAnimatedSVGAttribute" in WebCore/dom, you should see how it works, and how to hook that logic into cloneNode. Do you want to give it a try?
> > 
> > Niko, the way this should work is that Element::cloneElementWithoutChildren calls attributes(true), then Element::attributes calls updateAnimatedSVGAttribute. Why isn’t that working?
> 
> Didn't investigate yet. I only know a workaround, that proves it's related to the synchronization:
> If you insert " alert(ellipse1.getAttribute("cx"));" before cloning the ellipse, it works as expected.
> 
> I'll check soon what's going on.

That was easy:

PassRefPtr<Element> Element::cloneElementWithoutChildren()
{
...

    if (namedAttrMap)
        clone->attributes()->setAttributes(*attributes(true)); // Call attributes(true) to force attribute synchronization to occur (for svg and style) before cloning happens.
...
}

The synchronization only happens if namedAttrMap is not null. In this case, as no XML attributes have been specified, the namedAttrMap is null, and thus synchronization does not happen.
A possible fix would be to add following...

#if ENABLE(SVG)
    if (!areSVGAttributesValid())
        updateAnimatedSVGAttribute(anyQName());
#endif

... before checking whether namedAttrMap exists. This is the same check done in attributes(), see Element.h.

What do you think? Shall I prepare a patch?
Comment 11 Nikolas Zimmermann 2010-07-03 04:33:39 PDT
Created attachment 60448 [details]
Initial patch

Fixed bug with a simpe one-liner.
Comment 12 Darin Adler 2010-07-03 10:21:04 PDT
Comment on attachment 60448 [details]
Initial patch

This fix seems OK, but I think we can do better without spreading SVG-specific code around even more. There's no good reason for both the attributes function and this function to have to update the SVG attributes.

I think all we need to do is to call attributes(true) and check its return value for null instead of checking namedAttrMap for null.

    // Call attributes(true) to force attribute synchronization to occur for SVG and style attributes.
    if (NamedNodeMap* attributeMap = attributes(true))
        clone->attributes()->setAttributes(*attributeMap);

Doesn't that look better than adding the SVG code?
Comment 13 Nikolas Zimmermann 2010-07-05 01:58:57 PDT
(In reply to comment #12)
> (From update of attachment 60448 [details])
> This fix seems OK, but I think we can do better without spreading SVG-specific code around even more. There's no good reason for both the attributes function and this function to have to update the SVG attributes.
> 
> I think all we need to do is to call attributes(true) and check its return value for null instead of checking namedAttrMap for null.
> 
>     // Call attributes(true) to force attribute synchronization to occur for SVG and style attributes.
>     if (NamedNodeMap* attributeMap = attributes(true))
>         clone->attributes()->setAttributes(*attributeMap);
> 
> Doesn't that look better than adding the SVG code?

Heh, you're absolute right. I'll fix that.
Comment 14 Nikolas Zimmermann 2010-07-05 05:32:52 PDT
Created attachment 60518 [details]
Updated patch

Incorporated Darins comments.
Comment 15 Dirk Schulze 2010-07-05 05:35:30 PDT
Comment on attachment 60518 [details]
Updated patch

r=me
Comment 16 Nikolas Zimmermann 2010-07-05 05:50:01 PDT
Landed in r62484.