Preserve cursor position when navigating tables #3284
Preserve cursor position when navigating tables #3284juliaroldi wants to merge 5 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the table-navigation selection logic to preserve the caret’s text offset when moving vertically (ArrowUp/ArrowDown) between table cells, and adds unit tests to validate the behavior.
Changes:
- Capture the current text-node caret offset on ArrowUp/ArrowDown and forward it into table-selection handling.
- Adjust range placement in table cells to reuse the captured text offset when possible.
- Add new SelectionPlugin unit tests covering preserved offsets and normalization behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/roosterjs-content-model-core/lib/corePlugin/selection/SelectionPlugin.ts | Adds text-offset preservation for vertical table navigation and updates range placement logic. |
| packages/roosterjs-content-model-core/test/corePlugin/selection/SelectionPluginTest.ts | Adds unit tests validating preserved text offsets across ArrowUp/ArrowDown navigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.setRangeSelectionInTable( | ||
| td, | ||
| key == Up ? td.childNodes.length : 0, | ||
| textOffset && (key == Up || key == Down) ? textOffset : 0, |
There was a problem hiding this comment.
handleSelectionInTable used to pass td.childNodes.length as the offset when moving up (so the caret lands at the end of the target cell). With the new textOffset plumbing, the fallback becomes always 0 when textOffset is undefined, which changes ArrowUp behavior for non-text selections / non-preserved cases. Consider preserving the prior fallback (end-of-cell for Up, start-of-cell for Down) when textOffset is not provided (and use an explicit textOffset !== undefined check instead of truthiness).
| textOffset && (key == Up || key == Down) ? textOffset : 0, | |
| textOffset !== undefined | |
| ? textOffset | |
| : key == Up | |
| ? td.childNodes.length | |
| : 0, |
| const { node, offset } = normalizePos(cell, nodeOffset); | ||
| const useTextOffset = isNodeOfType(node, 'TEXT_NODE') && node.length >= nodeOffset; | ||
|
|
||
| range.setStart(node, offset); | ||
| range.setStart(node, useTextOffset ? nodeOffset : offset); | ||
| range.collapse(true /* toStart */); |
There was a problem hiding this comment.
setRangeSelectionInTable now sometimes treats nodeOffset as a text offset (setStart(node, nodeOffset)), but it still calls normalizePos(cell, nodeOffset) first. Since normalizePos interprets the offset relative to the current node (text offset for text nodes, child index for element nodes), passing a text offset when cell is an element can cause it to walk down the “lastChild” path and return an unrelated text node (e.g., the last paragraph in the cell). This can place the caret in the wrong location for cells with multiple child elements/paragraphs. Consider separating “DOM child offset” vs “text offset” semantics: compute the target leaf node independently (e.g., first editable text node in the cell), then clamp/apply textOffset only when you have a text node.
| it('From Range with text offset, Press Down - preserves text offset', () => { | ||
| getDOMSelectionSpy.and.returnValue({ | ||
| type: 'range', | ||
| range: { | ||
| startContainer: td2_text, | ||
| startOffset: 1, | ||
| endContainer: td2_text, | ||
| endOffset: 1, | ||
| commonAncestorContainer: tr1, | ||
| collapsed: true, | ||
| }, | ||
| isReverted: false, | ||
| }); |
There was a problem hiding this comment.
These new cursor-offset tests only use single-character text nodes ('1', '2', etc.), so they don't validate the common case where the preserved offset is in the middle of a longer string. Adding at least one case with multi-character text (and possibly nested elements like <div><span>...</span></div>) would better ensure the new offset preservation logic doesn't regress for realistic table cell contents.
When pressing key up or key down in table cells that have text, preserve the cursor position.
