pypa / readme_renderer

Safely render long_description/README files in Warehouse
Apache License 2.0
158 stars 89 forks source link

Unify handling of SVG and other images in RST. #113

Closed mila closed 6 years ago

mila commented 6 years ago

ReadMeHTMLTranslator was overidden to not to render object tags for SVG. Unfortunately it produced different results than for PNG or other image types.

This a first pull request, which addresses #112. Once image handling is unified, other issues can can fixed.

theacodes commented 6 years ago

Hmm, based on https://sourceforge.net/p/docutils/bugs/247/, docutils should already be outputting img tags for svgs. Can you help me understand why we need object_image_types = {'.swf': 'application/x-shockwave-flash'}?

mila commented 6 years ago

I have docutils 0.14 installed and they still render <object> for svg.

ReadMeHTMLTranslator extends docutils.writers.html4css1.HTMLTranslator which extends docutils.writers._html_base.HTMLTranslator.

theacodes commented 6 years ago

Gotcha. Could we use a newer base translator?

On Sat, Jul 28, 2018, 10:31 AM Miloslav Pojman notifications@github.com wrote:

I have docutils 0.14 installed and they still render for svg.

ReadMeHTMLTranslator extends docutils.writers.html4css1.HTMLTranslator which extends docutils.writers._html_base.HTMLTranslator.

  • _html_base.HTMLTranslator sets object_image_types = {'.swf': 'application/x-shockwave-flash'}
  • html4css1.HTMLTranslator adds svg there {'.svg': 'image/svg+xml', '.swf': 'application/x-shockwave-flash'} with a comment that "html4css1 strives for IE6 compatibility"
  • I removed .svg and kept .swf there, just to make the change minimal.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/pypa/readme_renderer/pull/113#issuecomment-408623135, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPUc7IGCIwNQJ84LMM7hJwtjb7DVrJfks5uLJ_jgaJpZM4VlCcz .

mila commented 6 years ago

Gotcha. Could we use a newer base translator?

I'm not sure about that. There are two implementations: html4css1 and html5_polyglot. The first one is the one we are using now, the latter claims to "generates polyglot markup: HTML5 that is also valid XML."

The polyglot markup may be a better choice, but it generates different output and makes tests failing. So if the translator has to be changes, I would do that in a separate issue.

theacodes commented 6 years ago

Sounds pretty reasonable, we should do that in a follow-up.

I'd like @di to take a look at this as well, as anything that affects the sandboxed output here should be double-checked.

theacodes commented 6 years ago

Thank you, @mila!