Core 2259 epub page roles - #467
Conversation
There was a problem hiding this comment.
Pull request overview
Adds structural semantics extracted from the EPUB ToC into generated page XHTML to improve screen-reader navigation and heading structure.
Changes:
- Extend ToC tree nodes to capture
data-toc-type/data-toc-target-typefrom ToC<li>elements. - Derive ARIA landmark roles (and labels) for structural pages (unit/chapter/preface/appendix/index) based on ToC metadata.
- Normalize page heading levels and optionally inject an “ancestor” title heading for chapter/unit intro pages; add targeted test coverage and snapshot updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| bakery-js/src/model/base-toc.tsx | Captures ToC metadata attributes into the generic ToC tree model. |
| bakery-js/src/epub/toc.tsx | Maps ToC metadata to ARIA roles and sets role/label info on the relevant PageFiles. |
| bakery-js/src/epub/toc.test.ts | Adds tests for ToC metadata capture, role assignment, and ancestor title behavior. |
| bakery-js/src/epub/page.tsx | Applies heading normalization, injects ancestor title, and writes role/epub:type/aria-label onto page content. |
| bakery-js/src/epub/page.test.ts | Adds tests for heading normalization, ancestor title insertion, and structural role attributes. |
| bakery-js/src/epub/snapshots/toc.test.ts.snap | Updates snapshots for new ToC attributes and ToC output changes. |
| bakery-js/src/epub/snapshots/page.test.ts.snap | Updates snapshots for heading normalization and new PageFile fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dantemss
left a comment
Dantemss
left a comment
There was a problem hiding this comment.
Seems fine, I just had one question
| stack.pop() | ||
| } | ||
|
|
||
| const parent = stack[stack.length - 1] |
There was a problem hiding this comment.
I was confused for a sec about the stack with the while loop but I see it's just a way to track potential parent headings, and not actually doing recursion.
| // A chapter/unit start (set below, via an ancestor INNER node) is a | ||
| // structural landmark and takes priority over this leaf's own | ||
| // tocTargetType — e.g. an "intro" page IS a chapter's first page, so | ||
| // it should stay `doc-chapter` rather than being reassigned here. |
There was a problem hiding this comment.
Took me way too long to figure out this comment is explaining the toc.page.ariaSpec === null check
There was a problem hiding this comment.
Yeah, I think I will rewrite this one 😂
| if (role !== undefined) { | ||
| const firstPage = this.findFirstPage(toc) | ||
| firstPage.ariaSpec = { role, label: toc.title } | ||
| firstPage.ancestorTitle = { title: toc.title, pos: toc.titlePos } |
There was a problem hiding this comment.
Is this still fine if there are no chapter intro pages etc?
https://openstax.atlassian.net/browse/CORE-2259