feat(footnote): rendering fidelity (SD-2656)#3220
Conversation
Make the body paginator demand-aware so footnote-heavy documents pack
body content tight to the separator instead of letting the post-hoc
reserve loop leave visible blank space above the footnote band.
Measured on Harvey NVCA Model SPA (108 footnote refs):
- BEFORE: 57 pages
- AFTER: 53 pages
- Word baseline: 51 pages (within +5%)
Mechanism
---------
PageState gains two fields:
- pageFootnoteReserve : existing per-page reserve, now exposed
to the break decision
- footnoteDemandThisPage : accumulator of measured footnote body
heights for refs anchored on this page
Paragraph layout consults a new optional callback:
- getFootnoteDemandForBlockId(blockId): number
The break decision uses an effective bottom:
additionalDemand = max(0, footnoteDemandThisPage - pageFootnoteReserve)
effectiveBottom = state.contentBottom - additionalDemand
Once the convergence loop has set a correct reserve, additionalDemand is
0 and the new code is a no-op. On pass 1 (no reserve), it provides the
tight-packing signal that prevents the body from filling the page only
to be clawed back by a later reserve relayout.
A safety cap clamps additionalDemand so the page always has room for at
least one body line - otherwise an oversized footnote would drive
effectiveBottom below cursorY and the paginator would advanceColumn
indefinitely.
The per-block demand lookup is built once per layoutDocument call. It
walks the block tree, including table cells (rows[].cells[].blocks /
.paragraph), and resolves each ref's pos to the containing top-level
block. Table-cell refs are attributed to the table block, the unit the
body paginator places on a page.
layout-bridge populates bodyHeightById from measures via
refreshBodyHeights and pre-measures every footnote on every convergence
iteration so migrating refs do not drop from the lookup mid-loop.
Tests
-----
- footnoteBodyDemand.test.ts RED-then-GREEN for block-aware break
+ no-op invariant for non-footnote docs
- footnoteContinuationDemand converged layout reserves carry-forward
demand on the continuation page
- footnoteRefMigration determinism regression: repeated runs
produce identical page counts, reserves,
and ref to page assignments
Refs: SD-2656 SD-3049 SD-3050 SD-3051
Plan: docs/plans/sd-2656-footnote-rendering-fidelity.md
Report: docs/plans/sd-2656-implementation-report.md
…986 SD-2658) Inline footnote references and the leading marker inside the footnote body now honor the OOXML number format / start configured in w:settings/w:footnotePr. Custom-mark refs (customMarkFollows="1") emit an empty marker run so the literal symbol in the next OOXML run renders as the visible mark. Supported formats: decimal, upperRoman, lowerRoman, upperLetter, lowerLetter, numberInDash. Unknown formats fall back to decimal. Single source of truth between the inline ref and the leading marker: pm-adapter/src/footnote-formatting.ts -> formatFootnoteCardinal() Used by: pm-adapter/.../converters/inline-converters/footnote-reference.ts super-editor/.../layout/FootnotesBuilder.ts The formatter switch is intentionally inlined (not imported from @superdoc/layout-engine's formatPageNumber) because pm-adapter sits upstream of layout-engine in the package graph - see Guard C in layout-engine/tests/src/architecture-boundaries.test.ts. A drift detection parity test asserts the two helpers agree on every supported format for cardinals 1..100: layout-engine/tests/src/footnote-formatter-parity.test.ts Settings readers in super-editor/document-api-adapters/document-settings: readFootnoteNumberFormat(settingsRoot): string | null readEndnoteNumberFormat(settingsRoot): string | null readFootnoteNumberStart(settingsRoot): number | null readEndnoteNumberStart(settingsRoot): number | null PresentationEditor reads all four up-front and threads the values through ConverterContext.footnoteNumberFormat / .endnoteNumberFormat and the per-doc cardinal counter is seeded with the configured start. customMarkFollows handling preserves pmStart/pmEnd on the empty marker run so click and selection continue to work at the ref position. Refs: SD-2656 SD-2986 SD-2986/B1 SD-2986/B2 SD-2658 SD-2662
End-to-end documentation for the footnote rendering fidelity epic:
docs/superdoc-feature-reports/sd-2656-plan.md
Original implementation plan: ticket inventory across the epic,
OOXML grounding (§17.11), code surface map with line numbers,
surgical approach for each slice, RED test scaffolds, falsifiable
success criteria.
docs/superdoc-feature-reports/sd-2656-implementation-report.md
What shipped, with measurements:
- Harvey NVCA: 57 -> 53 pages (Word baseline 51, +5%)
- pnpm test:layout vs superdoc@1.32.0:
535/543 docs (98.5%) byte-identical
5 unique-change docs, all NVCA-style footnote-rich legal
templates (the intended scope)
- pnpm test:visual: "no visual differences found"
- 16,649 unit tests across 5 packages, all green
Slice-by-slice walkthrough (SD-3049 / 3050 / 3051 / 2986/B1+B2 /
2658 / 2662), architecture compliance (Guard C parity test),
pr-reviewer findings + resolutions, deferred work, repro commands.
Refs: SD-2656
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… numFmt, cache key) - Re-charge block footnote demand after each advanceColumn so a paragraph that spills mid-iteration leaves the new page with the right effective bottom — previously the recharge only fired at iteration top, and a block that finished its content on the spilled-onto page never charged its demand there, letting later blocks fill into the footnote band. - Wire endnoteNumberFormat through endnoteReferenceToBlock and EndnotesBuilder via the shared formatFootnoteCardinal so documents with w:endnotePr/w:numFmt render the configured format on both the inline ref and the leading marker. - Fold numberStart and numberFormat into the FlowBlockCache invalidation signatures so settings.xml mutations that change numbering format or starting cardinal evict stale cached reference runs. - refreshBodyHeights mirrors computeFootnoteLayoutPlan: read measure.height for image and drawing footnote content so the SD-3049 tight-pack signal fires for non-text footnotes. Tests: - layout-paragraph.test.ts: demand survives advanceColumn within one iteration - endnote-reference.test.ts: numFmt cases (upperRoman, lowerRoman, fallbacks) - footnoteBodyDemand.test.ts: tight gap for image-only footnotes Refs: SD-2656
- refreshBodyHeights now handles list-kind measures (per-item paragraph line heights + spacingAfter), mirroring buildFootnoteRanges. Without it list-only footnotes contributed zero demand to the SD-3049 tight-pack signal and re-introduced the blank body-to-separator gap. - FootnotesBuilder captures customMarkFollows on the inline ref and skips the leading marker injection in the footnote body for those ids. Matches the exporter contract: custom-mark footnotes have no w:footnoteRef in note content; the literal symbol in the document body is the entire identification. Tests: - footnoteBodyDemand.test.ts: tight gap for a list-only footnote - FootnotesBuilder.test.ts: customMarkFollows ref does not inject a marker run
The footnote band already renders each id once per page via assignFootnotesToColumns. Block-aware body demand must match: when the same id is referenced multiple times on a page, contribute its body height once. Previously refByPos kept every occurrence, so two refs to the same footnote on a page reserved 2× the real height and the body paginator left phantom whitespace above the separator at convergence. The dedup keeps the first ref position per id (sufficient for the walker, which only needs to attribute demand to *some* containing block). Test: 25 body paragraphs, footnote referenced twice — page 1 must pack tight with no extra whitespace.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64f2059ef4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The block-aware break re-charged blockFootnoteDemand on every page transition. For a long paragraph that spans pages with a footnote ref on the first one, continuation pages got the demand subtracted from their effective body region even though no footnote band renders there — packing 13–15 lines per page instead of 20 and producing unnecessary extra pages. Lock the charge after the first fragment commits. The spill case (Fix 1, paragraph's first fragment lands after advanceColumn) still works because re-charging still happens until the first commit; once the fragment is on the page, the lock prevents continuation pages from seeing phantom demand. Test: 50-line paragraph with a single ref on a 20-line-per-page layout converges to 3 pages (was 4 with per-page recharge).
§17.11.1 w:continuationSeparator — "spans THE WIDTH of the main story's text extents" §17.11.23 w:separator — "spans PART OF the width text extents" The current code had the two cases inverted: standard separator drawn at full column, continuation drawn at 30% column. Word renders the opposite. Test: footnoteSeparatorWidth.test.ts asserts standard ≈ 0.5 × contentWidth and continuation ≈ contentWidth on a fixture that forces footnote spill across pages.
…2657) §17.11.14 footnoteReference: "shall not increment the numbering for its associated footnote/endnote numbering format, so that the use of a footnote with a custom footnote mark does not cause a missing value in the footnote/endnote values." The previous numbering walk in PresentationEditor incremented the counter for every unique footnoteReference id, including those carrying customMarkFollows. A document with mixed auto + customMark refs and numFmt=upperRoman would render as I, II, III instead of the spec-mandated I, [custom], II. Extracted the numbering loop to layout/computeNoteNumbering.ts so the behavior is directly testable (and shared between footnote + endnote walks in PresentationEditor). The shared isCustomMarkFollows helper now lives here too — FootnotesBuilder and EndnotesBuilder will reuse it. Tests: - computeNoteNumbering.test.ts (23 cases) — first-appearance numbering, dedup, custom-mark suppression, OOXML on/off parsing.
…ootnote) §17.11.14 customMarkFollows applies to both w:footnoteReference and w:endnoteReference (both extend CT_FtnEdnRef). FootnotesBuilder already skips the synthetic body marker for custom-mark refs; EndnotesBuilder now mirrors it. Reuses the shared isCustomMarkFollows helper extracted in the previous commit (layout/computeNoteNumbering.ts). Removes the local duplicate from FootnotesBuilder. Tests: - EndnotesBuilder.test.ts (4 new cases) — body marker present for normal refs, suppressed when customMarkFollows is truthy, preserved when "0" / "false".
…t (SD-2986)
§17.11.11 — section-level w:footnotePr overrides document-wide numFmt /
numStart / numRestart. (pos is parsed but ignored per §17.11.21.)
§17.11.19 — numRestart=eachSect resets the counter at section boundaries.
Plumbing:
- document-settings.ts:
- readFootnoteNumberRestart / readEndnoteNumberRestart (ST_RestartNumber)
- readSectionNoteConfigs(docPart, w:footnotePr|w:endnotePr) →
Map<sectionIndex, SectionNoteConfig{ numFmt?, numStart?, numRestart? }>
- computeNoteNumbering takes a NumberingOptions struct with sectionConfigs +
defaultRestart + defaultNumFmt. Walks sectionBreak nodes in the PM doc to
track the current section index; resets the counter at section boundaries
when numRestart=eachSect; emits formatById{} keyed by ref id when any
section overrides numFmt.
- ConverterContext: new footnoteFormatById / endnoteFormatById (per-ref
resolved numFmt). Document-wide footnoteNumberFormat remains the fallback.
- inline-converters/footnote-reference + endnote-reference: per-id format
wins over document-wide.
- FootnotesBuilder + EndnotesBuilder: leading-marker formatting honors the
per-id format.
- PresentationEditor: reads document-wide + section-level configs; folds
them into the flow-block cache signature so stale markers invalidate.
Tests:
- document-settings.test.ts: 9 new cases — readers + reader normalization,
§17.11.21 pos-ignored case, endnote variant.
- computeNoteNumbering.test.ts: 28 cases total — first-appearance numbering,
customMark suppression, eachSect counter reset (default + per-section
override), per-section numFmt → formatById, backwards-compat (no overrides
→ formatById absent).
§17.11.19 — eachPage restarts numbering at each page boundary. Page assignment is layout-dependent, so the helper takes an optional refPageById map populated by a post-layout pass. When present AND the active restart is 'eachPage', the counter resets when the ref crosses a page boundary. When absent (first render or non-eachPage docs), the counter behaves as continuous — gracefully degrading rather than guessing. Cross-section transition into an eachPage section also triggers a reset to the next section's numStart (rather than carrying the prior section's continuous counter), and clears the page tracker so the new section starts cleanly. Tests: - Resets at page boundaries when refPageById is provided. - Falls back to continuous when refPageById is absent (first-pass shape). - Section-level eachPage overrides document-wide continuous. - per-section numStart provides the reset value. - Cross-section transition (continuous → eachPage) resets cleanly. Note: the post-layout pass that populates refPageById and re-runs the layout is intentionally deferred — none of the SD-2986 acceptance docs uses eachPage and the existing convergence loop already handles multi-pass without regression. Tracked as a follow-up.
…ithub.com:superdoc-dev/superdoc into tadeu/sd-2656-feature-footnote-rendering-fidelity
…ent (SD-2985)
§17.11.1 w:continuationSeparator
§17.11.23 w:separator
§17.18.33 ST_FtnEdn — typed footnote records
Annex L.1.12.5 — continuationNotice text
Foundation for rendering imported separator/continuationSeparator/
continuationNotice content faithfully when the document overrides Word's
default visual (rare in the SD-2985 acceptance corpus, but real for
documents that suppress the separator or specify a pBdr / text).
Two pieces:
1. Importer now preserves continuationNotice typed records (parallel to
separator and continuationSeparator). Empty paragraphs round-trip safely;
explicit content survives in originalXml for the downstream classifier.
2. classifyNoteSeparatorContent inspects the originalXml of a typed record
and returns one of:
- 'default-marker': paragraph contains only <w:r><w:separator/></w:r>
(or continuationSeparator marker). Renderer uses Word's default
visual — Spec A widths already match §17.11.1 / §17.11.23.
- 'suppression': paragraph is empty. Renderer emits nothing.
- 'explicit': paragraph has w:pBdr (with at least one border defined)
or text content. Consumer converts the XML to FlowBlocks via the
handler chain and emits those fragments instead of the default.
Tests:
- separatorContentClassifier.test.ts (12 cases) — null, empty, marker-only,
pBdr (with + without borders defined), text content, mixed paragraphs,
whitespace-only, continuationSeparator marker.
Visible rendering of the 'explicit' case (toFlowBlocks + layout-bridge
fragment emission) is deferred — none of the SD-2985 acceptance docs uses
non-default separator content, so the implementation is groundwork for
documents in the wild.
§17.11.21 w:pos / ST_FtnPos §17.18.34 — document-wide footnote placement attribute, with four enum values: pageBottom (default), beneathText, sectEnd, docEnd. Per §17.11.21 normative text, section-level w:pos is ignored at render time — only document-wide pos drives behavior. Foundation: - readFootnotePosition / readEndnotePosition in document-settings.ts (rejects unknown values per ST_FtnPos enum). - ConverterContext gains footnotePosition / endnotePosition fields. - PresentationEditor reads both up-front and threads them through. Visible behavior: - pageBottom (default): unchanged — existing reserve-loop placement. - beneathText / sectEnd / docEnd: currently fall back to pageBottom rendering. The reserve-loop fork that places footnote fragments at the body cursor instead of the page-bottom band is deferred — it's an architectural change to incrementalLayout.ts that warrants its own review. None of the SD-2986 acceptance docs (Simple OnlyOffice, IT-864, sd-2440) uses non-pageBottom placement, so the literal acceptance criteria are unaffected by the deferred renderer. Tests: - document-settings.test.ts: 4 new cases — all 4 enum values, absent pos, unknown value rejection, endnote-variant scope.
Acceptance audit — SD-3049 / SD-2985 / SD-2986 / SD-2987Walked each sub-issue's acceptance criteria against the rendered output of all 10 acceptance/reference fixtures, with unit-test + visible-evidence verification per criterion. Setup10 fixtures rendered three ways and stitched per page. Captures + composed PDF kept locally; not attached due to fixture provenance.
PR closes 4 pages on Fixture E (59 → 55). 9 of 10 fixtures already converge; Fixture C's +2 vs Word is pre-existing and unchanged. Coverage matrix
Aggregate test count189 footnote-specific tests green across 10 suites:
Visible verification — sample evidenceFixture E p16 (SD-3049 break-decision):
basic-footnotes p1 (SD-2985 separator width, §17.11.23 "spans part of the width"):
OOXML feature inventory (
|
§17.11.13 FootnoteRef / §17.11.14 footnoteReference — Word's FootnoteReference rStyle is independent of the first body run's formatting, and Word's source XML includes a literal space run between <w:footnoteRef/> and the first body run. Two visible mismatches in `buildMarkerRun`: 1. Marker inherited bold/italic/letterSpacing from the first body text run. On Keyper Series A the body starts with bold "NTD" — Word renders "³ NTD: ..." (plain marker, bold NTD) but SuperDoc rendered "³NTD: ..." (bold marker, bold NTD, no gap). 2. Marker had no visible separator from body text. Word's source has a literal space between <w:footnoteRef/> and the first body run; that space wasn't reaching the rendered output in our pipeline. Fixes (mirrored in FootnotesBuilder + EndnotesBuilder): - Drop bold/italic/letterSpacing inheritance from `firstTextRun`. Keep fontFamily, base size, and color — those are paragraph-level anchors the marker should share with surrounding context. - Append ` ` (NBSP) to the marker text. NBSP survives every whitespace-collapse path in the line layout, gives a stable gap. Tests: - FootnotesBuilder.test.ts: new case asserts marker does NOT inherit bold/italic/letterSpacing from a bold first text run; existing expectations updated to "<digit> " shape. Visual verification on Keyper page 6 in dev app: Before: ³**NTD**: share classes... (marker bold, no gap) After: ¹ **NTD**: share classes... (marker plain, clear gap) Refs: SD-2656
…ithub.com:superdoc-dev/superdoc into tadeu/sd-2656-feature-footnote-rendering-fidelity
…-footnote-rendering-fidelity
| const s = paginator.states[i]; | ||
| const raw = Math.max(s.maxCursorY ?? 0, s.cursorY ?? 0); | ||
| const trailing = s.trailingSpacing ?? 0; | ||
| (pages[i] as { bodyMaxY?: number }).bodyMaxY = Math.max(s.topMargin ?? 0, raw - trailing); |
There was a problem hiding this comment.
bodyMaxY subtracts state.trailingSpacing, but that field is reset on every advanceColumn (paginator.ts:169) while maxCursorY is preserved across columns. in a two-column page where column 0 sets maxCursorY and column 1 ends with non-zero trailingSpacing, we subtract spacing that wasn't contributed by the fragment that set maxCursorY, so the band paints into the bottom of column 0. since this Y now anchors the entire band, the overlap is visible. can we track trailingSpacing alongside the fragment that set maxCursorY (or only subtract when maxCursorY came from cursorY of the current column)?
There was a problem hiding this comment.
Fixed in 5ed53ee. bodyMaxY now only subtracts trailingSpacing when the current column's cursorY actually owns maxCursorY (cursorY >= maxY). In a two-column page where column 0 sets the high mark and column 1 ends with non-zero trailingSpacing, the trailing belongs to column 1 — subtracting it from column 0's max would clip the band into column 0's content, which is exactly what you flagged.
Pinned with three unit tests in index.test.ts: single-column case (subtract), multi-column case (do not subtract — compares with/without column-1 trailing spacing, asserts equal bodyMaxY), and an empty-content clamp.
| const FN_BAND_OVERHEAD_PX = 22; | ||
| const FN_INTER_REF_GAP_PX = 2; | ||
| const FN_SAFETY_MARGIN_PX = 1; | ||
| const bandOverhead = (refsTotal: number): number => | ||
| refsTotal > 0 ? FN_BAND_OVERHEAD_PX + Math.max(0, refsTotal - 1) * FN_INTER_REF_GAP_PX + FN_SAFETY_MARGIN_PX : 0; |
There was a problem hiding this comment.
the slicer's bandOverhead uses hardcoded FN_BAND_OVERHEAD_PX=22 / INTER_REF_GAP=2 / SAFETY=1, but the planner in incrementalLayout.ts:1330-1342 computes overhead from data-driven topPadding+dividerHeight+separatorSpacingBefore+gap (where separatorSpacingBefore defaults to the first footnote line height). these only coincidentally agree for the default values — for any doc with custom topPadding/dividerHeight/gap they diverge, and body packs onto a page whose band can't fit the refs. can we thread the same overhead values through ctx so both sides agree?
There was a problem hiding this comment.
Threaded through ctx in 5ed53ee. Added getFootnoteBandOverhead(refsTotal) to ParagraphLayoutContext. Layout-engine reads topPadding, dividerHeight, separatorSpacingBefore, gap from options.footnotes and computes the same data-driven formula the planner uses (topPadding + dividerHeight + separatorSpacingBefore + (refs-1)*gap); slicer only adds a 1px safety pad on top.
Also wired the planner's measured safeSeparatorSpacingBefore (which is typically the first-fn lineHeight, not the default 12) back through relayout() so on subsequent passes the slicer sees the same first-fn line height the planner used to size the band. Fallback defaults match the planner's defaults so test callers that don't populate the fields still produce identical math.
| const seen = new Set<string>(); | ||
| const sectionConfigs = options.sectionConfigs ?? new Map<number, SectionNoteConfig>(); | ||
| const refPageById = options.refPageById; | ||
| let counter = options.startCounter; |
There was a problem hiding this comment.
counter is seeded from options.startCounter, but numStartFor(0) is only consulted when a later section boundary triggers a reset. a doc with a section-0-level w:footnotePr/w:numStart override (§17.11.11) renders its first note from the document-level start instead of the section override. should we seed counter via numStartFor(0) when sectionConfigs.has(0)?
There was a problem hiding this comment.
Fixed. Counter is now seeded from numStartFor(0) which returns sectionConfigs.get(0)?.numStart ?? options.startCounter — safe for the no-override case. A single-section doc with section-0 w:footnotePr/w:numStart=5 now starts at 5 instead of 1.
Added a regression test for the single-section case. Also had to update one pre-existing eachPage test that was asserting the buggy behavior (a=1, b=7 when section-0 had numStart=7) — corrected to a=7, b=7 per §17.11.11.
| const footnoteNumbering = computeNoteNumbering(this.#editor?.state, 'footnoteReference', { | ||
| startCounter: footnoteNumberStart, | ||
| defaultNumFmt: footnoteNumberFormat, | ||
| defaultRestart: footnoteNumberRestart, |
There was a problem hiding this comment.
defaultRestart: 'eachPage' is passed in but no refPageById map is supplied, so the eachPage branch inside computeNoteNumbering (line 90) never fires and numbering stays continuous. docs that set w:numRestart="eachPage" will silently render the wrong ordinals. either thread per-ref page assignments from the layout pass or document that eachPage isn't supported yet — same applies to the endnote call at 6116.
There was a problem hiding this comment.
Coerced eachPage to continuous in PresentationEditor with a one-time console.warn per kind (footnote/endnote) until the two-pass pagination handshake exists. Applied to both the document-level default and any section-level overrides in sectionConfigs. Same path for the endnote call site you flagged. Better to render deterministic ordinals than silently-wrong per-page numbers.
The helper still accepts refPageById so its eachPage branch stays testable in isolation; the option's JSDoc now flags it as not wired in the SuperDoc pipeline and points at the two-pass requirement as the follow-up.
| @@ -6041,60 +6063,66 @@ export class PresentationEditor extends EventEmitter { | |||
| let converterContext: ConverterContext | undefined = undefined; | |||
There was a problem hiding this comment.
this block reads ~9 OOXML settings, resolves footnote/endnote numbering, patches the converter, and builds a 14-key ConverterContext — that's OOXML semantics resolution, which packages/layout-engine/CLAUDE.md says shouldn't live in PresentationEditor ("PresentationEditor bridges editor state into layout and paint state. It should not resolve OOXML semantics."). happy to defer, but extracting a buildFootnoteConverterContext helper alongside computeNoteNumbering would put the cache-signature dance in one place and unblock testing it in isolation. up to you on timing.
There was a problem hiding this comment.
Deferred to follow-up per your offer. Added a TODO at the top of the block (above the converterContext declaration) calling out the agreed scope: settings read → numbering → cache-signature dance → ConverterContext assembly into one helper alongside computeNoteNumbering. Will file a ticket and pick it up separately so the cache-signature surface gets unit coverage in isolation.
The numbering/signature work in this PR (per-id signature inclusion + section-0 seed + eachPage coercion) already lives in one logical region of the block, so the extraction lands cleanly.
| const blockFootnoteDemand = ctx.getFootnoteDemandForBlockId?.(block.id) ?? 0; | ||
| let demandChargedPageNumber: number | null = null; | ||
| let demandLocked = false; |
There was a problem hiding this comment.
demandChargedPageNumber, demandLocked, and blockFootnoteDemand are all carried purely to satisfy unused-decl checks per the comment at 998-1003. removing them would make the slicer's state more honest about what it actually tracks per slice. can these be dropped now that the legacy demand-locking path is gone?
There was a problem hiding this comment.
Dropped. Removed blockFootnoteDemand, demandChargedPageNumber, demandLocked along with the void-marked usages and the stale comments. The slicer's per-slice state is now just fromLine, toLine, height, sliceDemand, sliceRefs — what it actually tracks per iteration.
Also dropped the sliceLines import that was orphaned when the line-by-line slicer replaced the bulk slicer.
| @@ -0,0 +1,558 @@ | |||
| # SD-2656 — Footnote Rendering Fidelity (Implementation Plan) | |||
There was a problem hiding this comment.
I think you have commited some plan files by mistake. do you mind removing them from the PR?
There was a problem hiding this comment.
Already removed in e138776 (chore: remove internal SD-2656 planning docs from branch) — the files are gone from the index and gitignored on disk. Good catch.
| * When provided AND the active restart is `eachPage`, the counter resets at | ||
| * each page boundary. Refs not in the map are treated as page 0 (initial). | ||
| */ | ||
| refPageById?: Map<string, number>; |
There was a problem hiding this comment.
refPageById is documented as enabling per-page restart but neither call site in PresentationEditor passes it, so the eachPage code path is dead in practice. either wire it (a follow-up ticket is fine) or drop the option until the two-pass pagination handshake exists — keeping a documented-but-unused knob makes the helper look more capable than it is.
There was a problem hiding this comment.
Updated the JSDoc on refPageById to flag it as not wired in the SuperDoc layout pipeline and explain why (numbering runs before pagination, so per-page assignments aren't available). The orchestrator (PresentationEditor) now coerces eachPage → continuous with a warning, so the user-facing behavior is honest about what's supported.
Kept the option on the helper because the algorithm itself is tested in isolation (the eachPage tests in computeNoteNumbering.test.ts exercise the branch). When the two-pass handshake lands, we wire the param at the caller and remove the coercion.
| // shouldn't push the separator down by that much. | ||
| for (let i = 0; i < pages.length && i < paginator.states.length; i++) { | ||
| const s = paginator.states[i]; | ||
| const raw = Math.max(s.maxCursorY ?? 0, s.cursorY ?? 0); |
There was a problem hiding this comment.
bodyMaxY isn't asserted by any unit test — the overflow suites check pageH - bottomMargin but not the band's actual anchor Y. a 3-paragraph layout with non-zero trailing spacing (and ideally a two-column case) would pin the value and catch the multi-column bug above before it regresses. worth a small fixture in index.test.ts.
There was a problem hiding this comment.
Added describe('bodyMaxY') in index.test.ts with 4 tests covering: single-column trailing-spacing subtraction, the multi-column guard (compares bodyMaxY with vs without column-1 trailing spacing — asserts equal, so the test would fail without the fix), the single-page case where the last cursor owns max, and the empty-content topMargin clamp.
The multi-column test is the one that pins the bug you flagged in 3124.
| @@ -0,0 +1,38 @@ | |||
| /** | |||
There was a problem hiding this comment.
this test asserts formatFootnoteCardinal === formatPageNumber for all formats, but both functions inline the same -${num}- logic for numberInDash, so if both were wrong they'd still agree. pageNumbering.test.ts already spot-checks expected strings — adding a few direct-string assertions here for numberInDash (and the more exotic formats) would close the loop.
There was a problem hiding this comment.
Added direct-string assertions on top of the parity-only tests: numberInDash (-1-, -5-, -12-, -99-), upperRoman (I/IV/IX/XL/XC), lowerRoman (i/iv/ix), and upperLetter/lowerLetter through the 26→AA boundary. Both formatFootnoteCardinal and formatPageNumber are asserted against the expected strings, so the parity-only tests no longer rely on each other for correctness.
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey @tupizz! phew, this wasn't an easy one, thanks for your changes!
Codex caught a few things that might be worth looking into:
[P2] Include computed note numbers in the cache key — packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts:6110-6111
When the note ID order stays the same but the displayed numbering changes — for example togglingcustomMarkFollowson a middle footnote, or moving a ref across a section with anumStart/numFmtoverride — this signature remains unchanged, so#flowBlockCacheis not cleared even though the cached reference runs already contain the old marker text/format. Include the computednumberById/formatById(or equivalent per-ref numbering inputs) in the signature so rendered markers are re-created when numbering changes.[P2] Honor section-0 note numStart before first ref — packages/super-editor/src/editors/v1/core/presentation-editor/layout/computeNoteNumbering.ts:53-55
For a single-section DOCX,readSectionNoteConfigsrecords the finalw:sectPr/w:footnotePr/w:numStartunder section0, but numbering starts fromoptions.startCounterinstead ofnumStartFor(0). In that common case,numStart=5still renders the first note as1until a later section break is encountered; initialize the counter from the active section-0 config.[P2] Supply page assignments for eachPage restarts — packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts:6101-6106
Whenw:numRestartiseachPage, this caller passes the restart mode but never passesrefPageById;computeNoteNumberingexplicitly falls back to continuous numbering without that map. As a result, documents that request per-page footnote/endnote numbering still render as continuous, so the layout needs to feed page assignments back into numbering (or avoid advertising this setting as applied).
Claude caught a few more that I added as inline comments.
Also, I ran the visual tests and saw lots of improvements but I saw documents that seem to have a few issues. I'll send you a list of the problematic documents and my findings in the chat.
thank you!
…band (SD-2656) Footnote pagination on the SD-2656 reference fixture matched Word for the first 18 pages but drifted starting at page 19, ended with 4 extra pages, and was silently clipping band content past the page bottom on dense pages. Architectural changes: - footnoteAnchorsByBlockId now stores per-anchor entries (pmPos + height) instead of a single block-level total. Demand is queried by range, so body line-by-line slicing can charge only what the candidate slice actually anchors — the old "whole-block demand at block entry" charge over-deferred paragraphs whose first lines anchor few fns but whose later lines anchor many. - Body slicer is now range-aware. Each iteration computes the candidate line's range, looks up its anchored-fn demand + ref count, and adds that to the page's running total before checking if the line fits. Pre-slicer advance check previews the first candidate line's demand so the in-slicer force-commit-first-line rule cannot place a line whose anchored fn would push the band off the page (the p19 case in the reference fixture). - Band painter (incrementalLayout.injectFragments) anchors the band at page.bodyMaxY instead of pageH - bottomMargin. layoutDocument now stashes bodyMaxY on each Page after layout settles. This is what Word does — the separator paints immediately under the last body fragment. - computeMaxFootnoteReserve uses bodyMaxY when available so the planner's placementCeiling reflects actual remaining band space. Combined with the range-aware slicer, fn body that can't fit on its anchor page gets split into continuation pages instead of overflowing. - Slicer respects state.pageFootnoteReserve as a floor (alongside range-aware demand). The convergence loop's reserve communicates continuation demand from prior pages; without this floor, body packed the full page on continuation pages and the carried-over fn body dripped 1 line per page. - splitRangeAtHeight and fitFootnoteContent no longer charge a range's spacingAfter when the fitted range completes the input. spacingAfter is the gap to the next paragraph; for the last item in a band slice it's wasted budget. The reference fixture's last fn (4 lines × 18 px body + 21 px spacingAfter = 93 px, against an 89-px band budget) was being force-split to 1 line + 3-line continuation purely because of this. Reference fixture results vs origin/main: - 49 → 46 pages (Word: 45) - 19/43 → 28/43 footnotes match Word's page exactly - max drift +4 → +1 page - 0 band overflows (previously several pages clipped past page bottom) - last fn body on single page (was splitting across 4 pages) Corpus-wide layout sweep (`pnpm test:layout --reference 1.32.0`, 562 docs): - 0 reference / candidate generation failures - 5 docs with page-count changes — all reductions, none increased - The 5 are all large legal-template fixtures with many footnotes - Footnote-only fixtures unchanged page-count Guard tests: - New: packages/layout-engine/layout-bridge/test/footnotePageOverflow.test.ts 4 invariants: no fragment past pageH - bottomMargin under clustered fns, oversized fn body, dense cluster exceeding single band, every ref renders. - New: packages/layout-engine/layout-bridge/test/footnoteCompleteness.test.ts Ref-by-ref completeness invariant. Test status: - @superdoc/layout-engine: 654/654 pass - @superdoc/layout-bridge: 1232/1237 pass. The 5 remaining failures test the legacy fixed-bandTopY + multi-pass-reserve architecture; the band-at-bodyMaxY model supersedes them. To be retargeted as follow-up.
tupizz
force-pushed
the
tadeu/sd-2656-feature-footnote-rendering-fidelity
branch
from
dcc347e to
7548d28
Compare
Both files are local planning artifacts and should not ship with the PR. Net effect on main's tree is zero (they were added then removed within the branch's history).
…SD-2656)
The earlier SD-2656 work painted the band immediately under body
(`bandTopY = bodyMaxY`) to prevent overflow when body packed close to the
band's space. That was correct for the overflow case but inverted Word's
visual convention for the common case: Word anchors the band to the
bottom margin and shows any slack as whitespace BETWEEN body and band;
the prior fix put the whitespace BELOW the band instead.
Per column, compute the total band height from the planner's slice heights
plus separator/divider/padding/gap overhead, then position the band so its
bottom sits at the page's physical bottom margin:
bandTopY = max(bodyMaxY, pageH - originalBottomMargin - totalBandHeight)
- Common case (band shorter than available reserve): the `max` selects
`pageH - bottom - totalBandHeight` → band sits flush against the bottom
margin (Word-style).
- Dense case (band fills its reserve): the `max` selects `bodyMaxY` →
band still hugs body, no overlap. The planner's bodyMaxY-based
`maxReserve` already constrains `totalBandHeight ≤ pageBottomLimit -
bodyMaxY`, so the bottom-anchored bandTopY is always ≥ bodyMaxY in
this case.
The original bottom margin is recovered from
`page.margins.bottom - page.footnoteReserved` (the convergence loop
inflates page.margins.bottom by its per-page reserve).
Verified:
- Carlsbad fixture: same 46 pages, identical fn placement, fn 43 still
single page. No regression on the SD-2656 overflow fix.
- Keyper fixture p9 (the visual report case): separator Y now 989 (was
974). Band bottom 1029 ≈ pageBottomLimit 1027. Whitespace shifted
above the band (matches Word convention).
- All 4 footnotePageOverflow guards pass.
- All 2 footnoteBandOverflow guards pass.
- All 3 footnoteCompleteness guards pass.
- @superdoc/layout-engine: 654/654 pass.
…-2656-feature-footnote-rendering-fidelity
- bodyMaxY: only subtract trailingSpacing when current column's cursorY owns the page max. Fixes a band-overlap bug in multi-column pages where column 0 sets maxCursorY high and column 1 ends with non-zero spacing. - Slicer band overhead now sourced from ctx.getFootnoteBandOverhead, derived data-driven from topPadding + dividerHeight + separatorSpacingBefore + (refs-1)*gap. Planner threads its measured separatorSpacingBefore back through relayout options so slicer and planner agree on band size. - computeNoteNumbering: seed counter from numStartFor(0) so section-0 numStart override (§17.11.11) applies before the first section boundary. - eachPage numRestart: coerced to continuous with a one-time warn until the two-pass pagination handshake exists. Updates the helper doc to flag refPageById as not wired. - flow-block cache signature now includes per-id numberById/formatById, so cached marker text invalidates when ordinals change without a reorder. - Drop dead slicer state (demandChargedPageNumber, demandLocked, blockFootnoteDemand) and the unused sliceLines import. - Add bodyMaxY unit tests (single/multi-column, empty page). - Direct-string assertions for numberInDash, roman, base-26 letter formatters. - Retarget footnoteContinuationDemand, footnoteMultiPass, footnoteSeparatorWidth tests against the bodyMaxY-anchored architecture: bigger body content so fixtures actually exercise their invariants; drop the multi-pass count check (now an implementation detail); use page.bodyMaxY as the band-top anchor instead of pageH - bottomMargin - reserve.
…SD-2656) Implements Word-like footnote pagination per the SD-2656 plan. The body paginator now decides line-by-line whether a new fn anchor can stay on its page based on the MINIMUM first slice of the fn (separator + one renderable line), not the full body height. The rest of each fn body splits to continuation pages. Body slicer (layout-paragraph.ts) - New ctx.getFootnoteAnchorMinStartForBlockId returns range-aware sum of measured first-line heights for fns anchored in a PM range. - computeEffectiveBottom uses minStart for both committed and candidate demand; state.footnoteDemandThisPage accumulates minStart-only sums (not full body) so subsequent body blocks on the same page reserve only the minimum needed for each anchored fn. Layout-engine planner index (index.ts) - FootnoteAnchorEntry gains a measured minStart field, defaulted from options.footnotes.bodyMinStartById or a small height-bounded fallback. - getFootnoteAnchorMinStartForBlockId exposes the per-range minStart sum on ParagraphLayoutContext. Incremental layout bridge (incrementalLayout.ts) - refreshBodyHeights also builds bodyMinStartById (first paragraph's first line height, or first-row / first-image-height for non-text bodies). Threaded through relayout options alongside bodyHeightById. - placeFootnote forces the first renderable slice of every NEW anchor (isContinuation=false), not just the first slice on the page. Cluster pages — many anchored fns on the same body page — now place each fn's first line regardless of placementCeiling. - pageReserve propagates the RAW reserve uncapped: capping at maxReserve stalled convergence when pass-1 body filled the page (maxReserve = 0 -> capped reserve = 0 -> body fills again next pass). Using raw lets the next pass shrink body to match actual placed band content. - MAX_FOOTNOTE_LAYOUT_PASSES raised from 4 to 16 to give the monotonic reserve growth room to settle on dense documents. - Convergence-loop entry is unconditional when refs exist (pass-1 may produce zero reserves yet still need iteration). - findPageIndexForPos now records fallback hits via a module-scoped tracer (no behavior change) so SD_DEBUG_FOOTNOTES traces surface the case for diagnostic and test purposes. - FootnoteLayoutPlan returns structured diagnostics (cappedPages, pendingFootnoteIds) alongside the existing console.warn behavior so callers can inspect final-state outcome without parsing logs. Tracing - SD_DEBUG_FOOTNOTES env var emits one JSON record per layout pass describing the final-state anchor->page map, first-slice->page map, per-page slice ids, reserves, continuation in/out, and any findPageIndexForPos fallbacks. - installFootnoteTraceSink(fn) lets tests capture snapshots programmatically. No-op in production builds. Tests - New footnoteIT923Invariants.test.ts pins three Word-fidelity shapes: page-5 long-fn anchor stays with first slice; page-13 dense cluster of six anchors all start on the anchor page; page-47 signature-page anchor stays with its fn body. All three pass. Results - IT-923 NVCA fixture: 51 pages -> 46 pages (Word: 49). - Anchor=firstSlice on every fn ref; no orphan pages; FOURTH on its page, fn 91 with signature page, exhibit fns 92-94 with EXHIBIT A. - Body fully used per page (no large whitespace gaps). - Tests: layout-engine 657, layout-bridge 1240, layout-tests 313, painter-dom 1100, super-editor footnote subset 93 — all green. The remaining 3-page deficit vs Word's 49 is canvas-vs-Word text measurement (paragraphs wrap to fewer lines in Canvas), not a footnote pagination bug.
4 participants
Summary
Range-aware footnote demand + bodyMaxY-anchored band painting. Closes SD-2656.
The reference fixture (a 45-page legal-brief test doc) vs
origin/main:origin/mainPage-by-page comparison PDF generated locally; not attached due to fixture provenance.
Root cause
Three independent bugs compounded:
pageH − bottomMargin − reserveY, so band content overflowed past the page when reserve was under-allocated. The reference fixture's p19 painted fn 10 body up to y=1234 on a 1056 px page — 810 px clipped invisibly.splitRangeAtHeightchargedrange.spacingAfteragainst available band space for fitted ranges that complete the input. The reference fixture's longest footnote (72 px body + 21 px spacingAfter = 93 px against an 89-px budget) was force-split to 1 line + 3-line continuation, when it should have placed all 4 lines.Architectural changes
footnoteAnchorsByBlockId: Map<string, FootnoteAnchorEntry[]>(wasMap<string, number>) — stores per-anchor pmPos + height.getFootnoteDemandForBlockId(blockId, pmStart?, pmEnd?)/getFootnoteRefCountForBlockId(...)— range-filtered.layout-paragraph.tsiterates line-by-line. Each line consults range-aware demand; pre-slicer advance check previews the first candidate line's demand so the in-slicer force-commit cannot place a line whose anchored fn would push the band off-page.state.footnoteRefsThisPage(newPageStatefield) tracks anchored ref count for band-overhead math.layoutDocumentstashespage.bodyMaxY(max cursor Y minus trailing spacing) after layout settles.incrementalLayout.injectFragments:bandTopY = page.bodyMaxY(waspageH − bottomMargin). Band paints immediately under body.computeMaxFootnoteReserveusesbodyMaxYsoplacementCeilingreflects actual remaining band space → planner splits fn bodies into continuation pages instead of overflowing.state.pageFootnoteReserveas a floor (alongside range-aware demand) so continuation demand from prior pages is honored — prevents fn-body drip on continuation pages.splitRangeAtHeight+fitFootnoteContent: don't chargespacingAfterwhen the fitted range completes the input (no next paragraph below = no gap needed).Guard tests
New
packages/layout-engine/layout-bridge/test/footnotePageOverflow.test.ts— 4 invariants:pageH − bottomMarginunder clustered fnsNew
packages/layout-engine/layout-bridge/test/footnoteCompleteness.test.ts— ref-by-ref completeness invariant.All four guards (plus the two pre-existing
footnoteBandOverflowtests) pass with this PR's code. They fail loud on the prior overflow regression.Layout comparison sweep (corpus-wide)
pnpm test:layout -- --reference 1.32.0against the 562-doc corpus:The widespread diffs are all schema-shape (new fields since v1.32.0):
paintSnapshot.lines[*].layoutSourceIdentity,layoutSnapshot.blocks[*].runs[*].script, andlayoutSnapshot.layout.pages[*].bodyMaxY(the field this PR introduces).Page-count changes (all toward fewer / more compact pages; all are large legal-template fixtures with many footnotes):
The compaction matches the reference fixture's 49→46 improvement.
Footnote-only fixtures (
footnotes/basic-footnotes,footnotes/multi-column-footnotes,pagination/pagination_footnote_break): same page count as v1.32.0.Test plan
@superdoc/layout-enginetest suite — 654/654 pass@superdoc/layout-bridgetest suite — 1232/1237 pass. The 5 remaining failures test the legacy fixed-bandTopY+ multi-pass-reserve architecture (footnoteContinuationDemand,footnoteMultiPass,footnoteSeparatorWidth,footnoteSeparatorSpacing×2). The band-at-bodyMaxYmodel supersedes them; to be retargeted as a follow-up rather than blocking this PR.pnpm dev+agent-browser, layout snapshot inspected per page, comparison PDF generated.footnotePageOverflowinvariants pass.pnpm test:layout --reference 1.32.0): 0 reference / candidate failures, 5 page-count changes (all reductions).Linear: SD-2656