Skip to content

Toc accessibility improvements#2937

Closed
ascholerChemeketa wants to merge 3 commits into
PreTeXtBook:masterfrom
ascholerChemeketa:toc-accessibility
Closed

Toc accessibility improvements#2937
ascholerChemeketa wants to merge 3 commits into
PreTeXtBook:masterfrom
ascholerChemeketa:toc-accessibility

Conversation

@ascholerChemeketa

Copy link
Copy Markdown
Contributor

Work related to #2794

This will add lots of ptx-toc-group-* ids to the HTML output for which there is no way to check against authored ids using the existing reserved id mechanism.

@rbeezer

rbeezer commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Claude has things to say, of course, coming next. Things I noticed:

  • xsl:value-of for @unique-id. We should have a "getter" template for that already. This, and similar, are invaluable for consistency and refactors.
  • I thought we could check @xml:id and @label for the prefix of the multiple new ids, using substring(). Unlikely to be necessary, I know. I think there has been no reply to your post on ptx- strings? So we will move to a prefix check anyway? Start now and shorten soon? (Do a new PR for broader coverage, don't do it here now. Keep this focused on ToC.) Claude got his ears up, too. ;-)

@rbeezer

rbeezer commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Good direction — this closes some real gaps (localized landmark label on the ToC nav, aria-controls/aria-expanded on the expanders, Escape-to-close, and moving focus on expand). A couple of necessary fixes before merge, both in the ARIA state itself:

  1. aria-expanded on the sidebar toggle ends up inverted. In toggletoc(), after sideBarIsHidden = !sideBarIsHidden the variable now means "is now hidden," but it's assigned straight to aria-expanded — so opening the sidebar sets aria-expanded="false" (while it's visible) and closing sets "true". The focus branch right after it is correct, which hides the problem in manual testing. setAttribute("aria-expanded", !sideBarIsHidden) (or setting it inside the show/hide branches) fixes it.

  2. aria-role="complementary" won't take effect — the attribute is role, not aria-role, so the landmark is never applied. Worth a second look once corrected, since the inner <nav> is already a navigation landmark.

  3. Focus-into-ToC on open is a no-op. document.querySelector("#ptx-toc").focus() targets the <nav>, which isn't focusable without tabindex="-1", so focus doesn't move (the close path works because the button is focusable). Adding tabindex="-1" to #ptx-toc makes the open path land.

Smaller things:

  • The toggle button is emitted with a hardcoded aria-expanded="true", but on narrow viewports the sidebar starts hidden — consider initializing that from actual visibility on load.
  • getTOCItemType() looks dead now (both call sites were replaced); and in toggleTOCItem, targetElement is computed but unused, and groupName is read twice across the if/else.
  • The expanders mix .ariaExpanded/.ariaLabel (IDL reflection) with setAttribute('aria-controls', …) next to it — setAttribute throughout is the safer choice for shipped JS and reads more consistently.
  • The new #ptx-toc-toggle button could take type="button", matching the generated expanders and the search buttons.

On the reserved-id question you raised in the description: rather than enumerating the generated group ids, reserving the ptx-toc-group- prefix (or ptx- generally) in the id-checking mechanism would cover them in one stroke.

Claude Opus 4.8, acting as a review assistant for Rob Beezer

@ascholerChemeketa

Copy link
Copy Markdown
Contributor Author

@rbeezer - Addressed issues and force pushed.

@ascholerChemeketa

Copy link
Copy Markdown
Contributor Author

Once the current queue of accessibility stuff is clear I can do a ptx-* cleanup on the reserved ids

@rbeezer

rbeezer commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Ready to merge. Some nits from Claude next. maybe at some point you will want to roll-up everything that is not urgent/critical into an omnibus PR?

@rbeezer

rbeezer commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

The force-push resolves everything from the last pass — the aria-expanded inversion, role="complementary" (was aria-role), tabindex="-1" on #ptx-toc, init of aria-expanded from actual visibility, removal of getTOCItemType(), consistent setAttribute for the ARIA state, type="button", and the mode="unique-id" getter for both the new group id and uid. Reads well.

One thing that I think doesn't fire as intended:

  • The "focus the first child when opened by keyboard" block guards on event instanceof KeyboardEvent:

    if (expanded && expander === document.activeElement && event && event instanceof KeyboardEvent) {

    The expander's only listener is a click handler, and activating a <button> with Enter/Space dispatches a synthesized click — a PointerEvent/MouseEvent with detail === 0, not a KeyboardEvent. So this branch never runs and the keyboard focus-advance is dead. Testing event.detail === 0 (or pointerType === "") is the usual way to catch keyboard activation of a click.

A few smaller, non-blocking notes:

  • Escape-to-close is nested inside the --auto-collapse-toc == "yes" block, so it only works on auto-collapse themes. On other themes the ToC is still a toggle overlay on narrow viewports, where Escape would be a natural dismiss — worth considering whether it should be wired whenever the overlay is toggleable.
  • targetElement in toggleTOCItem is computed but unused; its source expander.controlledGroup has no other reader (the aria-controls is set directly from subList.id), so both can go.
  • groupName is computed identically in each branch of the if/else — it can be hoisted above the conditional.

Claude Opus 4.8, acting as a review assistant for Rob Beezer

@rbeezer

rbeezer commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Merged as-is after a rebase. Messed up the first merge (no js/dist update, wrong PR number), so had to do a force push tot he main repo. Not the first time.

Thanks for all the HTML/JS/CSS work!

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.

2 participants