Conversation
…-1962) Embedded tables inside table cells now paginate correctly at row boundaries across pages, even when nested multiple levels deep (table-in-table-in-table). Previously, a nested table was treated as a single unsplittable segment, causing its content to be clipped or lost on continuation pages. Layout engine changes: - Add getEmbeddedRowLines() to recursively expand nested table rows into sub-segments instead of treating each row as one segment - Update getCellLines() to call recursive expansion for table blocks - Update computeCellPmRange() to use recursive segment counts Renderer changes: - Add getCellSegmentCount(), getEmbeddedRowSegmentCount(), and getEmbeddedTableSegmentCount() helpers mirroring the layout logic - Update blockLineCounts to use recursive segment counting - Compute per-row partial rendering info (PartialRowInfo) when a nested table row straddles a page break - Pass partialRow through renderEmbeddedTable → TableFragment so renderTableFragmentElement handles mid-row splits naturally
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
…entation docs (SD-1962) Blocks are now measured at their own section's content width instead of the global maximum across all sections. In mixed-orientation documents (portrait + landscape), the old approach measured all blocks at the widest section width, causing text line breaks to be computed too wide for narrower sections — resulting in text clipping inside table cells. - Add computePerSectionConstraints() to resolve per-block measurement dimensions from section breaks - Update measurement loop to use per-block constraints - Update remeasureAffectedBlocks() to use per-block constraints - Keep resolveMeasurementConstraints() for cache invalidation
There was a problem hiding this comment.
Pull request overview
This pull request fixes recursive pagination for deeply nested tables (table-in-table-in-table) by making both the layout engine's segment expansion and the renderer's segment counting recursive. Previously, deeply nested tables were treated as single unsplittable segments, causing content to be clipped or lost on continuation pages.
Changes:
- Added recursive segment expansion in the layout engine via
getEmbeddedRowLines()to enable splitting at sub-row boundaries within nested tables - Updated renderer segment counting to recursively count embedded table segments, ensuring alignment with layout engine logic
- Added support for partial row rendering in embedded tables with new parameters (
fromRow,toRow,partialRow,availableWidth) inrenderEmbeddedTable() - Implemented column width rescaling for embedded tables when measurement-scale exceeds render-scale
- Updated non-paragraph blocks (images, drawings) to occupy 1 segment in pagination calculations
- Added force-progress logic in
computePartialRow()to handle segments taller than a full page
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/layout-engine/layout-engine/src/layout-table.ts | Added getEmbeddedRowLines() for recursive segment expansion; updated getCellLines() to recursively expand embedded table rows; updated computeCellPmRange() to handle embedded table segments; added fullPageHeight parameter to computePartialRow() for force-progress logic |
| packages/layout-engine/painters/dom/src/table/renderTableCell.ts | Added segment counting helpers (getCellSegmentCount(), getEmbeddedRowSegmentCount(), getEmbeddedTableSegmentCount()); updated renderEmbeddedTable() to support partial rendering and column rescaling; updated cell rendering loop to handle embedded table pagination and non-paragraph block segments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Get all lines from a cell's blocks (multi-block or single paragraph). | ||
| * | ||
| * Cells can have multiple blocks (cell.blocks) or a single paragraph (cell.paragraph). | ||
| * This function normalizes access to all lines across all paragraph blocks. | ||
| * | ||
| * @param cell - Cell measure | ||
| * @returns Array of all lines with their lineHeight | ||
| */ |
There was a problem hiding this comment.
There is a duplicate documentation comment. Lines 339-347 appear to be an orphaned JSDoc comment for getCellLines (which follows getEmbeddedRowLines). This creates confusion as it appears to document getEmbeddedRowLines but actually describes getCellLines. The orphaned comment should be removed since getCellLines already has proper inline documentation.
| segIdx++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The partial row height computation (lines 936-979) doesn't handle cells that use the legacy cell.paragraph property instead of cell.blocks. When cell.blocks is undefined or empty, but cell.paragraph exists, the height computation will incorrectly return 0 instead of computing the height based on paragraph lines. This could lead to incorrect partial row heights for cells with legacy structure. Consider adding an else clause after line 977 to handle the cell.paragraph case, similar to how getCellSegmentCount handles it at lines 118-120.
| } | |
| } | |
| } else if (cell.paragraph && cell.paragraph.lines && cell.paragraph.lines.length > 0) { | |
| const lines = cell.paragraph.lines; | |
| const start = Math.max(0, cellFrom); | |
| const end = Math.min(cellTo, lines.length); | |
| for (let i = start; i < end; i++) { | |
| const line = lines[i]; | |
| cellVisHeight += (line && line.lineHeight) || 0; | |
| } |
| for (let s = 0; s < nestedRowSegs; s++) { | ||
| if (segIdx >= cellFrom && segIdx < cellTo) { | ||
| cellVisHeight += (nestedRow.height || 0) / nestedRowSegs; |
There was a problem hiding this comment.
The height computation for partially visible nested table rows (line 962) divides the row height evenly by segment count. This is an approximation that may be inaccurate when nested table rows contain cells with non-uniform line heights. For example, if a nested table row has 3 segments but the first segment occupies 50% of the height and the other two segments split the remaining 50%, dividing evenly will misrepresent the actual visible height. Consider implementing a more precise height calculation that accounts for the actual distribution of heights within the nested table row, similar to how paragraph line heights are summed individually.
| for (let s = 0; s < nestedRowSegs; s++) { | |
| if (segIdx >= cellFrom && segIdx < cellTo) { | |
| cellVisHeight += (nestedRow.height || 0) / nestedRowSegs; | |
| const partialInfo = nestedRow as unknown as PartialRowInfo | undefined; | |
| const segHeights = | |
| partialInfo && Array.isArray(partialInfo.segmentHeights) | |
| ? partialInfo.segmentHeights | |
| : undefined; | |
| for (let s = 0; s < nestedRowSegs; s++) { | |
| if (segIdx >= cellFrom && segIdx < cellTo) { | |
| if (segHeights && segHeights.length === nestedRowSegs) { | |
| cellVisHeight += segHeights[s] || 0; | |
| } else { | |
| cellVisHeight += (nestedRow.height || 0) / nestedRowSegs; | |
| } |
| let partialRowInfo: PartialRowInfo | undefined; | ||
|
|
||
| for (let r = 0; r < tableMeasure.rows.length; r++) { | ||
| const rowSegs = rowSegmentCounts[r]; | ||
| const rowStart = segmentOffset; | ||
| const rowEnd = segmentOffset + rowSegs; | ||
| segmentOffset = rowEnd; | ||
|
|
||
| // Skip rows completely outside the range | ||
| if (rowEnd <= localFrom || rowStart >= localTo) continue; | ||
|
|
||
| if (embeddedFromRow === -1) embeddedFromRow = r; | ||
| embeddedToRow = r + 1; | ||
|
|
||
| // Check if this row needs partial rendering (recursive row with nested tables) | ||
| if (rowSegs > 1 && (rowStart < localFrom || rowEnd > localTo)) { | ||
| // This row is partially visible — compute per-cell fromLine/toLine | ||
| const rowLocalFrom = Math.max(0, localFrom - rowStart); | ||
| const rowLocalTo = Math.min(rowSegs, localTo - rowStart); | ||
| const row = tableMeasure.rows[r]; | ||
|
|
||
| const fromLineByCell: number[] = []; | ||
| const toLineByCell: number[] = []; | ||
| let partialHeight = 0; | ||
|
|
||
| for (const cell of row.cells) { | ||
| const cellTotal = getCellSegmentCount(cell); | ||
| const cellFrom = Math.min(rowLocalFrom, cellTotal); | ||
| const cellTo = Math.min(rowLocalTo, cellTotal); | ||
| fromLineByCell.push(cellFrom); | ||
| toLineByCell.push(cellTo); | ||
|
|
||
| // Compute visible height for this cell's segment range | ||
| let cellVisHeight = 0; | ||
| if (cell.blocks && cell.blocks.length > 0) { | ||
| let segIdx = 0; | ||
| for (const blk of cell.blocks) { | ||
| if (blk.kind === 'paragraph') { | ||
| const lines = (blk as ParagraphMeasure).lines || []; | ||
| for (const line of lines) { | ||
| if (segIdx >= cellFrom && segIdx < cellTo) { | ||
| cellVisHeight += line.lineHeight || 0; | ||
| } | ||
| segIdx++; | ||
| } | ||
| } else if (blk.kind === 'table') { | ||
| const nestedTable = blk as TableMeasure; | ||
| for (const nestedRow of nestedTable.rows) { | ||
| const nestedRowSegs = getEmbeddedRowSegmentCount(nestedRow); | ||
| for (let s = 0; s < nestedRowSegs; s++) { | ||
| if (segIdx >= cellFrom && segIdx < cellTo) { | ||
| cellVisHeight += (nestedRow.height || 0) / nestedRowSegs; | ||
| } | ||
| segIdx++; | ||
| } | ||
| } | ||
| } else { | ||
| const blkHeight = 'height' in blk ? (blk as { height: number }).height : 0; | ||
| if (blkHeight > 0) { | ||
| if (segIdx >= cellFrom && segIdx < cellTo) { | ||
| cellVisHeight += blkHeight; | ||
| } | ||
| segIdx++; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| partialHeight = Math.max(partialHeight, cellVisHeight); | ||
| } | ||
|
|
||
| partialRowInfo = { | ||
| rowIndex: r, | ||
| fromLineByCell, | ||
| toLineByCell, | ||
| isFirstPart: rowLocalFrom === 0, | ||
| isLastPart: rowLocalTo >= rowSegs, | ||
| partialHeight, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The loop can identify multiple rows requiring partial rendering when the visible segment range cuts through multiple rows with nested content. The partialRowInfo variable is reassigned on each iteration (line 981), so only the last partially-rendered row's information is preserved. However, the TableFragment contract only supports a single partialRow, not an array. This means if the first row needs partial rendering (starting mid-row) AND the last row needs partial rendering (ending mid-row), only the last row's partial info will be passed to renderEmbeddedTable, causing the first row to render incorrectly (it would render all its content instead of being clipped). Consider either: (1) ensuring the logic correctly handles the case where both the first and last rows need partial rendering, or (2) adding assertions to verify at most one row can be partial in this context.
| const blockLineCounts: number[] = []; | ||
| for (let i = 0; i < Math.min(blockMeasures.length, cellBlocks.length); i++) { | ||
| const bm = blockMeasures[i]; | ||
| if (bm.kind === 'paragraph') { | ||
| blockLineCounts.push((bm as ParagraphMeasure).lines?.length || 0); | ||
| } else if (bm.kind === 'table') { | ||
| // Embedded tables: recursively count segments (matches getCellLines expansion) | ||
| blockLineCounts.push(getEmbeddedTableSegmentCount(bm as TableMeasure)); | ||
| } else { | ||
| blockLineCounts.push(0); | ||
| // Non-paragraph blocks (image, drawing) occupy 1 segment | ||
| blockLineCounts.push(1); | ||
| } |
There was a problem hiding this comment.
The PR introduces segment counting for non-paragraph blocks (images, drawings), counting each as 1 segment. However, anchored images and drawings are excluded from normal flow rendering (lines 1034-1036, 1079-1081 skip them), but are still counted as segments by both the layout engine's getCellLines (layout-table.ts:398-406) and the renderer's segment counting loop (lines 855-866). If anchored blocks can appear inside table cells, this creates a mismatch: segments will be counted for anchored blocks, but they won't increment cumulativeLineCount during rendering, causing subsequent blocks to render at incorrect segment indices. Verify whether anchored blocks inside table cells are a valid scenario, and if so, both the layout engine and renderer should skip anchored blocks when counting segments.
tupizz
changed the title
1 participant
Before / After
Summary
Test plan
text_cut_table.docxrenders all three nesting levels correctly across pages 11-13🔗 Relates to SD-1962