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

Use an <img> for SVG output instead of an <object> #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tsibley
Copy link

@tsibley tsibley commented Nov 9, 2023

<img>, unlike <object>, has proper alt text support (i.e. that's not just fallback content) and is easily saved, opened in a new tab, etc. from the page where it appears.

This does mean the SVG's DOM will no longer be available for inspection by JavaScript in the containing document (e.g. object.contentDocument), but I don't think that was intended to be a feature of SVG output for this Sphinx extension.

Resolves #102 as well.

<img>, unlike <object>, has proper alt text support (i.e. that's not
just fallback content) and is easily saved, opened in a new tab, etc.
from the page where it appears.

This does mean the SVG's DOM will no longer be available for inspection
by JavaScript in the containing document (e.g. object.contentDocument),
but I don't think that was intended to be a feature of SVG output for
this Sphinx extension.

Resolves <mgaitan#102>
as well.
tsibley added a commit to nextstrain/cli that referenced this pull request Nov 21, 2023
Mermaid.js¹ supports a number of useful diagrams types that are
otherwise difficult to create programmatically.  The addition now is
motivated by wanting to add some sequence diagrams for new authn
documentation.

The Sphinx/Mermaid integration, sphinxcontrib-mermaid, defaults to
client-side rendering at page load time, but this comes with some
downsides and practically no upsides.  Configure it to render at doc
build time instead.

Related to this change, I submitted two patches upstream to the Sphinx
integration which I think improve the build-time render results: a patch
to remove unnecessary JS from the page by default² and a patch to
display the SVGs as images rather than embedded documents.³

¹ <https://mermaid.js.org>
² <mgaitan/sphinxcontrib-mermaid#132>
³ <mgaitan/sphinxcontrib-mermaid#131>
tsibley added a commit to nextstrain/cli that referenced this pull request Nov 21, 2023
Mermaid.js¹ supports a number of useful diagrams types that are
otherwise difficult to create programmatically.  The addition now is
motivated by wanting to add some sequence diagrams for new authn
documentation.

The Sphinx/Mermaid integration, sphinxcontrib-mermaid, defaults to
client-side rendering at page load time, but this comes with some
downsides and practically no upsides.  Configure it to render at doc
build time instead.

Related to this change, I submitted two patches upstream to the Sphinx
integration which I think improve the build-time render results: a patch
to remove unnecessary JS from the page by default² and a patch to
display the SVGs as images rather than embedded documents.³

¹ <https://mermaid.js.org>
² <mgaitan/sphinxcontrib-mermaid#132>
³ <mgaitan/sphinxcontrib-mermaid#131>
tsibley added a commit to nextstrain/cli that referenced this pull request Nov 21, 2023
Mermaid.js¹ supports a number of useful diagrams types that are
otherwise difficult to create programmatically.  The addition now is
motivated by wanting to add some sequence diagrams for new authn
documentation.

The Sphinx/Mermaid integration, sphinxcontrib-mermaid, defaults to
client-side rendering at page load time, but this comes with some
downsides and practically no upsides.  Configure it to render at doc
build time instead.

Related to this change, I submitted two patches upstream to the Sphinx
integration which I think improve the build-time render results: a patch
to remove unnecessary JS from the page by default² and a patch to
display the SVGs as images rather than embedded documents.³

¹ <https://mermaid.js.org>
² <mgaitan/sphinxcontrib-mermaid#132>
³ <mgaitan/sphinxcontrib-mermaid#131>
tsibley added a commit to nextstrain/cli that referenced this pull request Nov 22, 2023
Mermaid.js¹ supports a number of useful diagrams types that are
otherwise difficult to create programmatically.  The addition now is
motivated by wanting to add some sequence diagrams for new authn
documentation.

The Sphinx/Mermaid integration, sphinxcontrib-mermaid, defaults to
client-side rendering at page load time, but this comes with some
downsides and practically no upsides.  Configure it to render at doc
build time instead.

Related to this change, I submitted two patches upstream to the Sphinx
integration which I think improve the build-time render results: a patch
to remove unnecessary JS from the page by default² and a patch to
display the SVGs as images rather than embedded documents.³

¹ <https://mermaid.js.org>
² <mgaitan/sphinxcontrib-mermaid#132>
³ <mgaitan/sphinxcontrib-mermaid#131>
tsibley added a commit to nextstrain/cli that referenced this pull request Nov 22, 2023
Mermaid.js¹ supports a number of useful diagrams types that are
otherwise difficult to create programmatically.  The addition now is
motivated by wanting to add some sequence diagrams for new authn
documentation.

The Sphinx/Mermaid integration, sphinxcontrib-mermaid, defaults to
client-side rendering at page load time, but this comes with some
downsides and practically no upsides.  Configure it to render at doc
build time instead.

Related to this change, I submitted two patches upstream to the Sphinx
integration which I think improve the build-time render results: a patch
to remove unnecessary JS from the page by default² and a patch to
display the SVGs as images rather than embedded documents.³

¹ <https://mermaid.js.org>
² <mgaitan/sphinxcontrib-mermaid#132>
³ <mgaitan/sphinxcontrib-mermaid#131>
tsibley added a commit to nextstrain/cli that referenced this pull request Jan 13, 2024
Mermaid.js¹ supports a number of useful diagrams types that are
otherwise difficult to create programmatically.  The addition now is
motivated by wanting to add some sequence diagrams for new authn
documentation.

The Sphinx/Mermaid integration, sphinxcontrib-mermaid, defaults to
client-side rendering at page load time, but this comes with some
downsides and practically no upsides.  Configure it to render at doc
build time instead.

Related to this change, I submitted two patches upstream to the Sphinx
integration which I think improve the build-time render results: a patch
to remove unnecessary JS from the page by default² and a patch to
display the SVGs as images rather than embedded documents.³

¹ <https://mermaid.js.org>
² <mgaitan/sphinxcontrib-mermaid#132>
³ <mgaitan/sphinxcontrib-mermaid#131>
tsibley added a commit to nextstrain/cli that referenced this pull request Jan 18, 2024
Mermaid.js¹ supports a number of useful diagrams types that are
otherwise difficult to create programmatically.  The addition now is
motivated by wanting to add some sequence diagrams for new authn
documentation.

The Sphinx/Mermaid integration, sphinxcontrib-mermaid, defaults to
client-side rendering at page load time, but this comes with some
downsides and practically no upsides.  Configure it to render at doc
build time instead.

Related to this change, I submitted two patches upstream to the Sphinx
integration which I think improve the build-time render results: a patch
to remove unnecessary JS from the page by default² and a patch to
display the SVGs as images rather than embedded documents.³

¹ <https://mermaid.js.org>
² <mgaitan/sphinxcontrib-mermaid#132>
³ <mgaitan/sphinxcontrib-mermaid#131>
@jougs
Copy link

jougs commented Dec 18, 2024

One possible problem with this is that using <img> instead of <object> makes it impossible to select text in the figure and click on embedded links, which are both things I use a lot and would not want to miss.

Maybe this could be made configurable via a setting in conf.py? Something like mermaid_inline_svg = True | False would probably be a good compromise.

@tsibley
Copy link
Author

tsibley commented Dec 18, 2024

@jougs Noted! One thing I'll point out (in case it's not clear) is that the change in this PR only applies when the SVG is generated at build time instead of dynamically on page load, and build-time generation is not the default mode of operation for this Sphinx extension.

Making it configurable should be no problem, though, and sounds worthwhile. With mermaid_inline_svg = True, I'd expect that instead of emitting an <img> tag pointing at the external SVG file generated at build time, the contents of the SVG file would be emitted directly instead (sans doctype, if present). So it'd still not be using <object>. Does this match your expectation?

That said, I'm unlikely to actually do more work on this PR until @mgaitan or another maintainer of this project gives the nod that they're interested in accepting and releasing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transparency failing for svg output
2 participants