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

lookup fails on SVG viewBox attribute. #318

Open
calvinjuarez opened this issue Jun 23, 2020 · 4 comments
Open

lookup fails on SVG viewBox attribute. #318

calvinjuarez opened this issue Jun 23, 2020 · 4 comments
Labels

Comments

@calvinjuarez
Copy link

calvinjuarez commented Jun 23, 2020

AST Explorer link: https://astexplorer.net/#/gist/25e9a360157ab2e56a7bf14ac22daa5b/196c723e25cf99ec923c3ce0be36664817b264d2

<svg viewBox="0 0 100 100"></svg>

Given the SVG element above, the AST produces an svg element node whose attrs array has a { name: 'viewBox', value: '0 0 100 100' } object, but that name doesn't match anything in the element's own sourceCodeLocation.attrs hash. Instead, that hash has a viewbox (all lowercase) key. (See the AST Explorer screenshot below).

Is this by design? If so, is it recommended that location lookup always use .toLowerCase()? Or are SVG camelCase attributes an exception that should be handled in a special case in AST consumers? Have I missed documentation on this?

Or is this a bug? If so, there are a number of SVG camelCase attributes that should likely be handled similarly.

Screen Shot 2020-06-23 at 2 02 16 PM

@calvinjuarez
Copy link
Author

calvinjuarez commented Jun 24, 2020

I'm looking into a fix for this in the location info mixin. The foreign-content.js adjustTokenSVGAttrs method does the transformation from viewboxviewBox, but it (obviously) doesn't transform the key in the location.attrs hash created by the mixin. I'm not sure the best way to fix this. It feels bad to add logic to the core that depends on a mixin, but it also feels weird to expose what's essentially private data in the core to the mixin.

If anyone can offer direction on how best to go about adjusting the location key in the way the attribute name is adjusted, I'd appreciate the help.

Would it be an awful idea to import/require the SVG_ATTRS_ADJUSTMENT_MAP out of foreign-content.js into the plugin?

calvinjuarez added a commit to calvinjuarez/parse5 that referenced this issue Jun 24, 2020
calvinjuarez added a commit to calvinjuarez/parse5 that referenced this issue Jun 24, 2020
@calvinjuarez
Copy link
Author

calvinjuarez commented Jun 24, 2020

Update: I've got a branch with my current attempt at a fix that I'd like thoughts on: calvinjuarez@62f7a59

It feels a little under-cooked at the moment. I'd love tips on improvement.

@calvinjuarez
Copy link
Author

So looking closer, my changes would actually transform attributes on non-SVG elements, which is probably bad news. I'll likely need to expose the "are we in an svg" information from core so that the plugin can access it.

@fb55
Copy link
Collaborator

fb55 commented Feb 16, 2022

Interesting issue, and not easy to fix. Lowercasing all attribute names before the lookup is a valid workaround for now. Based on how the HTML tokenizer works, attribute names will always be unique by their lowercase representation, and attribute locations will always be referenced by the lowercase name.

@fb55 fb55 added the bug label Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants