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

Fix some files are recognized as SVG incorrectly, make the diff UI unable to work #23310

Closed
wants to merge 2 commits into from

Conversation

stuzer05
Copy link

@stuzer05 stuzer05 commented Mar 5, 2023

SVG file displaying is broken, meanwhile also breaks other diff files (such as php html templates) from displaying in the diff ui

Every commit here does not display SVGs, and also showcaases broken php html template display
https://try.gitea.io/stuzer05/test-preview/commits/branch/master

So I removed "smart" svg detection at all

Closes #19733

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 5, 2023

SVG file displaying is broken

It should work (I manually constructed those files, one side is valid while other side is invalid)

image

meanwhile also breaks other diff files (such as php html templates) from displaying in the diff ui

What's the root problem of the breaking? eg: how the detection goes wrong?


Maybe a case is like this:

view-source:https://try.gitea.io/stuzer05/test-preview/raw/commit/e2f752e123151fea55959571d56d57462025f0f6/test.blade.php

<!-- BEGIN: filter namings -->
<div>
            <!-- prettier-ignore -->
            <svg xmlns="http://www.w3.org/2000/svg" width="10" height="10" viewBox="0 0 10 10">
                <path fill="#8E8E93" fill-rule="evenodd" d="M9.816 8.981L7.08 6.243c.49-.67.753-1.479.75-2.309C7.819 1.767 6.067.013 3.9 0 2.864-.005 1.868.406 1.135 1.14.403 1.872-.006 2.868 0 3.905.01 6.072 1.762 7.828 3.93 7.84c.833.004 1.645-.262 2.315-.758l.003-.002 2.735 2.736c.148.156.368.218.576.165.207-.054.37-.216.423-.424.054-.207-.01-.428-.165-.576zm-5.89-1.925C2.193 7.046.79 5.642.783 3.909c-.005-.83.322-1.626.908-2.213.586-.587 1.383-.915 2.212-.912 1.733.01 3.135 1.414 3.143 3.147.004.83-.323 1.626-.909 2.213-.586.587-1.382.915-2.211.912z"/>
            </svg>
</div>

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 5, 2023
@stuzer05
Copy link
Author

stuzer05 commented Mar 5, 2023

What's the root problem of the breaking? eg: how the detection goes wrong?

The initial problem is php files containing svg are not displaying as gitea returns svg mime type for them. The svg regex cause this issue
https://try.gitea.io/stuzer05/test-preview/commit/f97b3784f8c8efc222128f6dffd4e5abb5fda462

Example of the current regex used https://regex101.com/r/m1bCCP/1

image
image

And also on latest firefox (110.0.1) this is what I see

@wxiaoguang
Copy link
Contributor

The initial problem is php files containing svg are not displaying as gitea returns svg mime type for them. The svg regex cause this issue

Yup, so I guess the detection should be improved, instead of removed?

@stuzer05
Copy link
Author

stuzer05 commented Mar 5, 2023

Yup, so I guess the detection should be improved, instead of removed?

There's so many cases when browser won't display an svg (e.g. https://try.gitea.io/stuzer05/test-preview/raw/commit/72f4584ea11cf8579f57d1ab909a6d7d349bc4e7/test%20%28copy%201%29.svg), I think it's better to remove this smart parsing. It's just unreliable

@wxiaoguang
Copy link
Contributor

If remove the smart detection, will it make the SVG files won't be shown as image in the diff view?

@stuzer05
Copy link
Author

stuzer05 commented Mar 5, 2023

If remove the smart detection, will it make the SVG files won't be shown as image in the diff view?

Yeah. I think http.DetectContentType(data) does not detect svgs for a reason

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 5, 2023

Have you tried that? According to Golang's issue tracker:

https://github.com/golang/go/issues/15888#issuecomment-237295881

http.DetectContentType is documented to be an implementation of https://mimesniff.spec.whatwg.org/ and that mimesniff spec doesn't define svg.
And as you found, mime.TypeByExtension has ".svg" in its built-in table:
https://github.com/golang/go/blob/release-branch.go1.6/src/mime/type.go#L35
So I don't think there's anything to do here.


I will take a look at this problem , and fix some more SVG related problems.

@stuzer05
Copy link
Author

stuzer05 commented Mar 5, 2023

Have you tried that? According to Golang's issue tracker:

https://github.com/golang/go/issues/15888#issuecomment-237295881

http.DetectContentType is documented to be an implementation of https://mimesniff.spec.whatwg.org/ and that mimesniff spec doesn't define svg.
And as you found, mime.TypeByExtension has ".svg" in its built-in table:
https://github.com/golang/go/blob/release-branch.go1.6/src/mime/type.go#L35
So I don't think there's anything to do here.

I will take a look at this problem , and fix some more SVG related problems.

No, I didn't check that. Maybe we can detect svg by file extension alone. That should actually cover most cases. Embedded svgs shouldn't really be detected (e.g. in html) as we don't want to display html content in diff preview

@stuzer05
Copy link
Author

stuzer05 commented Mar 5, 2023

Have you tried that? According to Golang's issue tracker:

https://github.com/golang/go/issues/15888#issuecomment-237295881

http.DetectContentType is documented to be an implementation of https://mimesniff.spec.whatwg.org/ and that mimesniff spec doesn't define svg.
And as you found, mime.TypeByExtension has ".svg" in its built-in table:
https://github.com/golang/go/blob/release-branch.go1.6/src/mime/type.go#L35
So I don't think there's anything to do here.

I will take a look at this problem , and fix some more SVG related problems.

Thank you! Maybe if you could make svg regex not match anything between html comments, except spaces and \n, that would resolve the initial bug. As of right now the bug is caused by the last html comment near <svg tag https://regex101.com/r/m1bCCP/1

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 5, 2023

The problem is that the detector fails on this test:

	assert.False(t, DetectContentType([]byte(`
<!-- comment1 -->
<div>
	<!-- comment2 -->
	<svg></svg>
</div>
`)).IsSvgImage())

the regexp was quite tricky before.

@wxiaoguang
Copy link
Contributor

I proposed a new PR for SVG/ImageDiff fix: Fix various ImageDiff/SVG bugs #23312

@stuzer05 stuzer05 closed this Mar 5, 2023
@stuzer05 stuzer05 deleted the fix-19733 branch March 5, 2023 14:55
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some files are recognized as SVG incorrectly, make the diff UI unable to work.
3 participants