Skip to content

Preserve cursor position when navigating tables #3284

Open
juliaroldi wants to merge 5 commits intomasterfrom
u/juliaroldi/preserve-cursor-postion-yab
Open

Preserve cursor position when navigating tables #3284
juliaroldi wants to merge 5 commits intomasterfrom
u/juliaroldi/preserve-cursor-postion-yab

Conversation

@juliaroldi
Copy link
Contributor

@juliaroldi juliaroldi commented Feb 6, 2026

When pressing key up or key down in table cells that have text, preserve the cursor position.
PreserveTextKeys

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
textOffset && (key == Up || key == Down) ? textOffset : 0,
textOffset !== undefined
? textOffset
: key == Up
? td.childNodes.length
: 0,

Copilot uses AI. Check for mistakes.
Comment on lines 579 to 583
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 */);
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +2043 to +2055
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,
});
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant