Protect revealjs processing by warning about any internal id used in the document

gl-eb commented 2 months ago

Bug description

When a div with #title-slide is present in the document (used to horizontally center text https://github.com/quarto-dev/quarto-cli/issues/1231) and hash-type is set to number, the title on the title slide is top-aligned instead of vertically centered. Removing hash-type: number reverts the expected behaviour.

Steps to reproduce

---
title: "What a Great Title"
format:
  revealjs:
    hash-type: number
    center-title-slide: true
---

## This Slide Is Perfect

:::{#title-slide}
Profound statement
:::

Expected behavior

The title on the title slide should be vertically centered

Actual behavior

The title on the title slide is top-aligned

Your environment

OS: macOS Sonoma 14.6.1
IDE: Positron
- Version: 2024.09.0 (Universal) build 1
- Code - OSS Version: 1.92.0
- Commit: f37f4f5044a2a619e73d5db61a31e37fbd3faf18
- Date: 2024-09-03T02:37:20.474Z
- Electron: 30.1.2
- Chromium: 124.0.6367.243
- Node.js: 20.14.0
- V8: 12.4.254.20-electron.0
- OS: Darwin arm64 23.6.0

Quarto check output

Quarto 1.5.55
[✓] 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.55
      Path: /Applications/quarto/bin

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

[✓] Checking LaTeX....................OK
      Using: TinyTex
      Path: /Users/gleb/Library/TinyTeX/bin/universal-darwin
      Version: 2024

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

[✓] Checking Python 3 installation....OK
      Version: 3.12.5
      Path: /Users/gleb/.pyenv/versions/3.12.5/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: /opt/homebrew/Cellar/r/4.4.1/lib/R
      LibPaths:
        - /Users/gleb/Library/R/arm64/4.4/library
        - /opt/homebrew/lib/R/4.4/site-library
        - /opt/homebrew/Cellar/r/4.4.1/lib/R/library
      knitr: 1.48
      rmarkdown: 2.28

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

cderv commented 2 months ago

When a div with #title-slide is present in the document (used to horizontally center text https://github.com/quarto-dev/quarto-cli/issues/1231)

About this, I don't think this is the right solution to center horizontally a slide. First, this is using an id which is supposed to be unique on a HTML page. id = "title-slide" is already used... by the title slide of the presentation. Then, using this internal id for something else than the title slide is not recommended.

Why using this id ? You don't need it. It "works" because the CSS rules that apply to title slide with this id will then apply to this div. But you don't want all the style to apply. Just use same CSS rules

Either inline

:::{style="text-align: center;"}
Profound statement
:::

Using using another class and defining the CSS rule in a CSS or SCSS theme file.

:::{.center-h}
Profound statement
:::

.reveal section div.center-h {
    text-align: center;
}

hash-type is set to number, the title on the title slide is top-aligned instead of vertically centered. Removing hash-type: number reverts the expected behaviour.

This is related to

https://github.com/quarto-dev/quarto-cli/issues/4418

fixed by

https://github.com/quarto-dev/quarto-cli/pull/4708

When hash-type: number we are not expecting any element to have #title-slide. We are removing it ourself https://github.com/quarto-dev/quarto-cli/blob/cf0b7d06e67acb6180c822cc77e89cb936bb3c00/src/format/reveal/format-reveal.ts#L386-L395

As I explained doc.getElementById("title-slide"); is supposed to matched only one id. But adding another one in the document makes it not removed, and then the center-title-slide processing is not applying correctly https://github.com/quarto-dev/quarto-cli/blob/cf0b7d06e67acb6180c822cc77e89cb936bb3c00/src/format/reveal/format-reveal.ts#L585-L588

Our processing is looking for #title-slide id. And it is finding the additional div element and not the title slide, so the title slide centering applies to the div added and not the expected title slide.

Here, the problem really is to not use #title-slide as an id. By applying it on another element, obviously this inherits all behavior supposed to apply to this element.

Even if we make it better for hash-type: number to avoid this problem, it could lead to other undesired side effect as an id is supposed to be unique.

used to horizontally center text #1231

This discussion contains other solutions. Using #title-slide is not a good one. You should probably try others

Hope it helps understand the situation.

gl-eb commented 2 months ago

About this, I don't think this is the right solution to center horizontally a slide. First, this is using an id which is supposed to be unique on a HTML page. id = "title-slide" is already used... by the title slide of the presentation. Then, using this internal id for something else than the title slide is not recommended.

I should have known about id's having to be unique. I vaguely remember finding the issue I linked a while ago and the id = "title-slide" method being the only one that worked for whatever reason. The issue only came up yesterday when I noticed the change in title slide formatting.

I've switched to using text-align: center now. I also appreciate you explaining the mechanics behind this, thanks!

quarto-dev / quarto-cli