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

Adjust SVG attributes irrespective of their namespace #69

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

maurobringolf
Copy link
Contributor

@maurobringolf maurobringolf commented Jul 7, 2022

Hi!

I am using tyxml and ran into the same issue as ocsigen/tyxml#307, but with the attribute viewBox instead. It only occurs with the ppx so I looked into the html_parser as suggested by @mooreryan in the issue (thanks!).

As far as I can tell, this is what happens:

  1. Html parser sees <svg> tag and parses it in the SVG namespace here.
  2. This is supposed to trigger the case corrections for attributes in here.
  3. The correction function itself only corrects attributes that have the SVG namespace prefix here.

I don't know enough about namespaces and the use cases of this library, but from my POV there are two fixes:

  1. Add the SVG namespace to all attributes behind the scenes, similar to how the SVG namespace is added to the svg element implicitly.
  2. Ignore the namespace in adjust_svg_attributes because the decision to adjust attribute names is already made by the caller.

This PR adds a test with my own use case and passes it by choosing option 2. I also built and tested the tyxml repository against this PR which works fine.

@aantron aantron marked this pull request as ready for review July 13, 2023 13:58
@aantron aantron merged commit 70106f8 into aantron:master Jul 13, 2023
@aantron
Copy link
Owner

aantron commented Jul 13, 2023

Thank you! This was indeed a bug, and was contrary to the behavior in the spec.

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

Successfully merging this pull request may close these issues.

2 participants