Bug 83056 - Serializing SVG removes namespace from attributes
Summary: Serializing SVG removes namespace from attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: Nobody
URL: http://bl.ocks.org/2294676
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-03 12:19 PDT by Ian
Modified: 2013-08-29 10:22 PDT (History)
3 users (show)

See Also:


Attachments
Example of two ways to do the same thing (1.03 KB, application/xhtml+xml)
2013-06-22 16:14 PDT, Alex Milowski
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ian 2012-04-03 12:19:01 PDT
I've coded an example which uses DOM serialization to rasterize some SVG with Canvas:
http://bl.ocks.org/2294676

In Webkit (but not Firefox) I have to run a regular expression on my serialized SVG to add back the xlink: to my xlink:href attributes.

This may be caused by this bug: https://bugs.webkit.org/show_bug.cgi?id=22958

It may be related to an issue here: http://stackoverflow.com/questions/8979267/xmlserializer-strips-xlink-from-xlinkhtml-svg-image-tag
but I'm not sure it's the same thing since I am not dynamically generating these DOM elements
Comment 1 Alexey Proskuryakov 2012-04-04 13:07:44 PDT
Duplicate of bug 79586?
Comment 2 Rob Buis 2012-04-04 13:13:57 PDT
(In reply to comment #1)
> Duplicate of bug 79586?

Very likely. Fixing 79586 should fix this one as well.
Cheers,

Rob.
Comment 3 Alex Milowski 2013-06-18 17:13:16 PDT
This is the same as bug 22958.
Comment 4 Rob Buis 2013-06-22 11:26:36 PDT
This seems fixed to me. I think the 79586 bugfix fixed this as well.
Comment 5 Alex Milowski 2013-06-22 16:14:06 PDT
Created attachment 205257 [details]
Example of two ways to do the same thing

The attached example does the same thing slightly differently.   Both are correct from a DOM/JS perspective but the second requires the serializer to generate a prefix.  The current serializer code fails to do this correctly.

Thus, it really depends on how you define fixed.  Is it fixed to leave a known issue?  IMHO, this is really fixed when bug 22958 is fixed.
Comment 6 Rob Buis 2013-06-22 19:18:16 PDT
Hi Alex,

(In reply to comment #5)
> Created an attachment (id=205257) [details]
> Example of two ways to do the same thing
> 
> The attached example does the same thing slightly differently.   Both are correct from a DOM/JS perspective but the second requires the serializer to generate a prefix.  The current serializer code fails to do this correctly.
> 
> Thus, it really depends on how you define fixed.  Is it fixed to leave a known issue?  IMHO, this is really fixed when bug 22958 is fixed.

I meant fixed in the sense that the blocks page works. Or rather, when taking it and removing the webkit specific replace hack it works.
I am updating my patch for bug 16496, hopefully it can fix more than just that bug, since it seems like there is some overlap in the XMLSerializer bugs.
Comment 7 Alex Milowski 2013-06-23 20:46:45 PDT
Sure.  That makes sense.  

I don't know what the policy is for closing bugs like this.  Certainly the problem described for that particular page is fixed within the current code.
Comment 8 Rob Buis 2013-06-24 07:08:28 PDT
(In reply to comment #7)
> Sure.  That makes sense.  
> 
> I don't know what the policy is for closing bugs like this.  Certainly the problem described for that particular page is fixed within the current code.

Well, the latest patch on bug 16496 fixes both of the problems in your testcase. So once that goes in, we can close this bug too, though we have to check whether we want to keep your testcase in the tests.
Comment 9 Alex Milowski 2013-06-24 14:35:33 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Sure.  That makes sense.  
> > 
> > I don't know what the policy is for closing bugs like this.  Certainly the problem described for that particular page is fixed within the current code.
> 
> Well, the latest patch on bug 16496 fixes both of the problems in your testcase. So once that goes in, we can close this bug too, though we have to check whether we want to keep your testcase in the tests.

I took a peek at this patch.  It doesn't look like it handles the general problems with the serializer but certainly can address issues with the SVG and xlink namespace/prefix.

The real fix requires an adjustment to the algorithm to keep track of generated prefixes and/or their declaration.  That is, you need to general prefixes as necessary, not just for known namespaces, and the keep track of their scope.  Neither of those happen currently in the MarkupAccumulator.cpp code.

See bug 117764 for some more esoteric examples of where this breaks down.

That said, I'm happy to close out specific bugs with smaller changes and wait for the general one to be addressed.
Comment 10 Rob Buis 2013-06-24 14:51:54 PDT
Hi Alex,

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Sure.  That makes sense.  
> > > 
> > > I don't know what the policy is for closing bugs like this.  Certainly the problem described for that particular page is fixed within the current code.
> > 
> > Well, the latest patch on bug 16496 fixes both of the problems in your testcase. So once that goes in, we can close this bug too, though we have to check whether we want to keep your testcase in the tests.
> 
> I took a peek at this patch.  It doesn't look like it handles the general problems with the serializer but certainly can address issues with the SVG and xlink namespace/prefix.
> 
> The real fix requires an adjustment to the algorithm to keep track of generated prefixes and/or their declaration.  That is, you need to general prefixes as necessary, not just for known namespaces, and the keep track of their scope.  Neither of those happen currently in the MarkupAccumulator.cpp code.

Yeah, my thinking was follow the "hardcoded" handling of attribute prefixes (xlink, xml xmlns) in appendAttribute for now to be on the safe side. I am relying on review iterations to improve the code quality :)
 
> See bug 117764 for some more esoteric examples of where this breaks down.

Thanks.

> That said, I'm happy to close out specific bugs with smaller changes and wait for the general one to be addressed.

Yep, see above, I am trying to be pragmatic about it. It is hard enough to pass all/not brrak tests and/or make the serialized test output sane :)
Comment 11 Rob Buis 2013-06-27 14:06:38 PDT
> I took a peek at this patch.  It doesn't look like it handles the general problems with the serializer but certainly can address issues with the SVG and xlink namespace/prefix.
>
>
> The real fix requires an adjustment to the algorithm to keep track of generated prefixes and/or their declaration.  That is, you need to general prefixes as necessary, not just for known namespaces, and the keep track of their scope.  Neither of those happen currently in the MarkupAccumulator.cpp code.

I am starting fixing this in bug 19121. It would be nice if you could have a look :)
Comment 12 Rob Buis 2013-08-29 10:22:30 PDT
Committed r154779: <http://trac.webkit.org/changeset/154779>