Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse error on case-sensitive attributes in SVG #18

Open
thibaudcolas opened this issue Apr 26, 2019 · 5 comments
Open

Parse error on case-sensitive attributes in SVG #18

thibaudcolas opened this issue Apr 26, 2019 · 5 comments

Comments

@thibaudcolas
Copy link

While trying this tool on a bunch of codebases, I ran into issues with SVG code embedded in HTML. For example,

<a href="#" role="tab">
    <svg viewBox="0 0 51.8 103.7" aria-hidden="true">
        <title>Arrow</title>
        <use xlink:href="#arrow"></use>
    </svg>
</a>

With this code, Jinjalint will report a parse error on the viewBox attribute, because of the capital "B":

Parse error: expected one of '=', '>', '[0-9-_.]', '[:a-z]', '\\s+', '{#', '{%', '{{' at 14:84

I assume this is because Jinjalint treats this as if it was HTML, and wants attributes to be lowercase. SVG being based on XML, it is case-sensitive, and it would be incorrect to replace this with viewbox.


I think potential solutions for this would either be to ignore attribute casing inside svg tags (in the tag’s own attribute, and its children), or have a whitelist of attributes that should be treated as case-sensitive.

@thibaudcolas
Copy link
Author

It looks like a simple fix for this would be to allow uppercase letters in all attributes, by changing:

def make_attribute_parser(jinja):
attribute_value = make_attribute_value_parser(jinja)
return (
locate(
P.seq(
interpolated(tag_name),

to use the following attr_name instead of tag_name

attr_name_start_char = P.regex(r'[:a-z]')
attr_name_char = attr_name_start_char | P.regex(r'[0-9A-Z-_.]')
attr_name = attr_name_start_char + attr_name_char.many().concat()

Of course this isn’t 100% appropriate since only SVG attributes can be camelCase, and this regex isn’t strictly restricting "camelCase".


I could also think of two alternatives that might be more correct, but I’m not sure how much effort they would be to implement:

  • Use a whitelist of (SVG) attributes that are allowed to contain uppercase letters – this should be relatively easy to implement but the whitelist might be annoying to maintain (although SVG doesn’t change that much over time, https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute, and I guess it doesn’t hurt to add SVG 2.0 attributes already even if support is patchy)
  • Change the parsing rules depending on whether we are inside an svg tag or not. This would be more correct but also seems like much more work

Additionally I’m not sure how this attributes parsing works with namespaced attributes (e.g. xml:lang), but this could also be similarly handled.

I’m leaning towards the attributes whitelist since it will be easy to get set up. If this was set up for HTML attributes as well this could also potentially catch many more issues with mis-typed attributes.

@SimonHeimberg
Copy link

I landed here because of camel case html attributes. It took quite some time to understand the problem, since the browsers accept camel case attributes, even if they are wrong probably.

Therefore I suggest additionally that the error message states the error more clearly. (Like: only lower case characters in attribute name)

@thibaudcolas
Copy link
Author

I’ve ended up fixing this in my fork in the way I describe above, along with other SVG-related parsing issues. Since this really is just parsing I think the parser should just be more lenient – browsers have no problem with uppercase tags. Enforcing that tags are lowercase / only use valid elements should be done by a linting rule, after parsing.

@SimonHeimberg
Copy link

I’ve ended up fixing this in my fork in the way I describe above, along with other SVG-related parsing issues.

Could you do a pr here? Or is fork not a git fork?

Since this really is just parsing I think the parser should just be more lenient – browsers have no problem with uppercase tags. Enforcing that tags are lowercase / only use valid elements should be done by a linting rule, after parsing.

👍

@thibaudcolas
Copy link
Author

Yep, can make a PR, although I’m not too hopeful about it getting merged. Here is the changed that fixed this: thibaudcolas/curlylint@fdd9554

It’s not what this issue is about but I subsequently had to do a much bigger change for the parser to not fail on self-closing SVG elements, thibaudcolas/curlylint@7bd5160.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants