quarto-dev / quarto-cli

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

R Leaflet legends are centered when labelled as `fig-`s #7843

Open jack-davison opened 9 months ago

jack-davison commented 9 months ago

Bug description

When a leaflet map is included in a Quarto HTML report, any categorical legend ends up centered when the chunk is labelled as a figure. This looks a little strange/ugly, particularly when the labels are either very different lengths or are only subtly different lengths. Oddly, if you just print the leaflet map without labelling it as a figure, everything works okay.

Note that you can manually fix this by adding this to a style tag, but I imagine this isn't the intended fix, and that messing around with quarto's style options may have unintended consequences.

.quarto-figure-center>figure>p, .quarto-figure-center>figure>div {
    text-align: left;
}

No idea if this happens with Python/Folium, but I intuitively expect so?

Steps to reproduce

---
title: Dodgy leaflet
format: html
---

```{r}
#| label: makemap
library(leaflet)

map <-
  leaflet() |> 
  addTiles() |> 
  addLegend(
    labels = c("Big long label oh no!", "lil' lab"), 
    colors = c("red", "blue")
  )
#| label: fig-leaflet
#| fig-cap: "A leaflet figure"
map
#| label: nolab
map

![image](https://github.com/quarto-dev/quarto-cli/assets/45171616/51e68ca2-0e44-418b-9436-5aeb9d7c20cd)

### Expected behavior

The leaflet legend should be left aligned (bottom of image).

### Actual behavior

The leaflet legend is centered (top of image).

### Your environment

- RStudio 2023.03.0+386
- Windows 10

### Quarto check output

```bash
$ quarto check
A new release of Deno is available: 1.28.2 → 1.38.5 Run `deno upgrade` to install it.
[>] Checking versions of quarto binary dependencies...
      Pandoc version 3.1.1: OK
      Dart Sass version 1.55.0: OK
[>] Checking versions of quarto dependencies......OK
[>] Checking Quarto installation......OK
      Version: 1.3.450
      Path: C:\Users\jd38\AppData\Local\Programs\Quarto\bin
      CodePage: 1252

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

[>] Checking Python 3 installation....(None)

      Unable to locate an installed version of Python 3.
      Install Python 3 from https://www.python.org/downloads/

[>] Checking R installation...........OK
      Version: 4.2.3
      Path: C:/Users/jd38/AppData/Local/Programs/R/R-4.2.3
      LibPaths:
        - C:/Users/jd38/AppData/Local/R/win-library/4.2
        - C:/Users/jd38/AppData/Local/Programs/R/R-4.2.3/library
      knitr: 1.44
      rmarkdown: 2.25

[>] Checking Knitr engine render......OK

NB: I'm using R leaflet v2.2.0.9000.

cscheid commented 9 months ago

Thanks for the report. This is indeed the CSS we use to style Figure captions and labels, so we can't change it on our side.

Immediately, a better CSS workaround for you is this:

figure.quarto-float div.leaflet-control {
  text-align: left;
}

I'm wondering how we can address this. @cderv is there a way to check, at the end of knitr execution, if user code attached a package? The good fix here would be for us to detect that someone used leaflet, and then add a css sheet of fixups.

cderv commented 9 months ago

