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.

Sign up for GitHub

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

Jump to bottom

implement #237 add support for svg files in img element #254

Merged
merged 1 commit into from
Jul 16, 2018
Merged

implement #237 add support for svg files in img element #254

merged 1 commit into from
Jul 16, 2018

Conversation

syjer
Copy link
Contributor

@syjer syjer commented Jul 14, 2018 •
edited
Loading

Hi @danfickle , with this PR, we can use svg files in img tag.

I'm not sure if it's the best way, but it seems an unobtrusive change, so it feel kinda right :)

Additionally, I've added batik-codec as a dependency, so we can support base64 encoded images in the svg (as in the case of the svg linked in the issue #237).

I've tested with both the java2d and pdfbox backend.

Note: the dimension of the rendered svg (both as a svg and img element) is not handled like chrome or FF. But currently I don't know why. You can try with https://docs.chef.io/_images/start_chef.svg .

Rendered by pdfbox:

pdfbox

Rendered by browser:

chrome

The code of the page is simply:

<html>

<body>
top
<p><img style="border:1px solid red;" src="https://docs.chef.io/_images/start_chef.svg" alt="Chef Flow" /></p>
bottom
</body>
</html>

edit: ok, there is already an issue (#128) I'll try to investigate then.

@rototor
Copy link
Contributor

rototor commented Jul 15, 2018

Browsers usually use the content type and/or sniffer the file content to detect which "renderer" is needed for an image. That would be the "correct" way to handle this, but also would be way more complex to implement. So as a start I think it is ok to sniffer on the file extension.

As long as someone does not work with a content management system, which does not provide URLs with the right extensions, this should work fine.

Looks good to me.

@syjer
Copy link
Contributor Author

syjer commented Jul 15, 2018

@rototor Yes, my initial idea was to check the content-type. but I didn't find any helpers/method in the UserAgentCallback.

@danfickle
Copy link
Owner

I think that in the spirit of “perfect is the enemy of good enough” that using the url extension is ok. My only other concern is that allowing the use of external svg images might imply that arbitrary svg files are safe, which is not the case. However, we can address that in the documentation which I think I will move to a wiki to allow easier upkeep.

Anyway, huge thanks to @syjer and I’ll merge this now.

As to the size issue, I believe it defaults to 400 x 400 while the browsers default to a width of 100% with height determined by implicit aspect ratio. Possibly we could achieve this with a entry in the default style sheet. Can’t test now but will look later.

Also thanks to @rototor for reviewing.

@danfickle danfickle merged commit 8a3168d into danfickle:open-dev-v1 Jul 16, 2018
@syjer syjer deleted the svg-src-img branch July 16, 2018 08:38
@danfickle danfickle mentioned this pull request Aug 6, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@syjer @rototor @danfickle