Conversation
juliaroldi
requested review from
BryanValverdeU,
JiuqingSong,
Copilot,
flyingbee2012,
ianeli1 and
vinguyen12
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash that occurred when pasting table cells containing merged (span) cells (spanAbove or spanLeft) into a target table. The original code used the physical row index i to compute target row placement, but placing spanAbove source cells during the paste loop caused subsequent getTargetRowIndex calls to skip those cells and return out-of-bounds indices. The fix replaces physical indices with logical offsets and relies on the mutation-aware behavior of getTargetRowIndex/getTargetColIndex to correctly distribute span cells across target rows/columns.
Changes:
mergeModel.ts: Pre-computeslogicalRowOffsets(per source row) andlogicalColOffsets(per source cell within a row), then uses these logical offsets instead of raw physical indices when callinggetTargetRowIndexandgetTargetColIndexin the paste loop.mergeModelTest.ts: Adds two new test cases — one for a source table withspanAbovecontinuation rows and one for a source table withspanLeftcells — to verify no crash and correct cell placement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/roosterjs-content-model-dom/lib/modelApi/editing/mergeModel.ts |
Core fix: replaces physical loop indices with pre-computed logical row/column offsets to prevent out-of-bounds table row access during table-to-table merge. |
packages/roosterjs-content-model-dom/test/modelApi/editing/mergeModelTest.ts |
Adds two regression tests covering pasting a source table with spanAbove rows and spanLeft cells respectively. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify first row of source table was placed correctly at target [2,2] | ||
| // logicalRowOffsets[0] = 0, so targetRowIndex = getTargetRowIndex(table, 2, 0, 2) = 2 | ||
| // logicalColOffsets[0] = 0, so targetColIndex = getTargetColIndex(table, 2, 2, 0) = 2 | ||
| expect(table.rows[2].cells[2]).toEqual(newCell00); | ||
|
|
||
| // Second column of first row goes to col 3 | ||
| // logicalColOffsets[1] = 1, so targetColIndex = getTargetColIndex(table, 2, 2, 1) = 3 | ||
| expect(table.rows[2].cells[3]).toEqual(newCell01); | ||
|
|
||
| expect(normalizeTable.normalizeTable).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
The first test only asserts the placement of source row 0 cells (at target [2,2] and [2,3]) but does not verify the placement of the spanAbove continuation rows (1–3) or the second logical row (row 4). For example, asserting that table.rows[3].cells[2] equals newCell10, table.rows[4].cells[2] equals newCell20, etc., and that table.rows[6].cells[2] equals newCell40 would ensure the entire span structure is placed correctly and prevent regressions if the logic changes. Without these assertions, the test primarily validates that there is no crash, but not that the paste output is semantically correct.
When pasting table cells that are merged, counts only the logical cells and not all physical cells, otherwise, the number of pasted cells will appear bigger than then the actually copied. Causing this error:
mergeModel.ts:252 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'cells') at _ (mergeModel.ts:252:44) at t.mergeModel (mergeModel.ts:93:25) at e.formatContentModel.changeSource (mergePasteContent.ts:79:19) at t.formatContentModel (formatContentModel.ts:41:21) at e.formatContentModel (Editor.ts:182:18) at t.mergePasteContent (mergePasteContent.ts:53:12) at t.paste (paste.ts:76:5) at CopyPastePlugin.ts:135:25