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.

Sign up for GitHub

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

Jump to bottom

New title canonicalization for h1 is breaking custom rendering (using custom template and no boostrap) #11596

Closed
cderv opened this issue Dec 2, 2024 · 0 comments · Fixed by #11639
Closed

New title canonicalization for h1 is breaking custom rendering (using custom template and no boostrap) #11596

cderv opened this issue Dec 2, 2024 · 0 comments · Fixed by #11639
Assignees
@cscheid @cderv
Labels
bug Something isn't working html Issues with HTML and related web technology (html/css/scss) needs-discussion Issues that require a team-wide discussion before proceeding further regression Functionality that used to work but now is broken.

Comments

@cderv
Copy link
Collaborator

cderv commented Dec 2, 2024

New title canonicalization from

is a bit to eager for custom rendering lik pkgdown is doing. This has been found while investigating

This example recreates what pkgdown is doing

  • Take this custom template template.html

    <!DOCTYPE html>
<html lang="$lang$">
<body>

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

<main>
$body$
</main>

</body>
</html>

    It does not use any of quarto partial and fully custom. pkgdown use something like this by taking the $title$ to put it in a specific div it can retrieve from

  • Now use it a simple .qmd document like this

    ---
title: TESTING
format: 
  html:
    template: template.html
    minimal: true
    theme: none
---

# Heading

The output will be the following after quarto render

<!DOCTYPE html>
<html lang="en"><head></head><body>

<div class="meta">
  
</div>

<main><header id="title-block-header" class="quarto-title-block"><h1 class="title">TESTING</h1></header>
<h1 id="heading">Heading</h1>
</main>



</body></html>

Previous with Quarto 1.5

<!DOCTYPE html>
<html lang="en"><head></head><body>

<div class="meta">
  <h1>TESTING</h1>
</div>

<main>
<h1 id="heading">Heading</h1>
</main>



</body></html>

New addition from #11224 is applying here because all the checks are triggered

quarto-cli/src/format/html/format-html-title.ts

Lines 195 to 241 in 39baf40

// https://github.com/quarto-dev/quarto-cli/issues/10567
// this fix cannot happen in `processDocumentTitle` because
// that's too late in the postprocessing order
const titleBlock = doc.querySelector("header.quarto-title-block");
const main = doc.querySelector("main");
// if no main element exists, this is likely a revealjs presentation
// which will generally have a title slide instead of a title block
// so we don't need to do anything
if (!titleBlock && main) {
const header = doc.createElement("header");
header.id = "title-block-header";
header.classList.add("quarto-title-block");
main.insertBefore(header, main.firstChild);
const h1s = Array.from(doc.querySelectorAll("h1"));
for (const h1n of h1s) {
const h1 = h1n as Element;
if (h1.classList.contains("quarto-secondary-nav-title")) {
continue;
}
// Now we need to check whether this is a plausible title element.
if (h1.parentElement?.tagName === "SECTION") {
// If the parent element is a section, then we need to check if there's
// any content before the section. If there is, then this is not a title
if (
h1.parentElement?.parentElement?.firstElementChild !==
h1.parentElement
) {
continue;
}
} else {
// If the parent element is not a section, then we need to check if there's
// any content before the h1. If there is, then this is not a title
if (h1.parentElement?.firstElementChild !== h1) {
continue;
}
}
const div = doc.createElement("div");
div.classList.add("quarto-title-banner");
h1.classList.add("title");
header.appendChild(h1);
break;
}
}

#11224 was supposed to solve #10567 which seemed to be quite specific to breadcrumbs. So maybe the post processing applies to eagerly here.

  • Should it apply on Project type website rendering only ?
  • Should it apply only when no custom template as been passed ?

I see that this processing checks that .quarto-title-block exists and add it otherwise. I believe class is not always added in HTML document.
It is from this specific partial (and others) added only for bootstrap output

quarto-cli/src/format/html/format-html-title.ts

Line 19 in 39baf40

RenderedFormat,

I mean when minimal: true or theme: pandoc or theme: none, I don't think there will be this class however the processing will add it.

There are many variation of title block also and this check

quarto-cli/src/format/html/format-html-title.ts

Line 198 in 39baf40

const titleBlock = doc.querySelector("header.quarto-title-block");

@cderv cderv added html Issues with HTML and related web technology (html/css/scss) regression Functionality that used to work but now is broken. labels Dec 2, 2024
@mcanouil mcanouil added the bug Something isn't working label Dec 2, 2024
@cscheid cscheid added the needs-discussion Issues that require a team-wide discussion before proceeding further label Dec 2, 2024
@jhelvy jhelvy mentioned this issue Dec 5, 2024
Closed
@cderv cderv mentioned this issue Dec 9, 2024
Merged
cderv added a commit that referenced this issue Dec 9, 2024
@cderv
move fix to #10567 into website postprocessor
f658382 
Fix regression that appeared for non website #11596

Companion commit in 1.6 branch for #11596 in main
@cderv cderv mentioned this issue Dec 9, 2024
Merged
@cderv cderv mentioned this issue Dec 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cscheid cscheid

@cderv cderv

Labels
bug Something isn't working html Issues with HTML and related web technology (html/css/scss) needs-discussion Issues that require a team-wide discussion before proceeding further regression Functionality that used to work but now is broken.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

move fix to #10567 into website postprocessor quarto-dev/quarto-cli
3 participants
@cscheid @cderv @mcanouil