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.
There is no attachment (yet)?
Created attachment 60135 [details] SVG Clone Test case with Ellipse
Oops, sorry, attached. Thanks.
A good test is self-documenting. It should say something like "you should see two ellipses below".
Created attachment 60137 [details] SVG Clone Test - Two Ellipses
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...
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?
(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?
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.
(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?
Created attachment 60448 [details] Initial patch Fixed bug with a simpe one-liner.
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?
(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.
Created attachment 60518 [details] Updated patch Incorporated Darins comments.
Comment on attachment 60518 [details] Updated patch r=me
Landed in r62484.