quarto-dev / quarto-cli

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

Callout body div can become a `section` and confuse our CSS #4922

Closed cderv closed 8 months ago

cderv commented 1 year ago
---
title: Callout padding
format: html
---

::: {.callout-note}

# Title

## Another header

There is no padding here
:::

# Document Header

Content

::: {.callout-note}

# Title

There is correct padding here
:::

::: {.callout-note}

# Title

Something first

## Sub header

There is correct padding here
:::

In this example we have several cases. If you render that you'll notice that there is no padding applied on the callout that start with a header.

image

I believe this is because of section-divs: true because there will be not <div class="callout-body-container callout-body"> - it will be merged in the section header <section id="another-header" class="level2 callout-body-container callout-body">

The classes are added to the section directly so the CSS does not apply https://github.com/quarto-dev/quarto-cli/blob/2246187f1aa0c4bee2b68c677b8efa55a0f8c779/src/resources/formats/html/bootstrap/_bootstrap-rules.scss#L1298-L1301

Either we should just tweak the CSS to support content wrapped in section or tweak the body in our Lua filter.

I believe only changing the CSS rule is fine. I let you decide @cscheid

cscheid commented 1 year ago

I don't know which of the two changes is more prone to regression:

I think CSS is better because it means that an additional fix is also doable in CSS (in case users are affected by this). @jjallaire @dragonstyle what do you think?

cderv commented 1 year ago

Changing CSS might break layouts of other sites

I think the only change needed is also supporting section.callout-body in the selector in addition to div.callout-body

do you think this can break some layout unexpectedly ? I believe this is quite scoped to only target adding the padding for this node too.


By the way, I noticed in our scss file we don't use that much the power of SCSS syntax with nested elements etc... this would probably help us organized our rule more and make it easier to tweak style. Just a thought I had while looking at the scss code.

cscheid commented 1 year ago

Ok, so I'm trying to find where this regressed, and it's been broken since at least v1.1. I thought it was a regression, but it's not:

---
title: Callout padding
format: html
---

::: {.callout-note}

# Title

## Another header

There is no padding here

:::

This renders badly in 1.1.251:

image

I'm now convinced we should only fix this in 1.4.

cderv commented 1 year ago

We may not have urgency to fix. But this example https://examples.quarto.pub/create-tabsets-panel-from-r-code/ shows that it seems to have been working in 1.2.148 according to head content

<meta name="generator" content="quarto-1.2.148">

It is not important really, just a style issue that could be fixed using custom CSS, but annoying. It seem to have worked. So maybe there is something else.

Anyhow, I still thinks this is a small CSS tweak to account for the section-divs process. But this can wait 1.4

cscheid commented 1 year ago

So it was working on 1.2.148, but it was broken on 1.2.335:

image
cscheid commented 1 year ago

Nope, broke on 1.2.148 too:

image
cderv commented 1 year ago

what to you need to repro here ? the regression ? Maybe this is not a regression then (not sure how I ended up with this published doc like that) but this is still something to improve right ?

cscheid commented 1 year ago

I tagged it as need-repro because the file we have is not really a repro. Since it breaks on 1.2.148, we don't yet understand exactly what is going on.

allenmanning commented 1 year ago

Nice detective work. I'm curious to know if a test could have caught this regression.

cderv commented 1 year ago

the file we have is not really a repro. Since it breaks on 1.2.148, we don't yet understand exactly what is going on.

Sorry if that is not clear to you. The file is not that important. This is how I found about something odd, so I shared it. Then I spent time digging to the root cause, to see if it was include feature or not. When I found the problem, I opened this issue.

I shared a minimal example in my first post above that shows the difference when the content start with a header or not. I tried to explained what the problem is as I understand it. You can reproduce it by looking at the generated html.

Let me try to be more clear, with more example and details.

My understanding is that our CSS does not account for the fact that when there is a header starting the callout body content, the wrapping HTML node for the body of callout won't be a <div> with the class .callout-body but a <section>. This is what happens when you put a div before a header in Pandoc and I believe we do that in our Lua filter for callout.

So we do the equivalent of this for the callout body content

And this is why we get a <section> at the end

