Skip to content

Suffix the ids in the cloned body TOC - #14853

Open
cwickham wants to merge 6 commits into
mainfrom
fix/14844-double-toc-duplicate-ids
Open

Suffix the ids in the cloned body TOC#14853
cwickham wants to merge 6 commits into
mainfrom
fix/14844-double-toc-duplicate-ids

Conversation

@cwickham

@cwickham cwickham commented Sep 4, 2026

Copy link
Copy Markdown
Member

Closes #14844.

Description

With toc-location: left-body or right-body, the useDoubleToc branch clones
the whole table-of-contents nav and renames only the nav itself, from TOC
to TOC-body. Every id inside the clone was copied as is. A page therefore
carried two elements for the toc-title heading, and two for each
toc-<section> link. The number of duplicates grows with the size of the TOC.

Ids must be unique in the tree (HTML Standard, the id attribute).
The duplicates were inert until #14376 labelled the TOC landmark with
aria-labelledby="toc-title". That reference makes one of them load-bearing.

This PR suffixes every id the clone carries with -body, and repoints the
clone's aria-labelledby at the renamed heading:

<nav id="TOC" role="doc-toc" aria-labelledby="toc-title" class="toc-active">
  <h2 id="toc-title">Contents</h2>
  ...<a href="#section-1" id="toc-section-1">Section 1</a>

<nav id="TOC-body" role="doc-toc" aria-labelledby="toc-title-body">
  <h2 id="toc-title-body">Contents</h2>
  ...<a href="#section-1" id="toc-section-1-body">Section 1</a>

Why every id, and not only the heading

The heading is the one id that ARIA references, so a minimal fix could rename
only that. I widened it because nothing outside the nav reads the link ids.
They come from Pandoc's TOC writer, and a search across src/ for "toc-",
#toc-$, and getElementById("toc finds no consumer in Quarto's JS, SCSS, or
Lua filters.

The originals stay on the sidebar copy, which is first in tree order. So
document.getElementById("toc-section-1") returned the sidebar element before
this change and returns the same element after it. Third-party CSS or JS that
targets these ids sees no difference.

The accessible name does not change

Both landmarks keep the name from the TOC title. That shared name is the correct
outcome here, and it is what the ARIA Authoring Practices Guide asks for
(Landmark Regions, Step 3):

If a navigation landmark has an identical set of links as another
navigation landmark on the page, use the same label for each navigation
landmark.

axe-core's landmark-unique still reports these two nav elements. That is a
best-practice rule, not WCAG, and this is the case the APG carves out. Do not
"fix" it by giving the two TOCs different labels — that would make the page
worse for screen reader users. It belongs in an axe baseline.

Measured effect

axe-core 4.10.3, the version Quarto vendors, over a rendered
toc-location: left-body page:

rendered with duplicate ids duplicate-id-aria landmark-unique
main 4 inapplicable violation (moderate)
#14813 4 incomplete, critical violation (moderate)
this PR 0 passes violation (moderate)

duplicate-id-aria now passes rather than being inapplicable, because the
reference exists and resolves to one element.

Testing

Extended the two existing fixtures for this layout,
tests/docs/smoke-all/issues/3473-toc-side-body/{left-body,right-body}.qmd,
with positive assertions for the suffixed ids and negative assertions that the
clone no longer carries toc-title or toc-section-1.

Both pass. With the change to format-html-bootstrap.ts reverted,
left-body.qmd fails, so the assertions catch the fault they describe.

Also ran locally, all passing:

Checked by hand:

  • Both landmarks still report the name "Contents" in Chromium's accessibility
    tree, for nav#TOC and nav#TOC-body.
  • right-body behaves the same as left-body.
  • toc-title: "" produces no heading and no aria-labelledby. The guard holds,
    the link ids are still suffixed, and no id repeats.
  • toc-title: false fails YAML validation on the base branch too, so no TOC
    renders without a heading through metadata.

I did not run the full suite locally. CI covers it.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes
  • updated the appropriate changelog in the PR
  • ensured the present test suite passes
  • added new tests
  • created a separate documentation PR in Quarto's website repo and linked it to this PR

The ids are internal, and no documented option changes, so there is nothing to
document on quarto.org.

AI-assisted PR
  • AI tool used: Claude Code
  • Codebase grounding: local clone
  • Human review: I have reviewed, tested, and verified the AI-generated content before submitting.

#14376)

New language keys navigation-{main,section,secondary,page,breadcrumbs}-label
label the navbar, sidebar (adaptive), secondary nav, prev/next page nav, and
breadcrumbs; the TOC nav is labelled by its localized heading via
aria-labelledby (html + revealjs). Defaults omit the word "navigation" per the
APG landmark-regions practice, since screen readers announce the role after
the label.

