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

xmlns attribute is required for single SVG file #720

Closed
marmooo opened this issue Jun 28, 2024 · 3 comments
Closed

xmlns attribute is required for single SVG file #720

marmooo opened this issue Jun 28, 2024 · 3 comments

Comments

@marmooo
Copy link

marmooo commented Jun 28, 2024

#704 has a bug.

It is true that inline SVG does not require an xmlns attribute,
but it is required if we are loading the SVG file from HTML
or treating it as a single file.

If you open an SVG without an xmlns attribute in a browser,
you will see that the image will not be displayed.

@tdewolff
Copy link
Owner

Thanks for raising this issue. The XMLNS attribute should only be removed when the SVG document is "inline". This is only activated for SVG elements inside HTML. Otherwise, a regular SVG document is not considered inline and should thus not remove the XMLNS attribute. It is strange that this does happen for you, could you show me an example that reproduce this behaviour?

@marmooo
Copy link
Author

marmooo commented Jun 29, 2024

Sorry for the lack of examples.
After further investigation, it appears to be a random bug.
It seems to happen about 70% of the time.
Since it was not possible to check without a certain number of files,
I will provide an example of a repository where the error occurs.

go install github.com/tdewolff/minify/v2/cmd/minify@latest
git clone https://github.com/marmooo/touch-kanji
cd touch-kanji
minify src/favicon/favicon.svg  # --> ok
minify -r docs -o .  # --> sometimes fail

The easiest way to check is to enter the following commands:

cd touch-kanji
minify -r docs -o .
git status  # <-- if docs/favicon/favicon.svg is added, it's a bug. 

@tdewolff
Copy link
Owner

tdewolff commented Jul 2, 2024

Good point, this should be fixed in the latest release!

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