> pandoc -t html
::: { .callout-body }
## Callout body header

content
:::
^Z
<section id="callout-body-header" class="callout-body">
<h2>Callout body header</h2>
<p>content</p>
</section>

But if the content doesn't start with a header, we get a div as our CSS expect

> pandoc -t html
::: { .callout-body }

content before header

## Callout body header

Content after
:::
^Z
<div class="callout-body">
<p>content before header</p>
<h2 id="callout-body-header">Callout body header</h2>
<p>Content after</p>
</div>

This is what I tried to explained in my comments above, I hope this details make it clearer to you what I think the problem is.

Otherwise we can discuss live maybe.

cscheid commented 1 year ago

yes, let's talk live. (I was asking "is this a regression" because that's what controls whether we want to fix this for 1.3 or not.)

cderv commented 1 year ago

I was asking "is this a regression" because that's what controls whether we want to fix this for 1.3 or not.

Understood ! I was missing that. I'll look into this on my side then. For now I can't tell for sure this is regression

cderv commented 1 year ago

I'll look into this on my side then. For now I can't tell for sure this is regression

Running 1.2.148 recreates me the same doc, with the <div> above the section. So this is a regression at some point. I'll dig more quickly.

Though I correctly understand now that this is not really expected / permitted to use other header in a callout.

We should probably add to our doc this limitation: https://quarto.org/docs/authoring/callouts.html

cderv commented 1 year ago

OK I found why I get the difference I can see on https://examples.quarto.pub/create-tabsets-panel-from-r-code/ rendered with 1.2.148 compared to now.

This is a bit tricky and this would be the equivalent smaller reprex for the examples HTML published online

---
title: Callout padding
format: html
---

:::: {.callout-note collapse="true"}

::: {.content-visible when-format="pdf"}
See this on PDF only
:::

## Another header

There is no padding here

::::

So the "regression" is how the special callout and conditional div are handled within each other. It seems in 1.2.148, using such fenced div was equivalent of adding text above the internal header. Now it is not.

Anyhow, now we know. Probably nothing to change here then. Except documenting the content limitation of callout, and not doing what I am doing in our examples published - it was quite useful to include my explanation document this way in an example doc like that. Aim was to separate actual demo content from explanation.

cscheid commented 1 year ago

It seems in 1.2.148, using such fenced div was equivalent of adding text above the internal header. Now it is not.

Wait, it should be, though. We had a bug where we kept the div for the conditional content, and now we are supposed to just inline the content itself into the parent structure. Is that not happening here? Because if so, that's the bug.

cderv commented 1 year ago

I think we're good here. The conditional chunk in my example above is correctly supposed to be non included. It seems the processing has changed since then. I don't think we have a bug - the content is correctly inline into the parent structure when conditional match the format.

allenmanning commented 1 year ago

Based on the last comment I will close as Won't Fix. Please re-open if I got this wrong.

cderv commented 1 year ago

Yes we probably won't fix this if I understood correctly that we have this limitation on purpose.

Anyhow, as mentioned in https://github.com/quarto-dev/quarto-cli/issues/4922#issuecomment-1481658034

Probably nothing to change here then. Except documenting the content limitation of callout,

Shouldn't we document better that our callout feature is intended to be used with simple text content and that using {{< include ... >}} as I did, or even using content with header is not supported ? (due to TOC and navigation challenge IIUC)

allenmanning commented 1 year ago

I see, it seems we should re-open as a doc enhancement.

cscheid commented 8 months ago

Ok I'm revisiting this and there's totally a bug we should fix, in addition to the doc fixes.

cscheid commented 8 months ago

I think we should fix the CSS.

The issue is that any div that starts with a header gets converted to a section element. When that div is a callout body, then the callout body gets converted to a section. Some parts of our CSS add padding to div.callout-body, instead of just .callout-body, which is what other parts do. Take _bootstrap-rules.scss, around line 1416:

.callout.callout-style-default div.callout-body {
  padding-left: 0.5em;
  padding-right: 0.5em;
}

We add padding only to div.callout-body

cscheid commented 8 months ago

When we add padding to any elements with that class, it's better:

image

I think this is a safe fix.