Node.cloneNode does not work on SVG nodes

[Fady Samuel]

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.

[Simon Fraser (smfr)]

There is no attachment (yet)?

[Fady Samuel]

Created attachment 60135 [details]
SVG Clone Test case with Ellipse

[Fady Samuel]

Oops, sorry, attached. Thanks.

[Simon Fraser (smfr)]

A good test is self-documenting. It should say something like "you should see two ellipses below".

[Fady Samuel]

Created attachment 60137 [details]
SVG Clone Test - Two Ellipses

[Fady Samuel]

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...

[Nikolas Zimmermann]

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?

[Darin Adler]

(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?

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

(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?

[Nikolas Zimmermann]

Created attachment 60448 [details]
Initial patch

Fixed bug with a simpe one-liner.

[Darin Adler]

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?

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

Created attachment 60518 [details]
Updated patch

Incorporated Darins comments.

[Dirk Schulze]

Comment on attachment 60518 [details]
Updated patch

r=me

[Nikolas Zimmermann]

Landed in r62484.