Unify handling of SVG and other images in RST. #113
Conversation
ReadMeHTMLTranslator was overriden to not to render object tags for SVG. Unfortunately it produced different results than for PNG or other image types. With this commit, HTML code for SVG images will be same as for other images. It means that it won't have width, height or align attributes. But it will be better to address this issue for all images types at once.
|
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 |
|
I have docutils 0.14 installed and they still render
|
|
Gotcha. Could we use a newer base translator?
…On Sat, Jul 28, 2018, 10:31 AM Miloslav Pojman ***@***.***> wrote:
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.
- _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
<#113 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPUc7IGCIwNQJ84LMM7hJwtjb7DVrJfks5uLJ_jgaJpZM4VlCcz>
.
|
I'm not sure about that. There are two implementations: The polyglot markup may be a better choice, but it generates different output and makes tests failing. |
|
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. |
readme_renderer/rst.py
Outdated
| self.body.pop() | ||
| # add on `img` with attributes | ||
| self.body.append(self.starttag(node, "img", **atts)) | ||
| # We need to swap RST's use of `object` with `img` tags |
There was a problem hiding this comment.
Can you update this comment to mention that we're override the base class to not output svg with object?
readme_renderer/rst.py
Outdated
| # add on `img` with attributes | ||
| self.body.append(self.starttag(node, "img", **atts)) | ||
| # Overrides base class not to output `<object>` tag for SVG images. | ||
| object_image_types = {'.swf': 'application/x-shockwave-flash'} |
There was a problem hiding this comment.
Let's just make this object_image_types = {}, we don't actually want to support any <object> tags at all.
| @@ -0,0 +1 @@ | |||
| <img alt="alternate text" src="https://example.com/badge.svg"> | |||
There was a problem hiding this comment.
We're kind of experiencing a regression here, right? Before, this:
.. image:: https://example.com/badge.svg
:height: 100px
:width: 25.0%
:alt: alternate text
:align: rightwould become:
<img align="right" alt="alternate text" height="100px" src="https://example.com/badge.svg" width="25.0%">but now it becomes:
<img alt="alternate text" src="https://example.com/badge.svg">There was a problem hiding this comment.
Yes, but the current behavior is problematic:
align="right"does not actually align the image, CSS is necessary for that.height="100px"wont have any effect, because units (px) must be added to CSS, not image attributes- Those attributes are preserved for SVG images only (or more precisely for URLs ending ".svg")
#114 addresses image alignment and dimensions. Without unifying SVG with other image types, the subsequent PR would fix styling of non-SVG images only.
(I created two PRs because the second relaxes what HTML is allowed, so there can be more discussion.)
|
Thank you, @mila! |
This fixes a regression introduced in #113 where height/width attributes were removed from images defined in reStructuredText. More discussion here: #113 (comment)
This fixes a regression introduced in #113 where height/width attributes were removed from images defined in reStructuredText. More discussion here: #113 (comment)
This fixes a regression introduced in #113 where height/width attributes were removed from images defined in reStructuredText. More discussion here: #113 (comment)
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.