fix(datagrid): overlay the inline cell editor exactly on the drawn cell - #2579
Open
shreeve wants to merge 1 commit into
Open
fix(datagrid): overlay the inline cell editor exactly on the drawn cell#2579shreeve wants to merge 1 commit into
shreeve wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 关于 Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
注册 for free
to join this conversation on GitHub.
Already have an account?
登录 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.
Problem
Opening an inline edit on a data-grid cell visibly shifted the value it was editing. The editor overlay was taller than the row at every row-height setting (spilling over the row below), its text sat at the top of the overlay while the drawn cell centers its baseline in the row, and its glyphs started 1pt right of the drawn text. Entering edit mode should not move the glyphs at all.
Root cause
Two unrelated geometry owners for the same glyphs. The CoreText renderer (
DataGridCellRenderer) draws atx = minX + DataGridMetrics.cellHorizontalInsetwith the baseline centered in the row. The overlay (CellOverlayBase) sized itself fromboundingRectForFont.height + 12, anchored at the cell top, with anNSTextViewleft on its default 5ptlineFragmentPaddingand zerotextContainerInset. Nothing shared a constant, so height, baseline and x inset all disagreed.Measured on macOS 15 (mono 12pt, 28pt row): overlay 30.33pt tall in a 28pt row, first baseline at 12.0 (top-aligned) vs 18.54 drawn, glyph x 5 vs 4.
Fix
One geometry owner,
DataGridCellTextGeometry, consumed by both sides:baselineY(rowHeight:font:): the centered-baseline formula, floored to a whole point. Measured at 1x and 2x backing: TextKit floors rendered baselines to integral points at every scale whileCTLineDrawhonors fractions, so the drawn side adopts the floor (moves at most 0.5pt, once, uniformly) and the editor lands on it exactly.textContainerTopInset(rowHeight:font:):baselineY - defaultBaselineOffset(for: font), computed from a detachedNSLayoutManager. Reading the text view's ownlayoutManagerwould downgrade it to TextKit 1 and revert the no-wrap overlay layout (Data grid breaks with many columns: flickering, columns stop rendering, horizontal scroll lags behind viewport #2381), so neither the code nor the tests touch it; the parity test measures throughtextLayoutManager.A single-line value's overlay is now exactly the cell rect, with the vertical scroller and elasticity off (a grid font taller than the row would otherwise scroll its own descenders). A value that breaks into lines still grows, capped at 120pt, with its height derived from the same geometry (
textContainerInsetis symmetric, so the content pays the top inset twice). Line breaks are counted the way TextKit lays them out (LF, lone CR, NEL, U+2028, U+2029, CRLF as one), and an edit that becomes multiline after opening (Option+Return, paste) reframes the overlay and re-arms the scroller. The border slimmed from 2pt to a 1px stroke; a layer border paints over edge pixels and cannot displace glyphs. The read-onlyCellOverlayViewershares all of it.Glyph-origin parity was probe-verified at delta 0.000 across three fonts before implementation, and
frameOfCell(atColumn:row:)vsrect(ofColumn:)was measured to agree on x for every editable column.Tests
DataGridCellTextGeometryTests(new):NSTextLineFragment.glyphOrigin), all four row heights x three fonts, including a font taller than the 20pt row.NSLayoutManagerand the written-out formula, so a drift in the shared geometry fails the test instead of being copied into the expectation.All suites owning the touched types pass (128 tests):
DataGridCellTextGeometryTests,CellOverlayTextLayoutTests,CellOverlayAppearanceTests,DataGridCellAccessoryAppearanceTests,KeyHandlingTableViewOverlayTests,TableViewCoordinatorLayoutTests,ValueFontTests.The open-editor-and-type flow itself is not UI-automated: the overlay editor requires a live connection and grid focus, which does not run deterministically on the CI runner (the drawn grid publishes cells XCUITest refuses to click; see the accessibility invariant). The geometry it exercises is covered by the unit suites above.
Before / After
Before: opening an edit on the
statuscell of row 4 shows a bordered box 1.5 rows tall covering the top of row 5, with the value shifted up and left.After: the value does not move; the only change is a 1px focus-colored stroke around the cell.
Screenshots to be attached as a comment.