Excessive vertical space before definition/theorem/etc environments in latex output.

fulem commented 1 year ago

Bug description

There is excessive vertical space before definition/theorem/etc environments in latex output.

Reproduce:

# Chapter

## Section

### Subsection

#### Subsubsection

Some text before the definition.

::: {#def-def}
Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Ut purus elit,
vestibulum ut, placerat ac, adipiscing vitae, felis. Curabitur dictum gravida 
mauris.
:::

::: {#exm-exm}
Nam dui ligula, fringilla a, euismod sodales, sollicitudin vel, wisi. Morbi
auctor lorem non justo. Nam lacus libero, pretium at, lobortis vitae, ultricies et, tellus.
:::

Some text before the theorem.

::: {#thm-thm}
Nulla malesuada porttitor diam. Donec felis erat, congue non, volutpat
at, tincidunt tristique, libero. Vivamus viverra fermentum felis.
:::
::: {.proof}
Quisque ullamcorper placerat ipsum. Cras nibh. Morbi vel justo vitae lacus
tincidunt ultrices. Lorem ipsum dolor sit amet, consectetuer adipiscing elit. 
In hac habitasse platea dictumst.
:::

Some text afterwards.

Quarto configuration is the standard when creating a new quarto project in RStudio. So, the latex class is set as documentclass: scrreprt. However, the issue is noticeable in other classes.

Pdf render where the amount of inter space can be appreciated:

Compare with the hand written latex version using same class:

Examining the generated tex file i see every definition (and similar) environment is prepended with some extra code. For example, for the definition we have:

Some text before the definition.

\leavevmode\vadjust pre{\hypertarget{def-def}{}}%
\begin{definition}[]\label{def-def}

Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Ut purus elit,
vestibulum ut, placerat ac, adipiscing vitae, felis. Curabitur dictum
gravida mauris.

\end{definition}

I don't know what is the purpose of \leavevmode\vadjust pre{\hypertarget{def-def}{}} but it is adding unwanted vertical space. In fact, removing it solves the issue.

But there is another thing not related to aforementioned extra code. Note the height where the proof QED symbol (the white box) is placed. In the quarto version is below normal position. From my tests, i see that an empty line between the end of the proof text and the end proof tag makes a difference. That is, quarto generates the proof as follows:

\begin{proof}

Quisque ullamcorper placerat ipsum. Cras nibh. Morbi vel justo vitae
lacus tincidunt ultrices. Lorem ipsum dolor sit amet, consectetuer
adipiscing elit. In hac habitasse platea dictumst.

\end{proof}

But if we just remove the final empty line to have:

\begin{proof}

Quisque ullamcorper placerat ipsum. Cras nibh. Morbi vel justo vitae
lacus tincidunt ultrices. Lorem ipsum dolor sit amet, consectetuer
adipiscing elit. In hac habitasse platea dictumst.
\end{proof}

then the QED symbol goes up where we expect it to be.

More info:

Quarto 1.2.280
R studio 2022.12.0 Build 353
Windows 11 Pro 22H2

Checklist

[X] Please include a minimal, fully reproducible example in a single .qmd file? Please provide the whole file rather than the snippet you believe is causing the issue.
[X] Please format your issue so it is easier for us to read the bug report.
[X] Please document the RStudio IDE version you're running (if applicable), by providing the value displayed in the "About RStudio" main menu dialog?
[X] Please document the operating system you're running. If on Linux, please provide the specific distribution.

cscheid commented 1 year ago

A quick grep of quarto's source code for leavevmode yields this:

$ rg leavevmode -l
tests/docs/templates/acm/acm_proc_article-sp.cls
tests/docs/templates/elsevier/elsarticle.cls
tests/docs/extensions/format/academic/_extensions/quarto-journals/elsevier/elsarticle.cls
tests/docs/extensions/format/academic/_extensions/quarto-journals/acm/acm_proc_article-sp.cls
tests/docs/extensions/project/_extensions/quarto-journals/springer/sn-jnl.cls

So we're not inserting leavevmode ourselves. Searching for leavevmode pandoc on google yields this:

https://github.com/jgm/pandoc/issues/2704

This doesn't appear to be a bug on our side.

fulem commented 1 year ago

Comparing with bookdown, the following labeled definition environment

::: {.definition #mydef}
foo
:::

generates

\begin{definition}
\protect\hypertarget{def:mydef}{}\label{def:mydef}foo
\end{definition}

which is different to quarto, and it doesn't add any additional space before the environment.

Both bookdown and quarto uses pandoc, so i guess their are just invoking pandoc with different arguments. A possible solution would be to know if there is some specific argument that causes pandoc to behave different, and instruct quarto to do the same.

What do you think?

fulem commented 1 year ago

For what it's worth, bookdown uses a lua filter to deal with custom environments.

cderv commented 1 year ago

Here is some context about this \leavevmode insertion.

TL;DR

When processing those block in Quarto we keep a Div with an id and only modify the content to add the raw latex needed
This will let Pandoc writer handle the id part by inserting \hypertarget and add the \leavevmode part causing the issue then in LaTeX
Removing the id on the Div could be the solution

Details

This is added by Pandoc writer following the fix of the issue Carlos pointed to ( https://github.com/jgm/pandoc/commit/46135ac8758c4fbb9dfe676b1462463f5476bbff). It was added with one purpose at the time. See comment

The fix I've committed adds the \leavevmode in one particular case only: when you have a Div with an id (which will trigger a hypertarget) and the first block in the Div is a paragraph. That at least handles the pandoc-citeproc case well, without causing any problems I've yet noticed...

One user did warn about the vertical spacing that could happen though. (https://github.com/jgm/pandoc/issues/2704#issuecomment-283644668)

The \vadjust pre is from https://github.com/jgm/pandoc/issues/7078 after a discussion about whether this \leavemode is still needed.

It seems it was only aimed at Beamer for citation links - we are not in this case. So maybe this is an argument to ask for change ?

Anyway, we could maybe fix on our side based on the why above and difference with bookdown.

In bookdown, you don't have the issue because we are by passing the Pandoc writer on the insertion the hypertarget. We are inserting ourself the raw LaTeX in the custom block created. https://github.com/rstudio/bookdown/blob/dfe92a227b1dce5b8f0a2f758fb545e7df1b7e62/inst/rmarkdown/lua/custom-environment.lua#L137 So no \leavevmode introduced by Pandoc because of a Div with an id - you get the result shared just above. I think we did that to keep the \hypertargetwith the \label

Removing the id on the Div in theorem.lua would probably fix the issue. I don't know if the \hypertarget part is really needed here. 🤔 We could add inside the content, as in bookdown or not;

@cscheid @dragonstyle if we remove the id on the div, could this break crossref or is it read already and process by the internal value ? https://github.com/quarto-dev/quarto-cli/blob/f7fce46f348aeadb7fa902e3ca88568a27158f45/src/resources/filters/crossref/theorems.lua#L47-L50

I'll try a PR and we can adapt

cderv commented 1 year ago

So I have done a PR which fix this. It does not include \hypertarget{} - do we need to add ? Does one of you know the usage in LaTeX / PDF and why Pandoc adds it for all the links ? It seems to me \label is enough to reference \theorems

In doubt I will add it back

dragonstyle commented 1 year ago

I think Pandoc adds it as a target for cross ref links that reference the theorem (e.g. try adding See @thm-thm.)

cderv commented 1 year ago

The \label in LaTeX seems enough from the test. 🤔 I'll re-check

dragonstyle commented 1 year ago

Ok that might actually be correct - if it is working then I can't explain the hypertarget!

cderv commented 1 year ago

Ok I think I know. This is to write using Links syntax

See [Theorem](#thm-thm)

@thm-thm will create the LaTeX \ref syntax, but Markdown link syntax will add a

\protect\hyperlink{thm-thm}{Theorem}

so you need the \hypertarget{} counterpart for the link to work. I'll add it.

cderv commented 1 year ago

This should now be fixed in next prerelease version.

sun123zxy commented 1 year ago

Sorry for the disturbance, but the QED symbol problem can still be reproduced in Quarto 1.3.340. As mentioned above, it is caused by the empty line inserted between the final line and \end{proof}.

Will it be fixed?

cderv commented 1 year ago

@sun123zxy maybe something has been missed. As this issue is 6 month old and close, can you open a new one with reproducible example of what is the actual behavior and what is expected ?

Having this in a new issue will insure that it won't be missed. Thank you !

quarto-dev / quarto-cli