SVGKit / SVGKit

Display and interact with SVG Images on iOS / OS X, using native rendering (CoreAnimation)
Other
4.47k stars 1.08k forks source link

Parsing error with xmlns:x specified #795

Open dehlen opened 1 year ago

dehlen commented 1 year ago

New Issue Checklist

Issue Description and Steps

When parsing the following SVG (only relevant part copied below)

<?xml version="1.0"?>
<svg xmlns:x="http://ns.adobe.com/Extensibility/1.0/" xmlns:i="http://ns.adobe.com/AdobeIllustrator/10.0/" xmlns:graph="http://ns.adobe.com/Graphs/1.0/" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" id="Layer_1" x="0px" y="0px" width="1440px" height="1440px" viewBox="0 0 1440 1440" xml:space="preserve">
</svg>

the resulting UIImage is nil. This is due to the fact that in SVGSVGElement:postProcessAttributesAddingErrorsTo the x position is parsed incorrectly. The returned NSString from this call: NSString* pos_x = [self getAttribute:@"x"]; is http://ns.adobe.com/Extensibility/1.0/ and not 0px. I verified that in NamedNodeMap:getNamedItem there are three dictionaries one for https://www.w3.org/2000/xmIns/ and another one for https://www.w3.org/2000/svg. Only the latter contains the correct x value. However since https://www.w3.org/2000/xmIns/ is the first item in the list its x value is retrieved and then parsing this string to a number fails with the following error:

Printing description of self->parseErrorsAndWarnings:
[Parse result: 0 warnings, 0 errors(recoverable), 1 errors (fatal). First fatal error: Error Domain=SVGK Parsing Code=32523432 "Exception = Asked to convert a (19) value to a (5) (couldn't find a valid conversion route). Float (4 d.p.) = 0.0000, String = http://ns.adobe.com/Extensibility/1.0/" UserInfo={NSLocalizedDescription=Exception = Asked to convert a (19) value to a (5) (couldn't find a valid conversion route). Float (4 d.p.) = 0.0000, String = http://ns.adobe.com/Extensibility/1.0/}

I am wondering: is this a bug in the library or is the SVG not correctly authored. Anyway there might be a better solution to this instead of bailing out. I am happy to hear your thoughts.

Kind regards, David

PS: I attached a screenshots of the debugger displaying the problem.

Bildschirmfoto 2023-08-09 um 11 04 34 Bildschirmfoto 2023-08-09 um 10 45 30
adamgit commented 1 year ago

Good catch.

I added the xmlns support myself - the original SVGKit implementation ignored namespaces, so almost all the code fetches attributes without refrence to a namespace - and in almost all cases there are no conflicts. Namespaces are usually only used when they're needed, so they're rare in the wild. What Adobe has done here isn't 'wrong' it's just 'annoyingly stupid design and borderline passive-aggressively difficult for no good reason'. When I added the support to SVGKit I tried to make an 'educated guess' about which namespace is intended when none is specified - otherwise we'd have to rewrite every class, and adding new classes would have required a lot more typing. It may be that code (my 'guess') could be tweaked to bypass this (I have a vague memory I iterate the namespaces in the order we find them in the document, and accept the first one that has a match -- hence the Adobe one gets picked first here)

Formally ... we should probably be insisting on a namespace for every attribute lookup done by any of our parsers, since "x" has no meaning, only "(some namespace).x" has meaning.

Note: from memory, I automatically assign things to the svg namespace if they have no explicit one (IIRC that's what the spec says to do). So - I think - if we changed the parsers to be explicit on namespace, every file should continue to parse identically as before.

dehlen commented 1 year ago

Interesting, thanks for your response :) From what I can tell the problem is that in SVGSVGElement width, height, x and y are retrieved via getAttribute which in fact looks in the dictionary and the dictionary indeed just returns the first result it will find in the dictionary of namespaces. However I found the method -(NSString*) getAttributeNS:(NSString*) namespaceURI localName:(NSString*) localName and am wondering whether it would be feasible to just switch the method call to this one and provide the svg namespace here as the first argument. However I am not in any way familiar with the svg spec so I assume there is a good reason this is not already done? I might be able to provide a PR if you can provide more details on how to address this problem.

adamgit commented 1 year ago

I am 95% sure the only reason it doesn't do a namespaced-lookup is 'because legacy code', ie the code that didn't understand namespaces already worked, so ... no-one changed it.

On Mon, 11 Sept 2023 at 06:19, David v.Knobelsdorff < @.***> wrote:

Interesting, thanks for your response :) From what I can tell the problem is that in SVGSVGElement width, height, x and y are retrieved via getAttribute which in fact looks in the dictionary and the dictionary indeed just returns the first result it will find in the dictionary of namespaces. However I found the method -(NSString) getAttributeNS:(NSString) namespaceURI localName:(NSString*) localName and am wondering whether it would be feasible to just switch the method call to this one and provide the svg namespace here as the first argument. However I am not in any way familiar with the svg spec so I assume there is a good reason this is not already done? I might be able to provide a PR if you can provide more details on how to address this problem.

— Reply to this email directly, view it on GitHub https://github.com/SVGKit/SVGKit/issues/795#issuecomment-1713236511, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACHL6KDLWHYPU5OYZCTTO3XZ2UNXANCNFSM6AAAAAA3JV6UFQ . You are receiving this because you commented.Message ID: @.***>