feat(datagrid): move the cell editor to the row above or below with the arrow keys - #2573
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
UpandDownin a cell's inline editor now save the cell and reopen the editor on the same column of the row above or below, the wayTabandShift+Tabalready walk the row.Fixes #2569.
Root cause
The inline editor is a standalone
NSTextViewoverlay (CellOverlayEditor), not a field editor. ItstextView(_:doCommandBy:)readinsertNewline:,cancelOperation:,insertTab:andinsertBacktab:and nothing else, somoveUp:andmoveDown: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.
NSTextMovementdeclaresupanddownbesidetabandbacktabas "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 becomesonMovement(row:column:movement:)carrying aCellEditorMovementenum with the same four cases AppKit names, andTableViewCoordinator.movementTarget(from:movement:in:)resolves all four in one switch.TabandShift+Tabkeep their existing meaning, including the wrap onto the next or previous row.UpandDownhold the column, step one row, and stop at the ends rather than wrapping.A cell value can hold line breaks (
Option+Returninserts one), so the arrows belong to the value's own lines first.CellEditorArrowExitis 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, whichCellOverlayTextLayoutTestsalready pins.Both
Shift+Arrow(moveUpAndModifySelection:) andOption/Commandarrows map to selectors of their own, so extending a selection and jumping to the ends of the value keep their native meaning.Control+PandControl+Ndo map tomoveUp:/moveDown:inStandardKeyBinding.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 ownDefaultKeyBinding.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:TabandShift+Tabhad the same hole. An arrow is handed back so the caret still moves through the marked text;Tabis 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
currentProvideris a stored property the view sets per render, the CSV inspector'srecomputeDisplay()hands its filter and sort to a detached task, and the data tab's.fullReplacelives inapplyDelta, which no cell commit reaches. A later render cannot disturb an open editor either, becauseDataGridView.updateNSViewreturns 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:
TabandShift+Tabhave 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:):handleOverlayTabNavigationselected the target row but never scrolled it into view, soTabwrapping onto the row below the last visible one opened the editor off screen and typing went into an invisible field.focusedColumn, so afterTabcarried 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 callsDataGridAccessibility.markActive(). OneTabpress 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 throughsetMarkedText.verify.sh build: PASS.verify.sh testover the three new suites plusKeyHandlingTableViewOverlayTests,CellOverlayTextLayoutTests,DrawnCellReachabilityTests,DataGridRowIdentityTests,FocusedColumnResolutionTests,KeyHandlingTableViewCopyTestsandTableViewCoordinatorLayoutTests: PASS.verify.sh docs: PASS.swiftlint --strictover all seven changed files: 0 violations.verify.sh lintitself reports FAIL, on three findings inMainSplitViewController+TabStripAccessory.swiftandEditorTabContextMenuBuilder.swiftand one staleAXCellsymbol inCLAUDE.md. All four are onmainalready: those files are byte-identical toorigin/mainon this branch.movingTheCursorLeavesAccessibilityAlone()the single failure out of the suite's nine.No
TableProUITestscoverage. 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.