Conversation
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.
* Fix publish file (#3267) * fix publish file * remove file * [Table Improvements] Add undoSnapshot when tab on a table cell (#3265) Add undoSnapshot after pressing Tab key in a table that has new content, otherwise if the user type content in a table and press tab to move to another cell and then undo the content, all the typed content will be removed. * [Table Improvements] Use keyboard to delete rows and columns (#3270) When press backspace or shift + delete when an entire row or column, delete the column and row. * [Table Improvements] Add Shift Cells Table Operation (#3271) Add new shift cells up and shift cells left table operations. These operations move the table cell content to the cells at left or above. * align table cell list (#3275) When apply alignment in table cells that has list items, also apply the alignment to the list items. * fill gaps (#3272) * Bump lodash from 4.17.21 to 4.17.23 (#3266) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.21 to 4.17.23. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.21...4.17.23) --- updated-dependencies: - dependency-name: lodash dependency-version: 4.17.23 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jiuqing Song <jisong@microsoft.com> * fix table format (#3277) When triggering clearFormat on table cells, do not clear the cell or the table format. * [Table Improvements] Add preview for table cell selection (#3274) When start shadow edit, check if table cells are selected, if they are selected, remove the background color to make the styles changes visible in the table. * Fix outdated JSDoc comments in setTableCellsStyle.ts (#3278) Fix JSDoc comments for removeTableCellsStyle function to match actual parameters Fix JSDoc comments for setTableCellsStyle function to match actual parameters * Fix 329516 (#3276) Co-authored-by: Bryan Valverde U <bvalverde@microsoft.com> * [Table Improvements] Insert table content (#3258) When inserting a table in a range selection, insert the selected content inside the table. * Bump webpack from 5.94.0 to 5.104.1 (#3285) Bumps [webpack](https://github.com/webpack/webpack) from 5.94.0 to 5.104.1. - [Release notes](https://github.com/webpack/webpack/releases) - [Changelog](https://github.com/webpack/webpack/blob/main/CHANGELOG.md) - [Commits](webpack/webpack@v5.94.0...v5.104.1) --- updated-dependencies: - dependency-name: webpack dependency-version: 5.104.1 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Filter temporary EOP elements in Word Online paste and add test pattern support (#3283) * Filter temporary EOP elements in Word Online paste and add test pattern support - Skip elements with both 'Selected' and 'EOP' classes during WAC paste processing to remove temporary End of Paragraph markers - Add unit tests for EOP element filtering behavior (3 test cases) - Enhance test runner with --testPathPattern and --testNamePattern flags for faster targeted test execution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Update packages/roosterjs-content-model-plugins/lib/paste/WacComponents/processPastedContentWacComponents.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Dark color improvement (#3279) * Dark color improvement * improve * Fix #3280 (#3282) * [Table Improvements] Ignore span cells when merge table cells (#3281) When merging table cells, count table that are span as one cell, so two or more cells cannot be merge to one single span cell. * Preserve cursor position when navigating tables (#3284) When pressing key up or key down in table cells that have text, preserve the cursor position. * Add adjustWordListMarginParser to fix duplicate list indentation when pasting from Word Desktop (#3288) Word Desktop pastes list items with full indentation as marginLeft on MsoListParagraph elements, which duplicates the paddingInlineStart (40px) already applied by the browser's default list styling. This parser subtracts the default 40px from the marginLeft of list items with the MsoListParagraph class to correct the indentation. - Add adjustWordListMarginParser using parseValueWithUnit for unit conversion - Register parser for listItemElement in processPastedContentFromWordDesktop - Update addParser call count expectations in pasteTest and ContentModelPastePluginTest - Add dedicated test case for the margin adjustment behavior * Remove mutations that is not under editor (#3290) Co-authored-by: Bryan Valverde U <bvalverde@microsoft.com> * Preserve trailing space in last paragraph segment by converting to nbsp (#3235) (#3287) * Preserve trailing space in last paragraph segment by converting to nbsp When the last text segment in a paragraph ends with a regular space, browsers collapse it during rendering. This change detects that case in handleText and replaces the trailing space with a non-breaking space (\u00A0) so it is preserved in the output. To support this, a new ModelToDomSegmentContext interface is introduced that extends ModelToDomContext with an isLastSegment flag. handleParagraph sets this flag for each segment before dispatching, and ContentModelSegmentHandler is updated to use ModelToDomSegmentContext as its context type, eliminating the need for type casts in the handlers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Improve trailing space to nbsp conversion with noFollowingTextSegmentOrLast Refactor handleParagraph to track whether a text segment is the last in the paragraph or has no following text segment (excluding SelectionMarkers). This ensures trailing spaces are converted to not only for the very last segment, but also when the next non-marker segment is not a Text segment. - Convert forEach to for loop in handleParagraph for segment iteration - Extract hasTextSegmentAfter helper to check for upcoming text segments - Add noFollowingTextSegmentOrLast property to ModelToDomSegmentContext - Update handleText to use the new property name - Fix stale isLastSegment references in handleTextTest - Add comprehensive tests for noFollowingTextSegmentOrLast in handleParagraphTest --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Do not invalidate cached model for ContentChangedEvent (#3291) * Bump minimatch from 3.1.2 to 3.1.5 (#3295) Bumps [minimatch](https://github.com/isaacs/minimatch) from 3.1.2 to 3.1.5. - [Changelog](https://github.com/isaacs/minimatch/blob/main/changelog.md) - [Commits](isaacs/minimatch@v3.1.2...v3.1.5) --- updated-dependencies: - dependency-name: minimatch dependency-version: 3.1.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [Table Improvements] Add new customization properties for table format (#3293) * merge model * new table styles * fixes * add test * nit * nit * Bump immutable from 4.0.0 to 4.3.8 (#3298) Bumps [immutable](https://github.com/immutable-js/immutable-js) from 4.0.0 to 4.3.8. - [Release notes](https://github.com/immutable-js/immutable-js/releases) - [Changelog](https://github.com/immutable-js/immutable-js/blob/main/CHANGELOG.md) - [Commits](immutable-js/immutable-js@v4.0.0...v4.3.8) --- updated-dependencies: - dependency-name: immutable dependency-version: 4.3.8 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix merge table error (#3302) 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 * Add 'resolveImageSource' option to support cid image source for canvas editing (#3301) * Feature: support cid: image sources via resolveImageSource for reliable canvas editing * Fix format --------- Co-authored-by: Liang Meng <liangmeng@microsoft.com> Co-authored-by: Jiuqing Song <jisong@microsoft.com> * Fix #3292 Put B/I/U/S under hyperlink (#3297) * Fix #3292 * Update packages/roosterjs-content-model-types/lib/contentModel/format/ContentModelFormatMap.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Keep Margins in the List items, and set the padding of list elements for better paste interop with Wac Components (#3304) * Keep Margins in the List items, and set the padding of list elements for better paste interop with Word Components * Remove obsolete test * Fix cursor position for arrow up (#3305) * merge model * move table cursor * Version bump to 9.48.0 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Julia Roldi <87443959+juliaroldi@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Bryan Valverde U <bvalverde@microsoft.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Liang <jojo.spirit@outlook.com> Co-authored-by: Liang Meng <liangmeng@microsoft.com>
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