Note: all-schema-definitions.json also picks up the lagged cache-globals
schema from #14735 (regenerated artifacts trail schema changes by one build).
With `toc-location: left-body` or `right-body`, `useDoubleToc` clones the
whole TOC `nav` and renamed only the `nav` itself, from `TOC` to `TOC-body`.
Every id inside the clone was copied as is, so the page carried two elements
for the `toc-title` heading and for each `toc-<section>` link.

Ids must be unique in the tree. The duplicates were inert until #14376
labelled the TOC landmark with `aria-labelledby="toc-title"`, which turned
one of them into an ARIA reference: axe-core's `duplicate-id-aria` went from
inapplicable to needs-review (critical) on these pages.

Suffix every id the clone carries with `-body`, and repoint its
`aria-labelledby` at the renamed heading. Nothing outside the nav references
these ids, and the originals stay on the sidebar copy, so `getElementById`
resolves exactly as it did before.

Both landmarks keep the accessible name from the TOC title. That shared name
is correct here: the APG asks for the same label on two navigation landmarks
that hold an identical set of links, so axe's `landmark-unique` best-practice
result stays and should not be fixed by renaming the labels.
@posit-snyk-bot

posit-snyk-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@cwickham
cwickham marked this pull request as ready for review September 8, 2026 15:35
@cwickham
cwickham requested a review from cderv September 8, 2026 15:35
Base automatically changed from feature/nav-landmark-labels to main September 9, 2026 10:28
…c-duplicate-ids-2

# Conflicts:
#	src/config/constants.ts
#	src/config/types.ts
#	src/project/types/website/website-navigation.ts
#	src/resources/editor/tools/vs-code.mjs
#	src/resources/editor/tools/yaml/all-schema-definitions.json
#	src/resources/editor/tools/yaml/web-worker.js
#	src/resources/editor/tools/yaml/yaml-intelligence-resources.json
#	src/resources/language/_language.yml
#	src/resources/projects/website/templates/nav-before-body.ejs
#	src/resources/projects/website/templates/sidebar.ejs
#	src/resources/schema/definitions.yml
#	src/resources/schema/json-schemas.json
#	src/resources/types/schema-types.ts
#	src/resources/types/zod/schema-types.ts
#	tests/docs/smoke-all/website/nav-landmark-labels/_quarto.yml
#	tests/docs/smoke-all/website/nav-landmark-labels/index.qmd
#	tests/docs/smoke-all/website/nav-landmark-labels/no-navbar.qmd
#	tests/docs/smoke-all/website/nav-landmark-labels/override.qmd

@cderv cderv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id-suffix approach doesn't check for collisions against the rest of the document. Left a note on the specific line. Tell me if you think this is too much of a edge case.

I also fixed conflict by merging main, and fixing the artifacts

Comment on lines +359 to +371
// Ids must be unique, so the clone can't keep the originals: suffix
// every id it carries, and repoint its aria-labelledby at the renamed
// heading. Nothing outside the nav references these ids, and the
// originals stay on `toc`, so existing lookups resolve as before.
const clonedLabelId = clonedToc.getAttribute("aria-labelledby");
const clonedIdEls = clonedToc.querySelectorAll("[id]");
for (let i = 0; i < clonedIdEls.length; i++) {
const clonedIdEl = clonedIdEls[i] as Element;
clonedIdEl.id = `${clonedIdEl.id}-body`;
}
if (clonedLabelId) {
clonedToc.setAttribute("aria-labelledby", `${clonedLabelId}-body`);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suffix isn't guaranteed unique against the rest of the document. Pandoc dedupes identical heading slugs, but it doesn't check whether a slug already matches another heading's slug plus -body.

# Setup produces toc-setup. # Setup Body produces toc-setup-body, already sitting in the sidebar TOC. Suffixing the clone's toc-setup gives toc-setup-body too, colliding with the second heading's own entry, and we're back to the duplicate-id-aria failure this PR closes.

I don't know if this case can happen a lot or if this is really edge case ? What do you think ?

If this is possibly, I think this needs a real uniqueness check against the existing ids rather than a fixed suffix, plus a fixture with a heading pair that triggers this (something like Setup / Setup Body) so it doesn't come back silently.

Comment on lines 373 to 376
const tocActionsEl = clonedToc.querySelector(".toc-actions");
if (tocActionsEl) {
tocActionsEl.remove();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code above runs the id-suffix loop over the whole cloned nav, including the .toc-actions subtree, before it's removed here. This is not wrong, but just wasted work if that subtree carries ids — we could just remove .toc-actions first and so do the toc id processing after. What do you think ?

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.

toc-location: left-body clones the TOC without renaming its ids

3 participants