We had issues like that in the past (https://github.com/rstudio/rmarkdown/issues/1949) and it happens that R package leaflet had already some patched CSS rules.

Adding there

div.leaflet-control {
  text-align: left;
}

solves this - which is similar fix we did for rstudio/rmarkdown#1949

So I would be inclined to do a PR there to change this instead of us trying to detect leaflet.

Would that be ok with @cscheid or do you want us really to detect that ?

cscheid commented 9 months ago

Oh, leaflet is from Posit? 🤦 yeah, we should totally fix that upstream!

cderv commented 9 months ago

@cscheid in fact, another ideas:

We could also just adapt our CSS rules for Quarto figures to not apply on div of class html-widget. I believe all HTML widgets output created with R will have a div of this class.

For example, the rule that causes issue here is

.quarto-figure-center>figure>p, .quarto-figure-center>figure>div {
    text-align: center;
}

And so we could do

.quarto-figure-center>figure>p, .quarto-figure-center>figure>div:not(.html-widget) {
    text-align: center;
}

This should prevent issues with any other HTML widgets. What do you think ?

cderv commented 9 months ago

Oh no that wouldn't work just like that... 😓 sorry

jack-davison commented 9 months ago

@cscheid in fact, another ideas:

We could also just adapt our CSS rules for Quarto figures to not apply on div of class html-widget. I believe all HTML widgets output created with R will have a div of this class.

For example, the rule that causes issue here is

.quarto-figure-center>figure>p, .quarto-figure-center>figure>div {
    text-align: center;
}

And so we could do

.quarto-figure-center>figure>p, .quarto-figure-center>figure>div:not(.html-widget) {
    text-align: center;
}

This should prevent issues with any other HTML widgets. What do you think ?

I'd support a widget-agnostic approach if possible, I've had similar issues with {timevis} (not from Posit, I believe) that were resolved by fudging the CSS at the top of the doc, which I can put in a separate issue. But good to hear that the {leaflet} fix isn't too hard!

cderv commented 9 months ago

This would be the rule that works

.quarto-figure-center > figure > p,
.quarto-figure-center > figure > div:not(.html-widget, :nth-child(1):not(.html-widget)) /* for mermaid and dot diagrams */ {
  text-align: center;
}

With need the nth-child(1) because of the way Quarto wraps those figure into a specific div enclosing the one for the HTML widget . Example

<div aria-describedby="fig-plotly-caption-0ceaefa1-69ba-4598-a22c-09a6ac19f8ca">

I'll make a PR with this fix.

gadenbuie commented 9 months ago

What are the .quarto-figure-center>figure>p, .quarto-figure-center>figure>div selectors intending to target?

IMHO these selectors are too broad for the text-align property, since that's an inherited property and its value will cascade to descendants. In the reported reprex, the element where text-align: center is being applied is 4 children below the element that matches .quarto-figure-center > figure > div.

cscheid commented 9 months ago

What are the .quarto-figure-center>figure>p, .quarto-figure-center>figure>div selectors intending to target?

It's intended to target (for example) the div emitted by mermaidjs. Clearly the selectors are being too broad, but I'm not sure what the clean fix would be.

cderv commented 9 months ago

Let's go back to context of this rule addition maybe. It was added in

So if I understand correctly, the aim is to get the SVG mermaid content aligned.

We could probably use a more targeted rule for this. What about ?

.quarto-figure-center>figure>div:has(svg.mermaid-js) {
    text-align: center
}
gadenbuie commented 9 months ago

@cderv Do you have a reprex for centered mermaid diagrams? I might be doing something wrong, but I can't get fig-align: center to work. Here's my example document, which I've tried without success with quarto 1.3.450 and 1.4.451 (latest dev release).

---
title: Quarto 7843
format:
  html:
    fig-align: center
---

Nulla et excepteur culpa voluptate incididunt aliquip veniam quis elit sunt deserunt quis enim.

```{mermaid}
%%| label: mermaid-diagram
%%| fig-cap: A mermaid diagram in its natural habitat.
%%| fig-align: center
flowchart TB
a --> b
b --> c

Ea officia reprehenderit consectetur voluptate minim ex ea exercitation fugiat eu proident amet cillum minim.



Side note: I also noticed the caption text has different styles when the cell `label` is removed.
cscheid commented 9 months ago

which I've tried without success with quarto 1.3.450 and 1.4.451 (latest dev release).

FYI, v1.4.525 -- and I think I just fixed those mermaid issues between 451 and 525.

gadenbuie commented 9 months ago

Thanks -- v1.4.525 is the one I downloaded and installed but qvm and I have been fighting lately 😞

cscheid commented 9 months ago

Ah. The issue is that you have a small bug in your label: you need fig-mermaid-diagram. Then:

image
gadenbuie commented 9 months ago

Oh! That makes sense now. Is this the expected appearance with the fig caption left-aligned and the mermaid diagram centered?

cderv commented 9 months ago

Sorry wasn't around - Yes that was it - you need to use the fig- prefix for it to be a figure and the figure centering to apply.

This is current limitaton because of a figure node requirement. This may evolved in the future

gadenbuie commented 9 months ago

If the last screenshot looks okay, then here's another set of rules that are worth considering:

.quarto-figure > figure > div {
  display: block;
  width: fit-content;
}

.quarto-figure-center > figure > div {
  margin-left: auto;
  margin-right: auto;
}

.quarto-figure-left > figure > div {
  margin-right: auto;
}

.quarto-figure-right > figure > div {
  margin-left: auto;
}

This seems pretty reasonable for htmlwidgets in general, but you could also make those rules more specific to mermaid by targeting .quarto-figure-* > figure svg.mermaid-js.

Edit: to clarify, these would replace the current .quarto-figure-* > figure > div rules.

cscheid commented 9 months ago

This seems pretty reasonable for htmlwidgets in general, but you could also make those rules more specific to mermaid by targeting .quarto-figure-* > figure svg.mermaid-js.

Selecting against svg.mermaid-js is probably not a good idea, because mermaid cells can also choose to render themselves to PNG files through mermaid-format.

cderv commented 9 months ago

Selecting against svg.mermaid-js is probably not a good idea, because mermaid cells can also choose to render themselves to PNG files through mermaid-format.

But in that case, this is nothing mermaid specific right ? It is like other image and this is covered by the first part of the rule targetting p element I believe. Isn't it ?

.quarto-figure-center>figure>p, .quarto-figure-center>figure>div {
    text-align: left;
}
cderv commented 9 months ago

And no... because there is an enclosing div 🤦‍♂️ and the CSS rules targets figure > p

Too bad there is no class on this enclosing div that tells us this is mermaid

HTML code ````html

Figure 1: A mermaid diagram in its natural habitat.
````
gadenbuie commented 9 months ago

@cderv I'm pretty sure my suggested rules will also cover that markup as well. Happy to submit as a PR if that's helpful

cderv commented 9 months ago

Yeah I am ok with that.

@cscheid what do you think of @gadenbuie rules at https://github.com/quarto-dev/quarto-cli/issues/7843#issuecomment-1850262683 ? They do not involve mermaid or svg

cscheid commented 9 months ago

I'm honestly terrified of CSS changes because of how hard they're to test against.

I'd much rather make a small conservative change that targets leaflet specifically for 1.4, and then consider a better fix with ample time to revert if needed in 1.5.

cscheid commented 9 months ago

I'm going to push this to 1.5, and at that point we can carefully consider the change from text-align to margin-left, etc.