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

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

Closed
cderv opened this issue Mar 22, 2023 · 22 comments
Closed

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

cderv opened this issue Mar 22, 2023 · 22 comments
Assignees
Labels
bug Something isn't working callouts Issues with Callout Blocks.
Milestone

Comments

@cderv
Copy link
Collaborator

cderv commented Mar 22, 2023

---
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

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

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

@cderv cderv added the bug Something isn't working label Mar 22, 2023
@cscheid
Copy link
Collaborator

cscheid commented Mar 22, 2023

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

  • Changing CSS might break layouts of other sites
  • Changing the DOM might break other filters and postprocessors

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?

@cscheid cscheid added this to the v1.3 milestone Mar 22, 2023
@cderv
Copy link
Collaborator Author

cderv commented Mar 22, 2023

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
Copy link
Collaborator

cscheid commented Mar 22, 2023

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.

@cscheid cscheid modified the milestones: v1.3, v1.4 Mar 22, 2023
@cderv
Copy link
Collaborator Author

cderv commented Mar 22, 2023

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
Copy link
Collaborator

cscheid commented Mar 22, 2023

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

image

@cscheid
Copy link
Collaborator

cscheid commented Mar 22, 2023

Nope, broke on 1.2.148 too:

image

@cscheid cscheid added the needs-repro Issues that are blocked until reporter provides an adequate reproduction label Mar 22, 2023
@cderv
Copy link
Collaborator Author

cderv commented Mar 22, 2023

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
Copy link
Collaborator

cscheid commented Mar 22, 2023

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
Copy link
Contributor

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

@cderv
Copy link
Collaborator Author

cderv commented Mar 23, 2023

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
* we take the first header away as the callout title
* We wrap the body in a div with some class, something like
````
::: { .callout-body }
## Callout body header

	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
Copy link
Collaborator

cscheid commented Mar 23, 2023

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
Copy link
Collaborator Author

cderv commented Mar 23, 2023

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
Copy link
Collaborator Author

cderv commented Mar 23, 2023

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
Copy link
Collaborator Author

cderv commented Mar 23, 2023

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
Copy link
Collaborator

cscheid commented Mar 23, 2023

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
Copy link
Collaborator Author

cderv commented Mar 24, 2023

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
Copy link
Contributor

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

@allenmanning allenmanning closed this as not planned Won't fix, can't repro, duplicate, stale Mar 27, 2023
@cderv
Copy link
Collaborator Author

cderv commented Mar 27, 2023

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

Anyhow, as mentioned in #4922 (comment)

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
Copy link
Contributor

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

@allenmanning allenmanning reopened this Mar 27, 2023
@allenmanning allenmanning removed bug Something isn't working needs-repro Issues that are blocked until reporter provides an adequate reproduction labels Mar 27, 2023
@allenmanning allenmanning added the documentation Doc improvements & quarto-web label Mar 27, 2023
@cscheid cscheid changed the title Missing padding in callout body when it starts with a header Document limitations of Callout contents Dec 4, 2023
@cscheid cscheid added callouts Issues with Callout Blocks. and removed articles labels Dec 4, 2023
@cscheid
Copy link
Collaborator

cscheid commented Dec 13, 2023

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

@cscheid cscheid added the bug Something isn't working label Dec 13, 2023
@cscheid
Copy link
Collaborator

cscheid commented Dec 13, 2023

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
Copy link
Collaborator

cscheid commented Dec 13, 2023

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

image

I think this is a safe fix.

@cscheid cscheid changed the title Document limitations of Callout contents Callout body div can become a section and confuse our CSS Dec 13, 2023
@cscheid cscheid removed the documentation Doc improvements & quarto-web label Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working callouts Issues with Callout Blocks.
Projects
None yet
Development

No branches or pull requests

3 participants