fix(layout): per-section footer constraints for multi-section docs (SD-1837)#2022
fix(layout): per-section footer constraints for multi-section docs (SD-1837)#2022
Conversation
…ring (SD-1837) Footer tables now respect per-section margins in multi-section documents. Previously, all footers were measured using the first section's content width, causing table overflow on sections with different margins. Key changes: - Add margins/pageSize to SectionMetadata for per-section constraint computation - Refactor layoutPerRIdHeaderFooters to compute per-section constraints using composite keys (rId::sN) for section-specific measurements - Handle pct-based table widths by pre-expanding constraints - Add rescaleColumnWidths to all table fragment creation sites (SD-1859) - Unwrap unhandled w:fldSimple fields (FILENAME, etc.) to render cached text
Visual diffs detectedPixel differences were found in visual tests. This is not blocking — reproduce locally with |
Fix multiple issues with footer editing in presentation mode: - Fix hover tooltip only showing on first page by detecting page index from DOM via elementsFromPoint in normalizeClientPoint - Fix double-click on footer selecting body text by properly preventing default browser behavior on pointerdown in footer regions - Fix clicking on page N redirecting to page 1 by keeping Y as global layout coordinates in normalizeClientPoint (only X is adjusted for page centering) and computing page-local Y separately from the page element's bounding rect for header/footer hit testing - Fix switching between footer editors on different pages by cleaning up the previous editing session in enterMode before setting up the new one - Fix footer table layout collapsing by setting table-layout:fixed on tables in the footer ProseMirror editor element - Fix wrong total page count in footer by reading body layout page count via getBodyPageCount dependency instead of header layout - Fix page-number NodeView reading incorrect totalPageCount property
There was a problem hiding this comment.
Pull request overview
This PR fixes multi-section header/footer layout and editing issues by making header/footer measurement and hit-testing section-aware, improving pointer normalization across pages, and correcting page-number/token handling in header/footer contexts.
Changes:
- Add per-section header/footer layout constraints (including handling of table width overflow and section-specific margins/page sizes).
- Fix header/footer editing interactions by improving pointer normalization (page detection, global vs page-local Y) and event handling to avoid native selection/scroll issues.
- Improve page-number/field rendering: correct total page count sourcing, refresh NodeView text when editor options change, and expand OOXML field preprocessing (e.g.,
fldSimple, legacyw:pgNum).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/super-editor/src/extensions/pagination/pagination-helpers.js | Adds a scoped class to header/footer ProseMirror roots to apply header/footer-specific table CSS. |
| packages/super-editor/src/extensions/page-number/page-number.test.js | Updates tests to use editor.options.totalPageCount when present. |
| packages/super-editor/src/extensions/page-number/page-number.js | Prefers totalPageCount option, and refreshes NodeView text in update(). |
| packages/super-editor/src/core/super-converter/field-references/preProcessPageFieldsOnly.test.js | Updates/adds tests for fldSimple unwrapping and legacy w:pgNum conversion. |
| packages/super-editor/src/core/super-converter/field-references/preProcessPageFieldsOnly.js | Adds fldSimple handling changes and legacy w:pgNum → sd:autoPageNumber. |
| packages/super-editor/src/core/presentation-editor/types.ts | Extends HeaderFooterRegion with section-aware displayPageNumber. |
| packages/super-editor/src/core/presentation-editor/pointer-events/EditorInputManager.ts | Passes pageIndex/pageLocalY through normalization + hit testing; prevents native selection on H/F pointerdown. |
| packages/super-editor/src/core/presentation-editor/header-footer/HeaderFooterSessionManager.ts | Improves hit testing with optional known page index/local Y; fixes editor session switching cleanup; sources total pages from body layout. |
| packages/super-editor/src/core/presentation-editor/dom/PointerNormalization.ts | Keeps Y in global layout coords; detects page via elementsFromPoint and computes pageLocalY from DOM rect. |
| packages/super-editor/src/core/presentation-editor/dom/PointerNormalization.test.ts | Updates expectations for new normalization return shape. |
| packages/super-editor/src/core/presentation-editor/PresentationEditor.ts | Wires new hit-test callback signature and provides getBodyPageCount. |
| packages/super-editor/src/core/header-footer/HeaderFooterRegistry.ts | Forces page-number DOM refresh after setOptions() updates in header/footer editors. |
| packages/super-editor/src/core/header-footer/HeaderFooterPerRidLayout.ts | Implements per-section header/footer measurement with composite keys and table-width-aware constraints. |
| packages/super-editor/src/core/header-footer/EditorOverlayManager.ts | Allows overlay overflow for footer table overflow behavior. |
| packages/super-editor/src/assets/styles/elements/prosemirror.css | Adds header/footer-scoped table CSS overrides. |
| packages/layout-engine/pm-adapter/src/sections/analysis.ts | Publishes section margins and pageSize into SectionMetadata. |
| packages/layout-engine/pm-adapter/src/sections/analysis.test.ts | Updates expected SectionMetadata to include nullable margins/pageSize. |
| packages/layout-engine/layout-engine/src/layout-table.ts | Adds proportional rescaling of table column widths when clamped to narrower sections. |
| packages/layout-engine/layout-engine/src/layout-table.test.ts | Adds tests for the new column-width rescaling behavior. |
| packages/layout-engine/layout-bridge/src/incrementalLayout.ts | Extends HeaderFooterLayoutResult to include effectiveWidth. |
| packages/layout-engine/contracts/src/index.ts | Extends contracts for SectionMetadata (margins/pageSize) and TableFragment (columnWidths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Rescales column widths when a table is clamped to fit a narrower section. | ||
| * | ||
| * In mixed-orientation documents, tables are measured at the widest section's | ||
| * content width but may render in narrower sections. When the measured total | ||
| * width exceeds the fragment width, column widths must be proportionally | ||
| * rescaled so cells don't overflow the fragment container (SD-1859). | ||
| * | ||
| * @returns Rescaled column widths if clamping occurred, undefined otherwise. | ||
| */ | ||
| function rescaleColumnWidths( | ||
| measureColumnWidths: number[] | undefined, | ||
| measureTotalWidth: number, | ||
| fragmentWidth: number, | ||
| ): number[] | undefined { | ||
| if (!measureColumnWidths || measureColumnWidths.length === 0) { | ||
| return undefined; | ||
| } | ||
| // When the table fits within the fragment, return original widths unchanged. | ||
| // This ensures every table fragment is self-contained with its own column widths, | ||
| // which is critical for header/footer tables where multiple sections share the same | ||
| // blockId but have different content widths (SD-1837). | ||
| if (measureTotalWidth <= fragmentWidth || measureTotalWidth <= 0) { | ||
| return measureColumnWidths; |
There was a problem hiding this comment.
The JSDoc says this helper returns “Rescaled column widths if clamping occurred, undefined otherwise,” but the implementation returns the original measureColumnWidths when the table fits. Either update the docstring to reflect the actual behavior (always returns widths when available) or change the implementation to return undefined when no rescaling occurs—whichever the callers expect.
| * When knownPageIndex is provided (from normalizeClientPoint), use it directly | ||
| * since y is already page-local. Otherwise derive pageIndex from global y. |
There was a problem hiding this comment.
hitTestRegion’s doc comment says “use [knownPageIndex] directly since y is already page-local,” but normalizeClientPoint now keeps y as global layout Y and provides page-local Y separately. This comment is misleading and makes the coordinate conventions harder to reason about—please update it to reflect that knownPageLocalY is page-local while y is global.
| * When knownPageIndex is provided (from normalizeClientPoint), use it directly | |
| * since y is already page-local. Otherwise derive pageIndex from global y. | |
| * `y` is a global layout Y coordinate. When `knownPageIndex` and `knownPageLocalY` | |
| * are provided (from normalizeClientPoint), use them directly as the page index | |
| * and page-local Y. Otherwise, derive pageIndex and page-local Y from the global `y`. |
| availableHeight: region.height, | ||
| currentPageNumber: region.pageNumber, | ||
| totalPageCount: layout?.pages?.length ?? 1, | ||
| currentPageNumber: parseInt(region.displayPageNumber ?? '', 10) || region.pageNumber, |
There was a problem hiding this comment.
displayPageNumber is derived from page.numberText, which can be non-decimal (roman numerals, letters, etc.). Using parseInt(region.displayPageNumber, 10) will produce NaN for those formats and silently fall back to the physical pageNumber, so header/footer editing will show the wrong page number for non-decimal numbering schemes. Consider passing the section-aware numeric page index separately (if you need an integer), or loosening the header/footer editor option type/validation to accept a display string for page-number rendering.
| currentPageNumber: parseInt(region.displayPageNumber ?? '', 10) || region.pageNumber, | |
| currentPageNumber: region.pageNumber, |
| // unwrap the field and emit child content directly. | ||
| // The child elements (w:r > w:t) contain the cached display value | ||
| // that Word rendered when the document was last saved. | ||
| const childElements = node.elements || []; | ||
| if (childElements.length > 0) { | ||
| for (const child of childElements) { | ||
| if (Array.isArray(child.elements)) { | ||
| const childResult = preProcessPageFieldsOnly(child.elements, depth + 1); | ||
| child.elements = childResult.processedNodes; | ||
| } | ||
| processedNodes.push(child); | ||
| } | ||
| i++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This new behavior unwraps all unhandled w:fldSimple nodes and drops the field wrapper/instruction, which conflicts with the function’s stated goal of preserving non-page fields for round-trip fidelity (and with callers like the DOCX importer that rely on that behavior). If this is only meant for rendering cached display text, consider keeping enough metadata to re-export the field later, or applying this unwrapping only in a layout/rendering-specific path rather than during import conversion.
| // unwrap the field and emit child content directly. | |
| // The child elements (w:r > w:t) contain the cached display value | |
| // that Word rendered when the document was last saved. | |
| const childElements = node.elements || []; | |
| if (childElements.length > 0) { | |
| for (const child of childElements) { | |
| if (Array.isArray(child.elements)) { | |
| const childResult = preProcessPageFieldsOnly(child.elements, depth + 1); | |
| child.elements = childResult.processedNodes; | |
| } | |
| processedNodes.push(child); | |
| } | |
| i++; | |
| continue; | |
| } | |
| // preserve the field node and its instruction for round-trip fidelity, | |
| // but still recursively process child content so that any nested PAGE/NUMPAGES | |
| // fields are handled correctly. | |
| const childElements = node.elements || []; | |
| if (Array.isArray(childElements) && childElements.length > 0) { | |
| const childResult = preProcessPageFieldsOnly(childElements, depth + 1); | |
| node.elements = childResult.processedNodes; | |
| } | |
| processedNodes.push(node); | |
| i++; | |
| continue; |
| } | ||
|
|
||
| #normalizeClientPoint(clientX: number, clientY: number): { x: number; y: number } | null { | ||
| #normalizeClientPoint(clientX: number, clientY: number): { x: number; y: number; pageIndex?: number } | null { |
There was a problem hiding this comment.
#normalizeClientPoint is typed as returning { x, y, pageIndex? }, but normalizeClientPointFromPointer now also returns pageLocalY. Since downstream code (via EditorInputCallbacks) consumes pageLocalY, it would be clearer/safer to include pageLocalY?: number in this method’s return type to avoid future refactors accidentally dropping it.
| #normalizeClientPoint(clientX: number, clientY: number): { x: number; y: number; pageIndex?: number } | null { | |
| #normalizeClientPoint( | |
| clientX: number, | |
| clientY: number, | |
| ): { x: number; y: number; pageIndex?: number; pageLocalY?: number } | null { |
| /** | ||
| * Resolve the rId for each section, inheriting from previous sections when not explicitly set. | ||
| * This follows Word's OOXML inheritance model: if a section has no ref for a given kind, | ||
| * it inherits the previous section's ref. | ||
| */ | ||
| function resolveRIdPerSection(sectionMetadata: SectionMetadata[], kind: 'header' | 'footer'): Map<number, string> { | ||
| const result = new Map<number, string>(); | ||
| let inherited: string | undefined; | ||
|
|
||
| for (const section of sectionMetadata) { | ||
| const refs = kind === 'header' ? section.headerRefs : section.footerRefs; | ||
| const rId = refs?.default; | ||
| if (rId) { | ||
| inherited = rId; | ||
| } | ||
| if (inherited) { | ||
| result.set(section.sectionIndex, inherited); | ||
| } | ||
| } |
There was a problem hiding this comment.
In the per-section layout path, resolveRIdPerSection only considers section.headerRefs/footerRefs.default. That means rIds referenced only by first/even/odd variants may never be laid out (and therefore won’t get composite ${rId}::s${sectionIndex} entries), causing createDecorationProvider to miss PRIORITY 1 for those pages and fall back to the variant-based/global layout. Consider including all variant refs when building the section→rId mapping (or at least ensuring every rId in blocksByRId gets a layout result, even if it’s only the fallback constraint).
|
|
||
| const result = normalizeClientPoint(options, 200, 150); | ||
| expect(result).toEqual({ x: 105, y: 90 }); | ||
| expect(result).toEqual({ x: 105, y: 90, pageIndex: undefined }); |
There was a problem hiding this comment.
normalizeClientPoint now always returns a pageLocalY property (even when undefined). This test’s strict toEqual assertion omits pageLocalY, so it will fail because the actual object includes pageLocalY: undefined. Update the expectation to include pageLocalY: undefined or switch the assertion to toMatchObject/property-level checks.
| expect(result).toEqual({ x: 105, y: 90, pageIndex: undefined }); | |
| expect(result).toEqual({ x: 105, y: 90, pageIndex: undefined, pageLocalY: undefined }); |
![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: 2bca00577f
ℹ️ 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".
| y: state.cursorY, | ||
| width, | ||
| height, | ||
| columnWidths: rescaleColumnWidths(measure.columnWidths, measure.totalWidth, width), |
There was a problem hiding this comment.
This writes resized widths into TableFragment.columnWidths, but the painter still renders table cells from measure.columnWidths (see packages/layout-engine/painters/dom/src/table/renderTableFragment.ts), so the new rescaling path is never consumed. In sections where a table fragment is clamped narrower than its measured grid, cells will still use the oversized columns and overflow the fragment width despite this change.
Useful? React with 👍 / 👎.
| this.#overlayManager.hideEditingOverlay(); | ||
| this.#activeEditor = null; | ||
| this.#session = { mode: 'body' }; |
There was a problem hiding this comment.
This eagerly resets the active header/footer session to body mode before descriptor/page-mount/editor setup completes. If any later branch returns early (for example missing descriptor, page mount timeout, or overlay creation failure), the method exits without #emitModeChanged()/overlay restoration, which can leave banner/awareness and selection-overlay state inconsistent with the actual mode after a failed page switch.
Useful? React with 👍 / 👎.
1 participant
Demo on the bugs and proposed solution
CleanShot.2026-02-13.at.16.18.54.mp4
Summary
Fix per-section footer constraints for multi-section documents and fix footer editing interaction bugs.
Per-section footer layout (SD-1837)
fldSimplefields (e.g., PAGE, NUMPAGES) in footers now render correctly during layoutFooter editing: hover tooltip on all pages
normalizeClientPointdid not detect which page the pointer was over, sohitTestRegionalways derived page index 0 from the Y coordinateelementsFromPointinnormalizeClientPointand passpageIndex+pageLocalY(computed from the page element's bounding rect) tohitTestRegionFooter editing: double-click selecting body text
pointerdownwas not callingevent.preventDefault()when the click landed on a footer region, allowing the browser to perform native text selectionevent.preventDefault()in#handlePointerDownwhen a header/footer region is detectedFooter editing: clicking page N redirecting to page 1
normalizeClientPointwas subtractinggetPageOffsetYfrom the Y coordinate, making it page-local. But all downstream consumers (clickToPosition, selection handling) expect global layout Yyas global layout coordinates innormalizeClientPoint. ComputepageLocalYseparately from the page element'sgetBoundingClientRectand pass it only to the header/footer hit testing pathFooter editing: switching pages scrolls to wrong page
#handleClickInHeaderFooterModeconsumed thepointerdownevent withoutpreventDefault, allowing native browser scroll. Additionally,#enterModedid not clean up the previous editing session before creating a new onepreventDefault). Clean up the previous session (disable editor, hide overlay) at the start of#enterModeFooter editing: table layout collapse
table-layout: autoinprosemirror.csscauses browsers to ignore explicit<col>widths fromcreateColGroup()table-layout: fixedon tables in the footer ProseMirror editor element after creationFooter editing: wrong total page count
HeaderFooterSessionManagerreadthis.#headerLayoutResults[0].layout.pages.length(the header/footer's own layout, always 1 page) instead of the body layout. Additionally,page-number.jsread a non-existentparentEditor.currentTotalPagespropertygetBodyPageCountdependency to read the actual body page count. Updatepage-number.jsto readeditor.options.totalPageCountfirst (set during editor creation)Footer editing: fldSimple field preprocessing
fldSimplefields withPAGEorNUMPAGESinstructions were not preprocessed for standalone renderingfldSimpleelements inpreProcessPageFieldsOnlyalongside existingfldChar-based field processingTest plan
pnpm --filter super-editor test— all 5547 tests pass