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

HTML: remove early return in SVG processing #12425

Merged
merged 5 commits into from
Jul 10, 2024

Conversation

tuncbkose
Copy link
Contributor

This PR removes some code that was added a long time ago when docutils didn't support SVGs. Removing this chunk passed all the tests.

I encountered this when I was trying to use :height: and :width: options with SVGs. Due to the removed code, if one specifies a unit, like 5cm, it gets directly passed to the HTML attribute, which is incorrect. With this PR, the size handling is done by docutils, which uses the style attribute where one can specify units.

One thing to note is that :scale: option for SVGs still doesn't work due to #12415. If this PR is accepted as it is, due to not returning early, the use of :scale: option for SVGs may emit an additional warning from docutils's HTMLTranslator::image_size about PIL being required. I'm not sure what is the best way to deal with that.

Feature or Bugfix

  • Bugfix

Detail

  • Fixes specifying units in :height: and :width: options for SVG files.

@picnixz
Copy link
Member

picnixz commented Jun 12, 2024

You say that the tests pass but do we actually cover this case? If not, could you add a test covering the use of that option please? (just to see whether docutils is supporting the options correctly?)

@tuncbkose
Copy link
Contributor Author

@picnixz I've added tests checking for the use of the style attribute, which is what docutils uses in the HTML.

@chrisjsewell
Copy link
Member

thanks,
for reference, this is the upstream visit method: https://github.com/live-clones/docutils/blob/d54b59f99809d2b7d1ff78ff1185452fcb977971/docutils/docutils/writers/_html_base.py#L1145

In addition, to what @picnixz mentioned (understanding if the test cases cover the option use),
I would also just like to clarify here how/where in the code base docutils covers these options

@tuncbkose
Copy link
Contributor Author

In docutils's visit_image, the options are obtained via this method (more specifically on this line) and put into the style attribute right below the first link.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tuncbkose, please could you add an entry to CHANGES? Otherwise looks good!

A

@tuncbkose
Copy link
Contributor Author

@AA-Turner rebased and added an entry to CHANGES. Thanks for the review!

@AA-Turner
Copy link
Member

@tuncbkose a tip for the future -- open a PR from a feature branch, rather than master -- makes things much easier!

A

CHANGES.rst Outdated Show resolved Hide resolved
@AA-Turner AA-Turner enabled auto-merge (squash) July 10, 2024 18:29
CHANGES.rst Outdated Show resolved Hide resolved
@AA-Turner AA-Turner merged commit bdd9140 into sphinx-doc:master Jul 10, 2024
22 of 23 checks passed
@AA-Turner AA-Turner added this to the 7.4.0 milestone Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants