quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.79k stars 310 forks source link

SVG are loosing classes when using `embed-resources`, impacting revealjs features (`class="slide-logo"`, `.absolute`) #9803

Open mcanouil opened 4 months ago

mcanouil commented 4 months ago

Using Quarto 122b0f3093c560a6c24365fc9241bb83d4590ef4 It works as expected using v1.4.554

Expected behaviour

Quarto documentHTML
````qmd --- title: "Quarto Playground" format: revealjs: logo: https://raw.githubusercontent.com/mcanouil/hex-stickers/main/SVG/rlille.svg embed-resources: false --- ## Slide This is a playground for Quarto. ![An image](https://placehold.co/600x400.png) ```` image

Actual behaviour

Quarto documentHTML
````qmd --- title: "Quarto Playground" format: revealjs: logo: https://raw.githubusercontent.com/mcanouil/hex-stickers/main/SVG/rlille.svg embed-resources: true --- ## Slide This is a playground for Quarto. ![An image](https://placehold.co/600x400.png) ```` image

The class="slide-logo" is lost when embedding.

cderv commented 3 months ago

This is Pandoc issue IMO so if it was working before probably following an update of pandoc

Try

❯ quarto pandoc --to html --embed-resources -o embed.html
<img src="https://raw.githubusercontent.com/mcanouil/hex-stickers/main/SVG/rlille.svg" class="slide-logo">
^Z

And look in embed.html - the class is missing.

cderv commented 3 months ago

But doing the same with Quarto 1.4.554 works - so this is a change in Pandoc.

cderv commented 3 months ago

1.4.554 uses pandoc 3.1.11.1 and latest use pandoc 3.2

And we can determine that change happens in Pandoc 3.2

library(pandoc)

min <- "3.1.11"
max <- "3.2"

versions <- pandoc::pandoc_available_releases()
#> ℹ Fetching Pandoc releases info from github...
versions <- versions[seq.int(which(versions == min), which(versions == max))]

for(version in versions) {
  if (!pandoc::pandoc_is_installed(version)) {
    pandoc::pandoc_install(version)
  }
  embed <- pandoc::pandoc_convert(
    text = '<img src="https://raw.githubusercontent.com/mcanouil/hex-stickers/main/SVG/rlille.svg" class="slide-logo">',
    to = "html",
    args = "--embed-resources", 
    version = version)
  if (!any(grepl("slide-logo", embed))) {
    cat("pandoc version ", version, " does not keep classes")
    break
  }
}
#> pandoc version  3.2  does not keep classes

Created on 2024-05-31 with reprex v2.1.0

cderv commented 3 months ago

Probably related to @cscheid PR in pandoc

so we may have broken ourself Quarto through Pandoc by trying to fix another issue 😅

Though I don't see how merging would mess this up, because we should have kept the class. So either if no class on the svg, it does not keep image class.

Or this is a side effect of this change somewhere else

cderv commented 3 months ago

Testing with another svg from pandoc repo

❯ quarto pandoc --to html --embed-resources -o embed.html
```{=html}
<img class="something" src="https://raw.githubusercontent.com/jgm/pandoc/7abfec3d2c0e6084b8029d38cd45912697822f3a/test/command/SVG_logo.svg" />

^Z


I don't get the class name either 
````html
<svg id="svg_7e8f68c20ccd090c0566" role="img" viewBox="-50 -50 100 100" width="100" height="100">
<title>SVG Logo</title>

So it seems when no class on the svg, the one from img tag is lost.

@cscheid I don't know Haskell enough to know if your change is supposed to handle this case.

cscheid commented 3 months ago

Hm - if this is a Pandoc regression (even one I introduced 🤦) we won't be able to fix it for 1.5.

cderv commented 3 months ago

What about doing a "hack" to at least restore behavior ? I have a branch working with the following

This would not solve the underlying svg issue, but this would "hide" this issue until we have a proper fix. I can make a PR with the idea.

cscheid commented 3 months ago

I don't know. This would only fix it in logo and format: revealjs, right? The bug would still exist in other usages of embed-resources. I'm not sure it's worth the extra complexity in the code base, that would then need to be reverted once upstream is fixed.

cderv commented 3 months ago

This would only fix it in logo and format: revealjs, right?

Yes exactly.

I'm not sure it's worth the extra complexity in the code base, that would then need to be reverted once upstream is fixed.

That is the question. I'll share the idea in a PR and you'll be the judge of that.

cderv commented 3 weeks ago

Here is one use case where another class is lost: When using .absolute class for positioning on an svg element. See

---
title: test SVG
format: revealjs
embed-resources: true
---

## Slide 1

![](https://quarto.org/docs/presentations/revealjs/demo/mini/images/kitten-450-250.jpeg){.absolute top=50 right=50 width="450" height="250"}

![](https://dev.w3.org/SVG/tools/svgweb/samples/svg-files/410.svg){.absolute top=50 right=50 width="450" height="250"}