Bug 41421

Summary: Node.cloneNode does not work on SVG nodes
Product: WebKit Reporter: Fady Samuel <fsamuel>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dino, eric, krit, simon.fraser, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
SVG Clone Test case with Ellipse
none
SVG Clone Test - Two Ellipses
none
Initial patch
none
Updated patch krit: review+

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.