fix(epub-codec): stop four XHTML container readers from silently dropping content - #996
Open
Mearman wants to merge 42 commits into
Open
fix(epub-codec): stop four XHTML container readers from silently dropping content#996Mearman wants to merge 42 commits into
Mearman wants to merge 42 commits into
Conversation
Mearman
force-pushed
the
fix/epub-codec-silent-content-drops
branch
3 times, most recently
from
September 5, 2026 17:15
69ae872 to
7160761
Compare
Mearman
force-pushed
the
fix/epub-codec-silent-content-drops
branch
from
September 5, 2026 18:38
b8799c5 to
54ecc51
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Mearman
marked this pull request as ready for review
September 5, 2026 19:48
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Mearman
force-pushed
the
fix/epub-codec-silent-content-drops
branch
from
September 5, 2026 20:27
60ca8b5 to
8291a5d
Compare
…ping content Four container-reading functions in the XHTML mapping only recognised a fixed set of child shapes and dropped anything else outright, with no diagnostic and no fallback -- a stricter failure than the honest degrade-with-diagnostic policy the rest of this package follows. readTable iterated element.children and only descended into tr/thead/tbody/ tfoot, so a <caption> -- a legal direct child of <table> per the HTML5 content model -- fell through to an empty array and vanished along with everything inside it. It is now read as an ordinary paragraph immediately before the table, exactly like a <figcaption>'s own treatment (its own direct-child <img> degrades to alt text for the identical reason), with a new epub/table-caption-unsupported diagnostic naming the one real loss: document-schema.js's ContentTable has no field of its own for a caption's distinct tag, so a same-format round trip cannot tell it apart from any other paragraph before the table. readDefinitionList only recognised a dt/dd that was a direct child of the <dl>, so a <dl> wrapping one or more dt/dd pairs in a <div> -- legal HTML5, used by producers that want a per-entry styling hook -- lost the whole wrapped group. Recursing into a <div> child finds the pairs it wraps exactly as if they sat directly in the <dl>. No diagnostic: the wrapper carries no property document-schema.js's vocabulary can express, identical to every other <div> this package already reads transparently. readList only recognised an <li> child, so a <ul>/<ol> nested directly as a sibling rather than wrapped in its own <li> -- not valid HTML5, but a shape real producers and converters emit -- lost the entire nested list, and any other stray content in that position (a bare <img>, stray text) vanished the same way. Stray content following a <li> is now fed through the identical readContainerChildren dispatch that <li>'s real children already use, under that item's own list membership: a stray list becomes a properly nested list one level deeper sharing the enclosing numId, a stray image becomes its own real block, and stray text becomes its own paragraph, all via one mechanism rather than a hand-rolled special case, with a new epub/list-content-outside-item diagnostic. Content before the very first <li> has no preceding item to attach to and is still dropped, unchanged from prior behaviour. readPre extracted a <pre>'s content with textContent(), which recurses into element children collecting only text and so silently discards any <img> found anywhere inside, at any depth. A <pre> block's own content model is plain text, so an embedded image genuinely cannot become a real ContentImageBlock the way one reached through readContainerChildren can -- there is no block list to insert one into, the same structural constraint inline.ts's own image fallback already applies. Its alt text is now spliced into the extracted text in its place, with a new epub/image-pre-unsupported diagnostic naming the loss. Confirmed by execution in #994.
Adds the issue's own reproduction cases for a <pre>/<code> block's <img> (spliced into the extracted text and one nested a level deeper inside <code>), a <table>'s own <caption> (text and image both), a <dl> wrapping one dt/dd pair in a <div> plus several such groups in sequence, and a <ul> nested directly as a sibling of <li> plus a stray <img> in the same position -- the case the fix commit's own diagnostic and text call out as the highest-impact of the four. A companion case pins that content before the very first <li> still drops, unchanged from prior behaviour. Also extends the diagnostics-coverage sweep so all three new EpubDiagnosticCodes entries (image-pre-unsupported, table-caption-unsupported, list-content-outside-item) count as reachable.
…osed The direct-child-<img> gotcha's own trailing clause named four container shapes as genuinely silent gaps tracked in #994: a <table>'s own <caption>, a <dl> wrapping dt/dd pairs in a <div>, a <ul>/<ol> nested directly as a sibling of <li>, and an <img> inside a <pre>/<code> block. Three are now fully recovered (caption as a preceding paragraph, div-wrapped dl groups by recursion, sibling list/stray content as a continuation of the preceding <li>); the fourth remains a genuine structural limit -- a <pre>'s content model is plain text, so an embedded image still cannot become a real ContentImageBlock -- but no longer vanishes silently either, since its alt text is spliced into the extracted text with a diagnostic.
…ray list content readList treated every non-<li> child of a <ul>/<ol> as malformed stray content, including plain whitespace text nodes between <li> siblings and <script>/<template> elements. The HTML Standard (3.2.5.1) requires inter-element whitespace to be ignored when checking an element's content model, and states the <ul>/<ol> content model as "zero or more li and script-supporting elements", making <script>/<template> legal siblings of <li>. The pretty-printed indentation nearly all real-world HTML uses was firing a spurious epub/list-content-outside-item diagnostic on almost every list, and a <script>/<template> element was routed through readContainerChildren (which has no case for either tag), leaking raw script source into content as a bogus paragraph under the preceding item's list membership. Both are now filtered out before stray-node collection: whitespace-only text nodes and script-supporting elements are skipped entirely, with no collection, no diagnostic, and no content emission.
readTable emitted a bogus {kind: "paragraph", runs: []} for an empty
<caption>, plus a spurious epub/table-caption-unsupported diagnostic,
contradicting this package's own documented rule (already enforced by
readContainerChildren's flush guard elsewhere) that an empty or
whitespace-only paragraph is dropped entirely on read.
The caption's runs are now checked before emitting anything: when they
are all whitespace, the caption carries nothing to lose, so it is
dropped with no diagnostic and only the table itself is returned.
appendElement's default case treated <script>/<template> as an ordinary inline container, recursing into it and emitting its raw content (JS source, or a <template>'s own inert subtree) as plain document text whenever one was reached while building a flat run sequence -- inside a <p>/<td>/<figcaption> directly, or nested arbitrarily deep inside any other element appendElement recurses through. Script-supporting elements are never legitimate document text per the HTML Standard's own content model, so they now short-circuit to a no-op in the same switch dispatch that already special-cases sub/sup and img, closing the leak universally rather than only at the one position read.ts's own isScriptSupportingElement check happened to guard.
readList dropped every whitespace-only text node sitting directly inside
a <ul>/<ol> before it could ever reach flushListStrayContent's own
stray-content collection, rather than only suppressing the diagnostic
that content would otherwise cause. The HTML Standard's "must be ignored
when establishing whether an element's contents match the content model"
rule (section 3.2.5 "Content models") governs conformance-checking alone,
not deletion of the character data itself, so a genuine inter-element
space between two stray inline siblings was being discarded outright,
silently joining adjacent words on read ("foo bar" becoming "foobar").
Whitespace text now flows into strayNodes exactly like any other stray
content, and flushListStrayContent's own guard skips the diagnostic (and
recovery, since there is nothing to recover) only when the *entire*
collected run of stray content, once actually read through a throwaway
sink, carries nothing but whitespace -- real content mixed with real
whitespace still gets the full diagnostic-and-recovery treatment, with
the whitespace preserved as part of it. The previous comment on this
rule also cited the wrong HTML Standard section (3.2.5.1, "The 'nothing'
content model") for the quoted sentence, which actually lives in 3.2.5.
…tion readTable built its recovered caption paragraph from only captionInline's runs, discarding captionInline.constructs entirely -- silently dropping any run-level construct extent inside a <caption> (most notably a footnote reference) with no diagnostic, since the field simply never made it onto the returned ContentParagraph. Splices constructs onto the paragraph the same way readContainerChildren's own flush already does, only when there is at least one to carry.
…rvation behaviour The gotchas bullet on #994's four content-drop fixes described the <caption> recovery unconditionally as "read as an ordinary paragraph immediately before the table", which stopped being the whole story once an empty or whitespace-only <caption> started dropping entirely instead. Splits the claim into both cases, and notes that a run-level construct the caption's own inline content carries now rides the recovered paragraph's own constructs field rather than being discarded.
…on is empty flushListStrayContent decided whether to recover stray content sitting directly inside a <ul>/<ol> by building the collected nodes' inline runs and checking whether the resulting text was blank -- but buildInlineRuns only ever produces TEXT runs, so any stray block-level construct whose text projection happens to be empty (a resolved image with no alt text, an <hr>, a table or nested list whose only content is such an image) was misjudged as whitespace-only and silently dropped, with no diagnostic, indistinguishable from real pretty-printed whitespace. Replace the speculative text-only probe with the real readContainerChildren result: recover and report whenever it yields any block at all, dropping only when it genuinely yields none (true inter-element whitespace). This also removes a redundant double-read of the same nodes -- the probe built inline runs through a throwaway sink purely to answer a question the real read already answers correctly.
readPreText walks a <pre>'s children on its own recursion rather than calling into src/xhtml/inline.ts's appendElement, so the script/template skip that function's own comment calls "universal" never actually reached a <pre>: both tags are legal children of <pre> per the HTML content model, and their content (raw JS source, an inert DOM subtree) still leaked into the extracted document text. Give readPreText the identical skip branch so a <script>/<template> sitting anywhere inside a <pre> is dropped rather than read as text, matching what already happens everywhere else in this module.
…irst <li> flushListStrayContent dropped any content collected before the first <li> in a <ul>/<ol> with no diagnostic at all, on the theory that it had no preceding item to attach to. That silently discarded real content: a <ul> nested as a sibling before any <li> (issue #994's own headline repro, `<ul><ul><li>b</li></ul><li>a</li></ul>`) lost the entire nested list, and plain stray text before the first item vanished the same way. Recover this content instead, through the same readContainerChildren dispatch already used for content between/after real <li> siblings, but without any list membership of its own -- it lands in the returned block sequence immediately before the list's own real items, matching what a browser renders for this malformed shape. The recovery-or-drop decision and the LIST_CONTENT_OUTSIDE_ITEM diagnostic now cover both cases uniformly, and the underlying read runs exactly once either way: its result is always either fully used or the empty list nothing was minted or reported for, never computed and then discarded.
…s empty readTable dropped a <caption> as empty purely by checking whether its built runs were all-whitespace, using Array.prototype.every over the runs array -- vacuously true when the array is empty. A caption whose only content is a footnote-reference anchor with no text of its own (`<caption><a epub:type="noteref" href="#fn1"></a></caption>`) produces zero text runs but one real RunConstructExtent, so the text-only guard discarded a construct that had nothing else in the caption to carry it. Only drop the caption when it carries neither text nor any construct.
…iblings readContainerChildren's own segment flush decided whether phrasing content sitting between two block-level siblings was empty by checking only whether its built text runs were all-whitespace -- the same Array.prototype.every-on-an-empty-array pattern that caption reading carried. A bare footnote-reference anchor with no text of its own (`<a epub:type="noteref" href="#fn1"></a>` sitting directly between two block siblings) produces zero text runs but one real RunConstructExtent, so the guard dropped the whole segment, construct included, treating it as indistinguishable from real inter-element whitespace. Only drop the segment when it carries neither text nor any construct.
…headings and ids containsHeading, buildIdElementMap, and readXhtmlBody's own footnote- anchor prescan each walk arbitrary XML descendants directly, bypassing buildInlineRuns's own appendElement dispatch, which is the only place that already treats <script>/<template> content as inert. As a result: - A heading sitting inside a <template> inside a <blockquote> incorrectly suppressed the blockquote's own division construct, even though the heading is never real, readable document content. - An id living only inside a <template> was indexed into the id->element map, letting a footnote reference resolve against a target that will never actually be read. - A footnote-reference anchor living only inside a <template> could seed footnoteTargetIds, causing an unrelated, genuinely live body element sharing that target id to be wrapped as a footnote body nothing real ever referenced. Add a shared isInertContainer(tag) guard and apply it in all three places, plus a local elementsWithTagSkippingInert for the footnote- anchor prescan (src/xml/query.ts's own elementsWithTag stays generic, since it is shared by other callers with no reason to assume this policy).
…lt directly from inline content
A table caption already kept a footnote-reference construct extent carried by its own inline
content, but a figcaption, a dt/dd, and a table cell each built their paragraph as
`{ kind: "paragraph", runs: inline.runs }` directly, discarding buildInlineRuns's own constructs
array outright -- the identical defect shape reproduced four more times rather than fixed once.
Extract the caption's own constructs-attachment logic into a shared constructsField helper and
funnel every such paragraph (heading, caption, table cell, dt, dd, figcaption, and
readContainerChildren's own segment flush) through it, so a construct collected while building
inline content is never dropped just because the paragraph carries no other property worth
spreading in.
…inition list readDefinitionListEntries silently dropped any <dl> child that was not dt/dd/div (a stray <p>, stray text, a stray <img>, or a one-tag variant like <section> wrapping dt/dd instead of <div>) with zero diagnostics -- the same defect class #994 was filed about, reachable here one tag away from the shape that issue's own fix already covers. Apply the identical treatment readList already uses for its own analogous whitelist: route any stray child through readContainerChildren and report a real diagnostic (epub/definition-list-content-outside-entry) naming the loss, rather than silently dropping it. <div> stays the one wrapper this narrower rule recognises without a diagnostic, since it is the one shape HTML5's own <dl> content model actually names as legal; a non-conformant wrapper's own dt/dd children lose their distinct term/definition treatment once routed through readContainerChildren, degrading to plain concatenated text -- a real fidelity cost, but a text-preserving one.
…ide them readTable's row loop only recognised tr/thead/tbody/tfoot/caption as valid table children, dropping anything else (stray prose, a stray div) with no diagnostic; its row loop only recognised td/th as valid row children, dropping stray text or elements sitting directly inside a <tr> the same silent way. findChildElement also only ever resolved the FIRST <caption>, discarding a second one outright. Apply the same general stray-content-recovery-with-diagnostic treatment already used elsewhere in this file to both whitelists: table-level stray content is recovered via readContainerChildren and reported through epub/table-content-unrecognized, positioned immediately before the table (a table's own rows fold into one indivisible ContentTable block, so there is no position WITHIN it to reinsert interleaved stray content into); row-level stray content is recovered as its own cell in the row's own column sequence via epub/table-row-content-outside-cell. A <table> carrying more than one <caption> now has every caption beyond the first read as its own paragraph too, via epub/table-duplicate-caption, rather than silently discarding it.
…redicate isInertContainer, isScriptSupportingElement, an inline tag-literal check inside readPreText, and a dedicated switch case in buildInlineRuns's own per-node dispatch (inline.ts) were four separate copies of the identical script/template check, so a future tag addition to one could silently miss the others. Move the check to context.ts as one shared isInertElement predicate, used everywhere: read.ts's buildIdElementMap, elementsWithTagSkippingInert, containsHeading, readPreText, readList, readDefinitionListEntries, and readTable, plus inline.ts's own appendElement. Pure refactor, no behavioural change -- the tag set stays script/template only.
…>/<template> <style> and <noscript> appearing in <body> fell through to the default inline-container handling and leaked their raw text as document prose -- readStyleResidue only quarantines <head>-level styles, and inline.ts's own appendElement only ever guarded against <script>/<template>. Extend the shared isInertElement predicate to also cover <style> (CSS, exactly like the <head>-level residue this package already quarantines rather than interprets) and <noscript> (scripting-disabled fallback markup this package cannot reliably distinguish from a "please enable JavaScript" placeholder). Both are now skipped everywhere the predicate is already consulted, including buildInlineRuns's own run-building recursion, so they can never leak into document text regardless of where they sit.
…very Content recovered before a list's very first <li> was documented and diagnosed as landing "with no list membership of its own" and rendering "exactly what a browser renders for this malformed shape" -- both false whenever the enclosing <ul>/<ol> is itself nested inside another list's <li>: the recovery reuses the plain `state`, so it inherits state.listItem when one is already set, and a browser indents such content at the outer item's own depth while this recovery always emits it as a shallower top-level sibling. Reword the doc comment and the diagnostic message to state the real behaviour -- order preserved, membership inherited from the enclosing context (none only when that context has none), nesting depth not necessarily preserved -- and add a regression test proving a nested case actually inherits the outer <li>'s own membership rather than carrying none. Also brings a handful of comments left saying "<script>/<template>" back in sync with isInertElement's now-wider <style>/<noscript> tag set.
…de any <tr> readTable's own thead/tbody/tfoot handling filtered a row group's children down to just <tr> elements, silently dropping anything else -- stray text, a stray paragraph, a stray image, an entire nested list, a stray caption -- with zero diagnostics, one level of nesting deeper than the table-direct and row-direct stray-content recovery this file already applies. A row group's own content model per the HTML Standard is "zero or more tr and script-supporting elements", the identical malformed-content shape the table-direct case already recovers, so collectRowGroupRows folds a row group's own non-tr, non-inert children into the same shared strayNodes accumulator readTable already recovers and reports through, rather than needing a second recovery path or diagnostic.
…tent recovery's own comment flushListStrayContent's own comment still said an inert element (script/template/style/noscript) "never reaches this function at all", flatly true only for one sitting as a direct child of the <ul>/<ol> itself -- readList's own loop filters those out before collection -- but false for one nested a level or more deeper inside a stray wrapper (e.g. a <script> inside a stray <div>), which does reach this function's own readContainerChildren call and resolves to zero blocks there instead. flushDefinitionListStrayContent's own comment already states this narrower, correct claim for the identical <dl> case; bring this comment in line with its sibling.
…nline content inside a <pre> readPre built its paragraph from readPreText's own flat-string walk and never called buildInlineRuns, so a run-level construct (a footnote reference, most commonly) sitting inside a <pre> was silently dropped with no diagnostic at all -- worse than the <img>-in-<pre> case, which at least degrades with a diagnostic naming the loss. containsFootnoteReference cheaply checks whether a <pre>'s own subtree carries a recognised footnote-reference anchor anywhere, at any depth, mirroring containsHeading's own inert-skipping descendant walk. Only when it does does readPre pay for readPreRuns, a run-splitting twin of buildInlineRuns that still preserves a <pre>'s own verbatim whitespace (never routing through buildInlineRuns' own normalizing walk) while bracketing a footnote reference's own text as its own run range a RunConstructExtent can point at, rebasing any nested call's own construct indices onto the outer run sequence at the point of the merge. The common case -- no footnote reference anywhere in the block -- still takes the original, unchanged single-run readPreText path.
…e-reference construct A heading paragraph has carried its own run-level construct via constructsField since that helper was extracted, but unlike every sibling site sharing it (a table caption, a table cell, a <dt>/<dd>, and a <figcaption>), it had no dedicated test proving a footnote reference survives on a heading built directly from inline content.
…im-whitespace paragraphs ContentParagraph gains an optional preformatted boolean naming a paragraph whose runs carry significant whitespace that must survive verbatim (HTML/EPUB's <pre>, ODF's Preformatted_Text style). Independent of codeLanguage, and independent of run count or content: a construct nested inside a preformatted block can split its content into further runs with no bearing on whether the block itself is preformatted, so a reader that already knows a paragraph came from a verbatim construct sets this directly rather than leaving a writer to infer it from run shape, which is never a reliable signal for the fact.
…d flag, not run count isPreBlockParagraph used to recognise a <pre>/<code> paragraph only via codeLanguage or a single monospace run embedding a newline, so a <pre> with no language class whose content includes a recognised construct (readPreRuns splits a footnote reference into its own run range) silently round-tripped as an ordinary <p>, losing the block's own verbatim whitespace. readPre now stamps every paragraph it produces with document-schema.js's own preformatted flag regardless of run count, and the writer checks that flag first; a recognised <pre> paragraph's runs and constructs write through the new writePreRunsToNodes (sharing writeRunsToNodes' extent-finding walk via the extracted writeRunRangeNodes, but never splitting an embedded newline into a <br/>, since <pre>'s content model has no <br> handling to read one back from).
… the XHTML reading path Every text-bearing walk in this package (buildInlineRuns, readContainerChildren's own phrasing/block split, readPre's three text-extraction paths, every stray-content collector in read.ts, and xml/query.ts's own textContent) dispatched on node.type === "text" alone, silently dropping a CDATA section -- the standard XML idiom a producer reaches for when its own literal text would otherwise need escaping (a code sample, or any content containing a raw </&). xml/node.ts's isTextLikeNode is now the one shared predicate every such walk dispatches on, and xml/entities.ts's decodeTextLikeNode is the one place that folds a CDATA node's raw, never-entity-encoded value in alongside a decoded text node's, so a caller combining the two never runs CDATA content back through decodeEntities. textContent itself now decodes internally rather than leaving its callers to decode the concatenated result, so its own CDATA descendants are never double-decoded; its four call sites in opf/metadata.ts drop their now-redundant decodeEntities wrap accordingly.
…y <col> A <table>'s own <colgroup> was skipped wholesale on the reasoning that its only legal content (<col>, and script-supporting elements) carries nothing document-schema.js's own vocabulary can represent -- true for conforming content, but any other content sitting directly inside a <colgroup> (a stray <p>, stray text, a stray <img>) is not valid HTML5 and was silently discarded with no diagnostic. collectColgroupStrayContent recovers it the same way collectRowGroupRows already does for a <thead>/<tbody>/<tfoot> one level up, feeding readTable's own shared strayNodes accumulator so it is recovered immediately before the table via the existing table-content-unrecognized diagnostic.
…list item writeList built a list item's own anchor content via writeRunsToNodes alone, never consulting the horizontal-rule/preformatted dispatch writeParagraph itself already used for every other paragraph. A <pre> nested directly inside an <li> (ordinary HTML: <li>'s content model is flow content) read back correctly as a preformatted paragraph but was then written out as an ordinary run sequence, destroying its verbatim whitespace; a <hr> nested the same way vanished from the written <li> entirely, since writeRunsToNodes on an empty-runs paragraph with no constructs produces zero XML nodes. writeHeading has the identical bypass shape, so it now routes through the same shared dispatch (writeParagraphAsEmbeddedNodes) for consistency, even though a heading's phrasing-only content model makes a <pre>/<hr> heading unreachable from a real read.
…ontract textContent is a published export re-exported from this package's own barrel. An earlier commit on this branch made it decode entities and include CDATA content internally, when it previously returned raw, undecoded text-node content only -- exactly the contract this package's own opf/metadata.ts relied on before that change, wrapping its result in decodeEntities itself. A stale external caller doing the identical decodeEntities(textContent(x)) would now silently double-decode (a literal "&amp;" round-tripping to "&" instead of the correct "&"). textContent is reverted to its original raw, text-node-only behaviour. The CDATA-aware, decode-internally behaviour lands instead as a distinctly named sibling, decodedTextContent, which opf/metadata.ts's four Dublin Core readers now call instead, dropping their own now-redundant decodeEntities wrap exactly as before.
…ph dispatcher h1-h6 permit only phrasing content per the HTML Standard, while <pre> and <hr> are flow content, so routing writeHeading through the same horizontal-rule/ preformatted/ordinary-runs dispatch writeList uses for a list item's anchor content let a heading styled entirely in a monospace font with an embedded newline -- tripping isPreBlockParagraph's own legacy heuristic for a foreign producer's <pre> -- write a non-conformant <pre> nested inside an <hN>, a shape real EPUB validators reject. "This reader can't produce that shape" was not a safe argument for the writer: the writer's job is round-tripping whatever a foreign producer's document actually contains, and a heading's content model already rules out <pre>/<hr> unconditionally regardless of provenance. writeHeading now writes its runs via writeRunsToNodes directly, matching every other call site's own reasoning for why a heading was never routed through the shared dispatcher in the first place.
…wn run on write writeRunRangeNodes' own while loop only iterated while there was at least one run left (index < runs.length), so a paragraph with zero runs -- a bare footnote-reference construct sitting alone between two block siblings, or a table caption containing nothing but one -- never reached its own construct extents at all. The reader already recovers both shapes correctly; the writer silently dropped the construct instead, producing an empty <p></p> and orphaning the footnote body it once pointed at. The loop now bounds itself with index <= runs.length so an extent sitting at the very end of the run sequence is still reached. Reachable through the same function: a point anchor (startRun === endRun) sitting strictly inside a paragraph's run sequence used to advance the walk past the run at that same index without ever rendering it, silently deleting that run's own text instead of merely failing to wrap it in the anchor. A point anchor now falls through to render the run at its own index after emitting the (empty) anchor element, rather than treating its empty range as consuming that index the way a real non-empty extent's endRun does. Adds write-then-reread round-trip tests for both the bare-empty-paragraph and empty-caption construct shapes, and for the point-anchor case, none of which the prior round's read-side fix alone exercised.
… not just its intent The field comment named HTML/EPUB's <pre> and ODF's Preformatted_Text style as the constructs this flag exists to carry, which reads as if both already set it. Only epub-codec's readPre does. markdown-codec's fenced/indented code block lowering and odf.js's Preformatted_Text paragraph reading still carry only codeLanguage/styleId, with no verbatim-whitespace signal of their own -- tracked in #1020 rather than left as a silent mismatch between this comment and the readers that actually exist.
…, not just the first writeRunRangeNodes located a footnote reference extent at each run index via Array.prototype.find, which resolves at most one match -- so when two extents shared the same startRun (two point-anchor references back-to-back, or two overlapping non-point ranges), only the first was ever emitted and the second silently vanished with no diagnostic. document-schema.js's own RunConstructExtentSchema comment states extents are "data, not brackets" precisely because more than one can legitimately share a boundary this way. Every extent at an index is now collected: every point anchor (startRun === endRun) is written as its own empty <a>, since wrapping zero runs never conflicts with a sibling point anchor at the same boundary. At most one non-point range extent can actually claim the runs from that index onward, so the first one in the input's own array order wins and any other range extent found at the same index is reported through the diagnostic sink (CONSTRUCT_UNREPRESENTED) as a genuine, unrepresentable overlap -- two sibling <a> elements can't both claim the identical starting run without nesting one inside the other, which HTML's interactive-content rule forbids. The underlying run text is never lost either way, since it is still written by whichever extent wins, or by the ordinary per-run path once the winner's own range ends.
…, not one <li> each decomposeSection opens a fresh ListGroupNode for every list paragraph at a given level, regardless of itemId, so a genuinely multi-block list item -- readList mints ONE itemId per real <li> and shares it across every block readContainerChildren produces from that <li>'s own children -- arrives at writeList as several separate, adjacent entries rather than one entry with several blocks. writeList wrote one <li> per entry unconditionally, so a <li> holding, say, a horizontal rule followed by a paragraph round-tripped back out as two sibling <li> elements instead of the single item the source document actually had. document-schema.js's own ContentListMembership.itemId field exists precisely to distinguish "one item, several blocks" (same itemId) from "several items sharing a numId/level" (different itemIds) -- a distinction the writer was silently not honouring. writeList now re-groups a run of consecutive entries sharing the same defined itemId into one <li>, writing each entry's own anchor content and nested children in turn inside it. An itemId of undefined never groups with anything, including another undefined neighbour, since absence means no item identity was carried at all. A related gap this does not close is pinned as a regression test rather than fixed here: when a footnote reference sits in a list item's last block with nothing plain in between before that footnote's own body, document-schema.js's decomposeSection attaches the footnote's construct group as a child of the still-open list item rather than at the section root, so its <aside> nests inside the <li> instead of sitting beside the list. That is a decomposeSection defect reachable from every codec built on it, tracked separately in #1022 rather than patched around here.
…al trigger
The gotchas bullet read as if epub/image-pre-unsupported only fires for an image with no
alt text ("naming the loss when it carries none"), matching the sibling inline-image
bullet's own genuinely conditional wording. readPreImageFallbackText actually calls the
sink unconditionally, before it even looks at the alt attribute -- the diagnostic fires
for every image reached inside a <pre>, alt text or not. Reworded to state that plainly.
…ording The CONSTRUCT_UNREPRESENTED message for a losing footnote extent claimed its run text is "written unwrapped", but the writer never drops that text: every run the losing extent covers is still emitted, wrapped inside the winning extent's own anchor wherever the two ranges overlap, and unwrapped only past the winner's own endRun. Reword the message to describe that actual behaviour instead of a drop that never happens.
…ing run range
writeRunRangeNodes's own diagnostic only fired for two extents sharing the
exact same startRun. A winning range extent advances the walk straight from
its own startRun to its own endRun, skipping every index in between, so a
second extent whose startRun falls strictly INSIDE that range -- a genuinely
crossing range, or a point anchor nested in the interior -- was never
compared against the walk's own index at all and vanished with no
diagnostic. This is the identical silent-drop class the same-startRun fix
closed, reached via a different input shape that document-schema.js's own
RunConstructExtentSchema names explicitly ("two entries may cross freely").
Replace the inline collision check with a Set tracking every extent the walk
actually emits, then sweep the full candidate set once the walk completes
and report CONSTRUCT_UNREPRESENTED for anything never emitted, whatever the
reason. The same run text still survives the loss either way: a range
extent's own runs are still written, wrapped by whichever extent's range
actually claims them or unwrapped past it; a point anchor wraps none of its
own, so only its own link marker goes missing.
…ly scope isFootnoteExtent silently excludes every other AnchorType a run-level RunConstructExtent can carry -- ooxml.js's own docx reader emits bookmark and comment extents at the identical run scope via pairRunRangeMarkers, and this writer has no representation or diagnostic for either one. Filed as #1025 rather than fixed here, since it is a distinct construct kind never handled at all, not a bug in this function's own footnote handling.
…n a comment
The comment citing ooxml.js's own bookmark/comment run-scope emitter named a
function ('pairRunRangeMarkers') that has never existed in this repository's
history. The real function it describes -- and the one already named
correctly twice elsewhere in ooxml.js's own source -- is
runRangeMarkerExtents (src/typed/docx/constructs.ts).
…g one absolute cause The comment and both CONSTRUCT_UNREPRESENTED messages claimed a point extent only ever goes unemitted by sitting inside another extent's own winning run range, and a range extent only ever goes unemitted by overlapping or colliding with another extent. Neither is the whole story: an extent whose own startRun sits beyond the paragraph's actual run count, or a range extent with endRun < startRun, falls through the identical code path with no other extent involved at all, so the old wording falsely blamed a specific conflicting extent that doesn't exist for either shape. Reword the comment and both messages to state the cause conditionally, covering the malformed-range case alongside the genuine collision/overlap case.
Mearman
force-pushed
the
fix/epub-codec-silent-content-drops
branch
from
September 5, 2026 20:55
8291a5d to
a17cb0c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #994
Started as four container-reading functions in
src/xhtml/read.tsthat only recognised a fixed set of child shapes and dropped anything else outright, with no diagnostic and no fallback -- stricter than the honest degrade-with-diagnostic policy the rest of the package follows. Chasing the same shape through the rest of the reader (and then the writer) turned it into a longer pass; this is what actually landed.Reading.
readPresplices an<img>'s alt text into a<pre>'s extracted text instead oftextContent()silently discarding it (epub/image-pre-unsupported-- the image itself still can't become a real block inside a plain-text content model).readTablereads a<caption>as an ordinary paragraph immediately before the table (epub/table-caption-unsupported), recovers content sitting outside any row/caption/<col>-- directly inside<table>, inside a<thead>/<tbody>/<tfoot>, or inside a<colgroup>-- the same way (epub/table-content-unrecognized), recovers a<tr>'s own content sitting outside any cell as its own cell (epub/table-row-content-outside-cell), and no longer drops every caption after the first when a table carries more than one (epub/table-duplicate-caption).readDefinitionListrecurses into a<div>wrappingdt/ddpairs (legal HTML5, no diagnostic needed since the wrapper carries nothing to lose) and recovers genuinely stray content -- a bogus wrapper, stray text, a stray<img>-- viaepub/definition-list-content-outside-entry.readListrecovers content sitting outside any<li>, both between/after real items (attaches to the preceding item's own nesting) and before the very first one (recovered at the list's own enclosing membership), viaepub/list-content-outside-item.Alongside those:
<style>/<noscript>are now skipped in body content like<script>/<template>already were, instead of leaking their own raw content into extracted text. A CDATA section reads exactly like an ordinary text node everywhere in the XHTML path (inline runs, stray-content collection,<pre>extraction, OPF metadata via the newdecodedTextContent), rather than being silently excluded. A run-level construct (most often a footnote reference) carried by inline content built directly into a heading, table caption, table cell,dt/dd, orfigcaptionis preserved on that paragraph's ownconstructsfield -- previously only an ordinary<p>kept it -- and a construct-only inline segment with no text of its own, sitting alone between two block siblings, is kept alive rather than dropped as if empty.Preformatted content.
document-schema.jsgainedContentParagraph.preformatted, a general "this paragraph's whitespace is significant" signal independent ofcodeLanguage.readPrestamps it on every paragraph it produces, and the writer now trusts that flag instead of inferring "this came from a<pre>" from run count or a single-monospace-run heuristic -- a footnote reference nested inside a<pre>splits it into more than one run with no bearing on whether it's preformatted, which the old heuristic silently misclassified.Writing.
writeListdispatches a list item's own anchor content through the same horizontal-rule/preformatted/ordinary-runs checkwriteParagraphuses, so a<pre>or<hr>nested directly inside an<li>keeps its own block shape instead of degrading to bare inline runs. A heading's own runs are written directly (writeRunsToNodes), never through that dispatcher:h1-h6permit only phrasing content, and both<pre>and<hr>are flow content, so nothing may write either one inside an<hN>regardless of what a foreign producer's input looks like. The writer's shared run-range walk now reaches a construct extent sitting at the very end of a run sequence -- a construct-only paragraph with zero runs, or a table caption containing nothing but a footnote reference -- instead of never entering its own loop body, and a point anchor (startRun === endRun) sitting strictly inside a run sequence preserves the run at that index instead of silently deleting its text.Testing
src/xhtml/read.test.tsandsrc/xhtml/write.test.tscover every shape above, including write-then-reread round trips for the construct-preservation and point-anchor fixes.src/diagnostics-coverage.test.tsextended so every new diagnostic code counts as reachable in the coverage sweep.pnpm exec turbo run _lint _typecheck _build _test _test:workers --filter=epub-codecall clean.