Summary: | Node.cloneNode does not work on SVG nodes | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fady Samuel <fsamuel> | ||||||||||
Component: | SVG | Assignee: | 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
Fady Samuel
2010-06-30 10:53:32 PDT
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. |