Skip to content

feat(datagrid): move the cell editor to the row above or below with the arrow keys - #2573

Merged
datlechin merged 1 commit into
mainfrom
feat/cell-editor-vertical-navigation
Aug 29, 2026
Merged

feat(datagrid): move the cell editor to the row above or below with the arrow keys#2573
datlechin merged 1 commit into
mainfrom
feat/cell-editor-vertical-navigation

Conversation

@datlechin

@datlechin datlechin commented Aug 29, 2026

Copy link
Copy Markdown
Member

What

Up and Down in a cell's inline editor now save the cell and reopen the editor on the same column of the row above or below, the way Tab and Shift+Tab already walk the row.

Fixes #2569.

Root cause

The inline editor is a standalone NSTextView overlay (CellOverlayEditor), not a field editor. Its textView(_:doCommandBy:) read insertNewline:, cancelOperation:, insertTab: and insertBacktab: and nothing else, so moveUp: and moveDown: fell through to the text view, where a single-line value has no line to move to and the keystroke did nothing.

AppKit already has the vocabulary for the missing half. NSTextMovement declares up and down beside tab and backtab as "movement codes for movement between fields", and a field editor reports them when the user leaves the field with an arrow key. The overlay is not a field editor, so it has to read the four selectors itself.

The fix

onTabNavigation(row:column:forward:) was a boolean-flagged callback that could only ever express two of the four movements. It becomes onMovement(row:column:movement:) carrying a CellEditorMovement enum with the same four cases AppKit names, and TableViewCoordinator.movementTarget(from:movement:in:) resolves all four in one switch. Tab and Shift+Tab keep their existing meaning, including the wrap onto the next or previous row. Up and Down hold the column, step one row, and stop at the ends rather than wrapping.

A cell value can hold line breaks (Option+Return inserts one), so the arrows belong to the value's own lines first. CellEditorArrowExit is the rule, and it is pure: the editor is left when the value is a single line, or when the caret sits on the first or last line with nothing selected. A line break is the only thing that starts a line here because the overlay never wraps text, which CellOverlayTextLayoutTests already pins.

Both Shift+Arrow (moveUpAndModifySelection:) and Option/Command arrows map to selectors of their own, so extending a selection and jumping to the ends of the value keep their native meaning. Control+P and Control+N do map to moveUp:/moveDown: in StandardKeyBinding.dict, and they leave the cell too: Cocoa's key bindings exist so code answers the command rather than the key, and gating on a key code would break both those and a user's own DefaultKeyBinding.dict.

An active input method keeps the key. Until the composition is committed the text view holds provisional text, so leaving the cell would save a half-composed value and carry the editor off it. hasMarkedText() now guards all four movements, not just the two new ones: Tab and Shift+Tab had the same hole. An arrow is handed back so the caret still moves through the marked text; Tab is swallowed rather than turned into a literal tab.

Why commit-then-navigate is safe against a re-sort

The editor commits before the target row is resolved, which raises the obvious question of whether the commit can reorder the grid under the number about to be used. It cannot, in the same runloop turn. Nothing recomputes a display position synchronously from a cell edit: the structure grid's currentProvider is a stored property the view sets per render, the CSV inspector's recomputeDisplay() hands its filter and sort to a detached task, and the data tab's .fullReplace lives in applyDelta, which no cell commit reaches. A later render cannot disturb an open editor either, because DataGridView.updateNSView returns at its third line while one is active. The reorder lands when the editor closes, with no editor open to be wrong about.

Four defects found in the same routine, fixed with it

The IME hole above is the first: Tab and Shift+Tab have shipped committing a half-composed value since the overlay editor existed.

The other three are in cursor handling. The vertical step needs the cell cursor to follow the editor, and the tab path's own turned out to be incomplete in three ways, all fixed by routing both through the grid's existing KeyHandlingTableView.focusCell(row:column:):

  • handleOverlayTabNavigation selected the target row but never scrolled it into view, so Tab wrapping onto the row below the last visible one opened the editor off screen and typing went into an invisible field.
  • It never moved focusedColumn, so after Tab carried the editor to the next column, closing it put the cell cursor back on the column the editing did not happen in.
  • postCellCursorMoved() reached for the cell's accessibility element unconditionally, and asking for one calls DataGridAccessibility.markActive(). One Tab press in the grid therefore switched every grid in the session to its accessibility layout, mounting a view per visible cell, with no assistive client attached: exactly the cost Data grid breaks with many columns: flickering, columns stop rendering, horizontal scroll lags behind viewport #2381 removed. It now returns early unless a client has already asked the grid something, which is the only state in which the element it posts about exists at all.

Testing

  • CellEditorArrowExitTests (10 cases): the caret rule, over single-line, empty, multi-line, trailing-break, \r\n, selection and out-of-range-selection inputs.
  • CellEditorMovementTargetTests (9 cases): target resolution on a real grid harness, covering the vertical step, both ends, the existing tab and backtab wraps, and that moving the cursor no longer activates the accessibility layout.
  • CellOverlayEditorMovementTests (8 cases): which selectors the editor takes and which it leaves to the text view, the movement each one reports, and both IME cases driven through setMarkedText.
  • verify.sh build: PASS.
  • verify.sh test over the three new suites plus KeyHandlingTableViewOverlayTests, CellOverlayTextLayoutTests, DrawnCellReachabilityTests, DataGridRowIdentityTests, FocusedColumnResolutionTests, KeyHandlingTableViewCopyTests and TableViewCoordinatorLayoutTests: PASS.
  • verify.sh docs: PASS.
  • swiftlint --strict over all seven changed files: 0 violations. verify.sh lint itself reports FAIL, on three findings in MainSplitViewController+TabStripAccessory.swift and EditorTabContextMenuBuilder.swift and one stale AXCell symbol in CLAUDE.md. All four are on main already: those files are byte-identical to origin/main on this branch.
  • The accessibility guard was checked in the negative as well: removing it makes movingTheCursorLeavesAccessibilityAlone() the single failure out of the suite's nine.

No TableProUITests coverage. Driving this flow needs a click into a cell of a live editable table and then a read of the overlay's contents, and the drawn-cell grid publishes no element for either: a row is as wide as the grid so its centre is empty width, columns are siblings of rows and later in the tree, so XCUITest reads every row and cell as obscured and refuses to click them, and the overlay text view carries no identifier of its own. Adding one purely to be asserted on would be test machinery in shipping code.

Screenshots

Not included: the change is a keystroke, and every frame of it looks like the editor already looks. The visible difference is which row the editor is on after the key, which a still does not carry.

@mintlify

mintlify Bot commented Aug 29, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 29, 2026, 11:39 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit 1df6455 into main Aug 29, 2026
9 checks passed
@datlechin
datlechin deleted the feat/cell-editor-vertical-navigation branch August 29, 2026 11:51
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.

I would like the up and down arrow keys to move the field being edited to the row above or below

1 participant