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

Lightbox and RevealJS : image size problem #11022

Closed
pagiraud opened this issue Oct 9, 2024 · 9 comments
Closed

Lightbox and RevealJS : image size problem #11022

pagiraud opened this issue Oct 9, 2024 · 9 comments
Labels
support a request for support

Comments

@pagiraud
Copy link

pagiraud commented Oct 9, 2024

Bug description

The lightbox class can be used for generating RevealJS presentations, which is very convenient to be able to “zoom” on certain images / figures. But when you do that, you lose the “autosize” for your images (I don’t know how it is called, it is the fact that an image, by default, can’t vertically overflow your slide) and so, often, you have to manually put a vertical size (typically between 500 and 550 for me) if you want to avoid a scrollbar. This has drawbacks when exporting to other formats (such as PDF with the article class) because then, the images are quite small.

Steps to reproduce

Example:

---
title: "Untitled"
format: revealjs
scrollable: true
editor: visual
---

## Without lightbox

![Some random image](https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg)

## With lightbox

![Some random image with lightbox](https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg){.lightbox}

Expected behavior

I think the two slides in the example above should look and behave exactly the same, except for the ability to click on the image on the second one.

Actual behavior

The two images are not of the same size.

Your environment

RStudio 2024.09.0 build 375

Quarto check output

Quarto 1.5.57
[✓] Checking versions of quarto binary dependencies...
      Pandoc version 3.2.0: OK
      Dart Sass version 1.70.0: OK
      Deno version 1.41.0: OK
      Typst version 0.11.0: OK
[✓] Checking versions of quarto dependencies......OK
[✓] Checking Quarto installation......OK
      Version: 1.5.57
      Path: /opt/quarto/bin

[✓] Checking tools....................OK
      Chromium: 869685
      TinyTeX: (not installed)

[✓] Checking LaTeX....................OK
      Using: Installation From Path
      Path: /usr/bin
      Version: 2023

[✓] Checking basic markdown render....OK

[✓] Checking Python 3 installation....OK
      Version: 3.12.3
      Path: /usr/bin/python3
      Jupyter: (None)

      Jupyter is not available in this Python installation.
      Install with python3 -m pip install jupyter

[✓] Checking R installation...........OK
      Version: 4.4.1
      Path: /usr/lib/R
      LibPaths:
        - /home/xxxxxxxxx/R/x86_64-pc-linux-gnu-library/4.4
        - /usr/local/lib/R/site-library
        - /usr/lib/R/site-library
        - /usr/lib/R/library
      knitr: 1.48
      rmarkdown: 2.28

[✓] Checking Knitr engine render......OK
@pagiraud pagiraud added the bug Something isn't working label Oct 9, 2024
@mcanouil
Copy link
Collaborator

mcanouil commented Oct 9, 2024

