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

add title canonicalization html postprocessor #11224

Merged
merged 1 commit into from
Oct 28, 2024
Merged

add title canonicalization html postprocessor #11224

merged 1 commit into from
Oct 28, 2024

Conversation

cscheid
Copy link
Collaborator

@cscheid cscheid commented Oct 28, 2024

Closes #10567.

This fixes the immediate bug, but the HTML emitted is still not exactly the same; we have a stray section tag on the document with an explicit # title heading (or, we have a missing section tag on the document with a title: title declaration).

@cscheid cscheid merged commit d1b4059 into main Oct 28, 2024
47 checks passed
@cscheid cscheid deleted the bugfix/10567 branch October 28, 2024 21:33
@jayhesselberth
Copy link

jayhesselberth commented Dec 2, 2024

@cscheid This change broke our (somewhat fragile) post-processing of quarto vignettes in pkgdown. Your comment above suggests that the structure of quarto HTML output may change again. Do you anticipate further modifying the structure of the HTML output in an upcoming quarto release? If so, we'll hold off making changes on the pkgdown side until the format is settled in the next release.

There are two specific things that impacted pkgdown post-processing:

  1. The title element disappeared from the document meta block in the HTML output. Not sure if this was intended. subtitle etc are still there, if specified. We had been using this to fetch the title.
  2. Moving the title into <main><header>...</header></main> added an extra title (h1) into our post-processed version.

@cderv
Copy link
Collaborator

cderv commented Dec 2, 2024

The title element disappeared from the document meta block in the HTML output. Not sure if this was intended. subtitle etc are still there, if specified. We had been using this to fetch the title.

This seems like an issue. HTML template as the field and it should be field with pagetitle which by default is supposed to be the same as title

<title>$pagetitle$$if(title-prefix)$ – $title-prefix$$endif$</title>

I'll look into that, based on the pkgdown bug report.

I am surprised that this change that seems to be targeted document without a title element. And the quarto vignette has this element. 🤔

@cderv
Copy link
Collaborator

cderv commented Dec 2, 2024

Oh I see. I misunderstood. Thanks for the clarification. 👀

@cderv
Copy link
Collaborator

cderv commented Dec 2, 2024

Ok I did not understood this meta block because this is something that comes from the pkgdown side. The quarto rendering is done with a custom template that create this block
https://github.com/r-lib/pkgdown/blob/dfa28bef27858bd442334b5cce8f645d555914be/inst/quarto/template.html#L25-L48

<div class="meta">
$if(title)$
  <h1>$title$</h1>
$endif$

$if(subtitle)$
  <p class="subtitle">$subtitle$</p>
$endif$

$for(author)$
  <p class="author">$author$</p>
$endfor$

$if(date)$
  <p class="date">$date$</p>
$endif$

$if(abstract)$
  <div class="abstract">
  $abstract$
  </div>
$endif$

</div>

In the output by the specific call to quarto render made by pkgdown (with some non-default meta), it ends up empty. This block should be filled with the $title$` pandoc variable, and it is not. This is the main issue. I'll try to make a minimal reprex and open a new issue.

@cderv
Copy link
Collaborator

cderv commented Dec 2, 2024

Ok so now I understand.

Within pkgdown, the custom template is applying which leads to this in the HTML before Quarto postprocessing

<div class="meta">
  <h1>My cool title</h1>
</div>

However, as the custom template does not use title-block.html partial, it will trigger the post processing from this PR.

if (!titleBlock && main) {

and a title block will be created anyway by this new post-processing. The h1 from the custom template is caught

const h1s = Array.from(doc.querySelectorAll("h1"));

and the h1 that pkgdown expect will be moved
header.appendChild(h1);

So this PR has indeed bad side effect on custom pkgdown and custom template. I'll open a separate issue with those informations.

@jayhesselberth
Copy link

@cderv your comment above suggests that we can just use the title-block.html partial. Is that one way around this issue?

@cderv
Copy link
Collaborator

cderv commented Dec 6, 2024

Are you looking for workaround until we fix this or a lasting solution for pkgdown side ?

@jayhesselberth
Copy link

CRAN wants an updated pkgdown by Dec 24 to address this issue so a workaround would be fine for now.

@cscheid
Copy link
Collaborator Author

cscheid commented Dec 6, 2024

@cderv @jayhesselberth Thanks for tracking this down. Let's make sure we fix this very soon if possible (eg, the next couple of days). Let's also add a regression test that explicitly exercises the pkgdown vignette path.

@jayhesselberth We aren't planning on major changes, but are likely to change the HTML structure over time. Nevertheless, the expectation is that we'll always document when we do. This specific issue interacts with the custom templates code paths in a subtle way, which is why it didn't come up during testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

breadcrumbs only appear if title set in topmatter
3 participants