A fix for some errors in escapeXML() #54
Conversation
…ned, a JavaScript error was thrown. I have implemented the fix in a generic fashion, i.e. it will now behave correctly for any non-string value that doesn't implement a toString() method (not just undefined values). If toString() is available, then this will be used to convert the value to a string prior to performing the necessary replacements, otherwise we catch the resulting JavaScript error and return an empty string. This works around an IE8 issue, whereby node.node.className.baseVal is not defined, resulting in a 'class' attribute set to undefined. I have logged the underlying issue as issue AliasIO#53, but this fix means that we should no longer generate a JavaScript error when this situation occurs.
Previously, this had been left out, meaning that invalid SVG markup would be generated for properties that contain "&", most commonly in URLs. This will hopefully fixes issue AliasIO#46.
MarkMaldaba
changed the title
|
I have also added a fix for the fact that ampersands weren't being escaped properly, which resulted in invalid markup in situations where an ampersand is present in an attribute property. If you need me to submit these as separate pull requests, I can. However, as they touch the same code it would require rebasing whichever commit gets pulled last, so I thought it more sensible to include them jointly in the same PR. Hopefully this second commit will fix issue #46. |
| // catch block will return an empty string. | ||
| // Note that (perhaps, surprisingly) string values have a toString() | ||
| // function, so we don't need to check the type before calling. | ||
| s = s.toString(); |
There was a problem hiding this comment.
How about s = s.hasOwnProperty('toString') ? s.toString() : '' instead of a try/catch block?
There was a problem hiding this comment.
That won't work if passed an object that inherits toString() from the prototype chain, e.g. something that extends the String class. You could use typeof s.toString == 'undefined', except I think that breaks if s is undefined (at least, it did on IE8) so you would also need to check that It was at this point I decided to play it safe and go with a try/catch block, in case there were other values that would also result in errors in this situation (e.g. null).
Maybe this level of rigour doesn't matter for this application as it may be that we always have either a string a number or undefined, but I didn't want to take the risk, so a try/catch seemed safer.
There was a problem hiding this comment.
In that case s = s && s.toString ? s.toString() : '' will do. Why would toString not work if the object extends the String class? Any object that inherits from Object.prototype will have a toString method, unless it was deliberately removed for some odd reason.
I think we should avoid try/catch and handle these cases explicitly.
There was a problem hiding this comment.
In that case
s = s && s.toString ? s.toString() : ''will do.
Depends how thorough you want the function to be. With that code, a value of false will result in "" whereas true will result in "true". In my version, you would get "false", which is more consistent and correct.
However, as I said, if we don't care about that kind of edge-case, which is probably not likely to occur in the context of what the plugin is doing, then your suggested code would be good enough.
Personally, I don't like using inline if statements that are more complex than individual variable references and literals. It becomes very hard to read, parse and understand as soon as you introduce more complex expressions. I would expand it into a standard if/else block in this situation.
However, it's your plugin so I'll happy implement it whatever way you like - please advise.
Why would toString not work if the object extends the String class?
s.hasOwnProperty('toString') won't work, as the property comes from the prototype chain. Checking s.toString directly as per your second suggestion, should work fine.
There was a problem hiding this comment.
Hi @AliasIO - I would be keen to resolve this PR, and made a couple of alternative suggestions above. Please let me know which solution you prefer and I'll update the PR to meet your preference.
2 participants
Fixed an issue in escapeXML() whereby if the supplied value is undefined, a JavaScript error was thrown.
I have implemented this fix in a generic fashion, i.e. it will now behave correctly for any non-string value that doesn't implement a toString() method (not just undefined values). If toString() is available, then this will be used to convert the value to a string prior to performing the necessary replacements, otherwise we catch the resulting JavaScript error and return an empty string.
This works around an IE8 issue, whereby node.node.className.baseVal is not defined, resulting in a 'class' attribute set to undefined. I have logged the underlying issue as issue #53, but this fix means that we should no longer generate a JavaScript error when this situation occurs.