You need to re-enable stretch: https://quarto.org/docs/presentations/revealjs/advanced.html#stretch
As auto-stretch doesn't work on non default (top-level) images such as .lightbox.
(Note that this is linked from "content overflow" https://quarto.org/docs/presentations/revealjs/index.html#content-overflow)

![Some random image with lightbox](https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg){.lightbox .r-stretch}

To me, there is no bug.

@mcanouil mcanouil added support a request for support and removed bug Something isn't working labels Oct 9, 2024
@cscheid
Copy link
Collaborator

cscheid commented Oct 9, 2024

Auto-stretch behavior in revealjs is a constant source of confusion, but I agree with @mcanouil here. We don't want auto-stretch on images with any other classes because those classes almost certainly indicate some intended behavior that will interact badly with .r-stretch.

I'm going to go ahead and close this one.

In the future, if you're not certain this is a bug, it helps us if you start with a discussion question first.

@cscheid cscheid closed this as not planned Won't fix, can't repro, duplicate, stale Oct 9, 2024
@pagiraud
Copy link
Author

Thanks for your answers. @mcanouil , I already tried to re-enable auto-stretch and it doesn’t work. The image is well stretched, but the lightbox doesn’t work, because it is not present in generated HTML.

![Some random image with lightbox](https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg){.lightbox}

gives

<div class="quarto-figure quarto-figure-center">
<figure>
<p><a href="https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg" class="lightbox" data-gallery="quarto-lightbox-gallery-1" title="Some random image with lightbox"><img data-src="https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg" alt="Some random image with lightbox"></a></p>
<figcaption>Some random image with lightbox</figcaption>
</figure>
</div>
![Some random image with lightbox](https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg){.lightbox .r-stretch}

gives

<img data-src="https://cdn.pixabay.com/photo/2023/04/20/12/22/globe-7939725_960_720.jpg" class="r-stretch quarto-figure-center" alt="Some random image with lightbox"><p class="caption">Some random image with lightbox</p>

From what @cscheid says, I guess it might be the intended behavior but then, if not a bug, the possibility to manually re-enable .r-stretch would be an enhancement. So I think it would be a good idea to re-open the issue.

To me the fact that it was a bug made no doubt but in the future I will start with a discussion.

@mcanouil
Copy link
Collaborator

I see.
The issue is actually not about the stretching as it works.
The issue is that setting .r-stretch "removes" .lightbox treatment or forbid Quarto to apply it.

For clarity and because that's not what this issue reported, I would open a new issue focussing on the conflict between .r-stretch (done via TypeScript) and .lightbox (done via a Lua filter).
As .r-stretch is Reveal.js specific, @cderv might have other insights.

@cderv
Copy link
Collaborator

cderv commented Oct 10, 2024

the possibility to manually re-enable .r-stretch would be an enhancement.

To be clear, the stretch feature is a revealjs feature: https://revealjs.com/layout/#stretch
It has some limitations in its implementation, and it is not compatible with images processed for lightbox treatment.

So this would be a Revealjs feature request, until we decide to reimplement the stretch feature ourself. Currently this is not the case and we inherit the stretch feature from Revealjs library.

@mcanouil
Copy link
Collaborator

Thanks @cderv, I did not know stretch was from Reveal.js.

@pagiraud
Copy link
Author

Thanks to all for these answers. It seems to be a tricky problem as it involves some frictions between components of your own and upstream features. @mcanouil , do you still think I should open a new bug here?

This may be none of my business and my expertise in OSS development is very limited but according to my own experience and what I’ve been able to observe in some communities / businesses, a good rule of thumb is to contribute as upstream as you can as often as you can, precisely to avoid this kind of issue (that can appear over time, with upstream upgrades breaking compatibility with your work). So, instead of reimplementing a feature (=internalizing), I think if I was in your position I would choose to propose lightbox as a native RevealJS feature (=externalising). It may cost more in terms of works of hours in the short term, but I tend to think it is more profitable in the long run. Please forgive me for my two cents. :)

@mcanouil
Copy link
Collaborator

No, see why in #11022 (comment)

@cderv
Copy link
Collaborator

cderv commented Oct 14, 2024

So, instead of reimplementing a feature (=internalizing), I think if I was in your position I would choose to propose lightbox as a native RevealJS feature (=externalising). It may cost more in terms of works of hours in the short term, but I tend to think it is more profitable in the long run.

In general I can see how the idea is appealing. However, in context of Quarto, it is is not that interesting because Lightbox feature is a Quarto feature in the sense it is common to all HTML format we have. So even if it is implemented as revealjs level, we would still need to maintain lightbox working for other HTML format we have.
Some differences could appear over time.

Though it could have benefits to have it included in revealjs directly and just leverage that.

Anyhow, unfortunately we don't have the power yet to contribute such work upstream considering our current plans on quarto itself. If anyone contributes it, we'll be happy to work integration in a future revealjs release.

For now best workaround is to manually set size when using Lightbox.

Thanks again for your feedback @pagiraud !

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

No branches or pull requests

4 participants