Bug 83056 – Serializing SVG removes namespace from attributes

• Component: XML
• Status: RESOLVED FIXED

[Ian]

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

[Alexey Proskuryakov]

Duplicate of bug 79586?

[Rob Buis]

(In reply to comment #1)
> Duplicate of bug 79586?

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

Rob.

[Alex Milowski]

This is the same as bug 22958.

[Rob Buis]

This seems fixed to me. I think the 79586 bugfix fixed this as well.

[Alex Milowski]

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.

[Rob Buis]

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.

[Alex Milowski]

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.

[Rob Buis]

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

[Alex Milowski]

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

[Rob Buis]

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 :)

[Rob Buis]

> 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 :)

[Rob Buis]

Committed r154779: <http://trac.webkit.org/changeset/154779>