Inconsistencies and problems in when images are rendered from RST

[mila]

Investigating https://github.com/pypa/warehouse/issues/4023, I found multiple related problems:

• HTML code for SVG images can be quite different from other image types.
• For SVG, width and height are assigned to width="..." and height="..." attributes, so relative dimensions cannot work.
• For non-SVG image types, width and height are ignored.
• For SVG images, align="..." attribute is set, which does not help with styling.
• For non-SVG images, align is ignored.
• For most images, alt="" fallbacks to image URL, not for SVG.
• Detection of SVG is broken, e.g. URL like .../django-multi-gtfs.svg?branch=master is not detected as SVG.

[di]

@mila I think this is fixed now that #113 and #114 are merged and released (https://pypi.org/project/readme_renderer/22.0/), can you close if that's accurate, or update this with anything that's currently outstanding?

[mila]

It would be great to deploy Warehouse using the new version. The goal is to close https://github.com/pypa/warehouse/issues/4023.

Unless I missed something, there should be no more work in readme_renderer. So, I'm closing this issue.

[dstufft]

I'm reopening this, because the fix in https://github.com/pypa/readme_renderer/pull/114 isn't going to work on PyPI due to our restrictive CSP policy which disallows inline styles. We're not going to relaxing that, so in order for this to actually work, readme_renderer will need to emit css classes in all cases.

[mila]

Good catch. I'm afraid that it's impossible to define css classes for each possible width or height. I see two options:

1) Do not support setting of width or height. class should be removed from allowed attributes to prevent CSP violations.

2) Patch rst renderer to generate width="..." and height="..." instead of style="...". Only absolute (px) dimensions would be supported.

There should be no problem with image alignment, it's already implemented using css classes.

[dstufft]

I guess I'd say that (2) is probably the best option here, unless we think that absolute dimension part is no go, then maybe (1).

Although maybe we could do something like (2), but also add a couple common sizes for relative dimensions that people can use too? I'm not sure how needed these things are TBH, so I don't know if either of these things are worth the effort or not.

I guess we should probably also ping @nlhkabu for input on whether mandating absolute sizing is going to break the UX or not.

[mila]

(1) has to be done in any case and #121 should make the job.

It would be nice to have (2), but it can be done later. pypa/warehouse#4023 is about image alignment and sizing is not necessary for that.

width="..." and height="..." were set for SVG images only and were buggy, because it's necessary to strip unit. So I would not consider this a regression.

[dstufft]

Ah good point, I'll go ahead and reclose that issue since alignment is still working correctly.

[mila]

Thanks, closing.