From ac9429955374a615436e22d27d0ea85168f1a67c Mon Sep 17 00:00:00 2001 From: Steve Shreeve Date: Sat, 29 Aug 2026 13:39:03 -0600 Subject: [PATCH] fix(datagrid): overlay the inline cell editor exactly on the drawn cell --- CHANGELOG.md | 1 + TablePro/Views/Results/CellOverlayBase.swift | 70 ++++++-- .../Views/Results/CellOverlayEditor.swift | 28 ++- .../Views/Results/CellOverlayViewer.swift | 8 +- .../Results/Cells/DataGridCellRenderer.swift | 5 +- .../Cells/DataGridCellTextGeometry.swift | 39 ++++ .../DataGridCellTextGeometryTests.swift | 166 ++++++++++++++++++ 7 files changed, 300 insertions(+), 17 deletions(-) create mode 100644 TablePro/Views/Results/Cells/DataGridCellTextGeometry.swift create mode 100644 TableProTests/Views/Results/DataGridCellTextGeometryTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index c289662c0..e5f419474 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Inline cell editor opening taller than the row and shifting a single-line value instead of overlaying it. - Parse error on any MongoDB filter written in shell syntax, such as `db.orders.find({status: 1})`. - MongoDB `.sort()` and `.projection()` silently ignored when written with unquoted keys. - Compare & Sync unable to drop an overloaded PostgreSQL routine, or any trigger. diff --git a/TablePro/Views/Results/CellOverlayBase.swift b/TablePro/Views/Results/CellOverlayBase.swift index ff4bd8c92..a3d6bfb88 100644 --- a/TablePro/Views/Results/CellOverlayBase.swift +++ b/TablePro/Views/Results/CellOverlayBase.swift @@ -86,22 +86,56 @@ class CellOverlayBase: NSObject { onRemove?() } + static let maximumOverlayHeight: CGFloat = 120 + + /// A single-line value gets exactly the cell it is editing, which is what keeps the + /// glyphs from moving when the overlay opens. Only a value that actually breaks into + /// lines grows, and its height budget comes from the same geometry the text view is + /// configured with: `textContainerInset` is symmetric, so the content pays the top + /// inset twice. static func overlayFrame(for cellFrame: NSRect, value: String) -> NSRect { - let lineHeight = ThemeEngine.shared.dataGridFonts.regular.boundingRectForFont.height + 4 - var newlineCount = 0 - for scalar in value.unicodeScalars where scalar == "\n" { - newlineCount += 1 - } - let lineCount = CGFloat(newlineCount + 1) - let contentHeight = max(lineCount * lineHeight + 8, cellFrame.height) - let height = min(max(contentHeight, cellFrame.height), 120) + let breaks = lineBreakCount(in: value) + guard breaks > 0 else { return cellFrame } + + let font = ThemeEngine.shared.valueFont + let inset = DataGridCellTextGeometry.textContainerTopInset( + rowHeight: cellFrame.height, font: font + ) + let lineCount = CGFloat(breaks + 1) + let contentHeight = lineCount * DataGridCellTextGeometry.lineHeight(for: font) + 2 * inset + let height = min(max(contentHeight, cellFrame.height), maximumOverlayHeight) return NSRect(x: cellFrame.origin.x, y: cellFrame.origin.y, width: cellFrame.width, height: height) } + /// Counts the breaks TextKit lays out, not just LF: a lone CR, NEL, or a Unicode line or + /// paragraph separator each start a new line fragment, and CRLF is one break. Counting + /// only "\n" classified a "line1\rline2" value as single-line, which sized the overlay + /// to one row and hid the second line behind it. + static func lineBreakCount(in value: String) -> Int { + var count = 0 + var previousWasCarriageReturn = false + for scalar in value.unicodeScalars { + switch scalar.value { + case 0x0A: + if !previousWasCarriageReturn { count += 1 } + previousWasCarriageReturn = false + case 0x0D: + count += 1 + previousWasCarriageReturn = true + case 0x85, 0x2028, 0x2029: + count += 1 + previousWasCarriageReturn = false + default: + previousWasCarriageReturn = false + } + } + return count + } + static func makeContainer(frame: NSRect) -> CellOverlayContainerView { let container = CellOverlayContainerView(frame: frame) container.wantsLayer = true - container.layer?.borderWidth = 2 + container.layer?.borderWidth = 1 container.layer?.cornerRadius = 2 container.layer?.masksToBounds = true container.applyLayerColors() @@ -130,18 +164,32 @@ class CellOverlayBase: NSObject { textView.textContainer?.containerSize = unbounded } - static func makeScrollView(in container: NSView) -> NSScrollView { + /// A row-height overlay holding a font taller than the row would otherwise show a + /// vertical scroller and scroll its own descenders; a single-line value has nothing to + /// scroll to, so the vertical axis is shut off entirely. + static func makeScrollView(in container: NSView, scrollsVertically: Bool) -> NSScrollView { let scrollView = NSScrollView(frame: container.bounds) scrollView.autoresizingMask = [.width, .height] - scrollView.hasVerticalScroller = true + scrollView.hasVerticalScroller = scrollsVertically scrollView.hasHorizontalScroller = false scrollView.autohidesScrollers = true scrollView.borderType = .noBorder scrollView.drawsBackground = true scrollView.backgroundColor = .textBackgroundColor + if !scrollsVertically { + scrollView.verticalScrollElasticity = .none + } return scrollView } + static func configureCellTextGeometry(of textView: NSTextView, rowHeight: CGFloat, font: NSFont) { + textView.textContainer?.lineFragmentPadding = DataGridMetrics.cellHorizontalInset + textView.textContainerInset = NSSize( + width: 0, + height: DataGridCellTextGeometry.textContainerTopInset(rowHeight: rowHeight, font: font) + ) + } + private func installDismissObservers() { guard let hostTableView else { return } diff --git a/TablePro/Views/Results/CellOverlayEditor.swift b/TablePro/Views/Results/CellOverlayEditor.swift index 82f63c59d..4337d2e06 100644 --- a/TablePro/Views/Results/CellOverlayEditor.swift +++ b/TablePro/Views/Results/CellOverlayEditor.swift @@ -8,6 +8,8 @@ import AppKit @MainActor final class CellOverlayEditor: CellOverlayBase, NSTextViewDelegate { private var editorTextView: OverlayTextView? + private var editorScrollView: NSScrollView? + private var editedCellFrame: NSRect = .zero private var initialValue: String = "" var onCommit: ((_ row: Int, _ columnIndex: Int, _ newValue: String) -> Void)? @@ -27,19 +29,23 @@ final class CellOverlayEditor: CellOverlayBase, NSTextViewDelegate { guard let window = tableView.window else { return } let frame = Self.overlayFrame(for: cellFrame, value: value) + let font = ThemeEngine.shared.valueFont let containerView = Self.makeContainer(frame: frame) - let scrollView = Self.makeScrollView(in: containerView) + let scrollView = Self.makeScrollView( + in: containerView, scrollsVertically: frame.height > cellFrame.height + ) let textView = OverlayTextView(frame: scrollView.bounds) textView.overlayEditor = self textView.isEditable = true textView.isRichText = false textView.allowsUndo = true - textView.font = ThemeEngine.shared.valueFont + textView.font = font textView.textColor = .labelColor textView.backgroundColor = .textBackgroundColor textView.focusRingType = .none Self.applyCellTextLayout(to: textView) + Self.configureCellTextGeometry(of: textView, rowHeight: cellFrame.height, font: font) textView.delegate = self textView.string = value textView.selectAll(nil) @@ -49,6 +55,8 @@ final class CellOverlayEditor: CellOverlayBase, NSTextViewDelegate { initialValue = value editorTextView = textView + editorScrollView = scrollView + editedCellFrame = cellFrame install(in: tableView, row: row, column: column, columnIndex: columnIndex, container: containerView) window.makeFirstResponder(textView) @@ -66,6 +74,8 @@ final class CellOverlayEditor: CellOverlayBase, NSTextViewDelegate { let dismissColumnIndex = columnIndex editorTextView = nil + editorScrollView = nil + editedCellFrame = .zero initialValue = "" removeOverlay() @@ -74,6 +84,20 @@ final class CellOverlayEditor: CellOverlayBase, NSTextViewDelegate { } } + /// Option+Return and pasted text can turn a single-line edit into a multiline one after + /// the overlay opened, and the row-height overlay would clip the new lines with no + /// affordance that they exist. The frame follows the text, exactly as it would have been + /// framed had the value arrived that way. + func textDidChange(_ notification: Notification) { + guard let textView = editorTextView, let container = containerView else { return } + let frame = Self.overlayFrame(for: editedCellFrame, value: textView.string) + guard frame != container.frame else { return } + container.frame = frame + let grew = frame.height > editedCellFrame.height + editorScrollView?.hasVerticalScroller = grew + editorScrollView?.verticalScrollElasticity = grew ? .automatic : .none + } + func textView(_ textView: NSTextView, doCommandBy commandSelector: Selector) -> Bool { if commandSelector == #selector(NSResponder.insertNewline(_:)) { if NSApp.currentEvent?.modifierFlags.contains(.option) == true { diff --git a/TablePro/Views/Results/CellOverlayViewer.swift b/TablePro/Views/Results/CellOverlayViewer.swift index de7f8d501..cee375b8a 100644 --- a/TablePro/Views/Results/CellOverlayViewer.swift +++ b/TablePro/Views/Results/CellOverlayViewer.swift @@ -21,17 +21,21 @@ final class CellOverlayViewer: CellOverlayBase, NSTextViewDelegate { guard let window = tableView.window else { return } let frame = Self.overlayFrame(for: cellFrame, value: value) + let font = ThemeEngine.shared.valueFont let containerView = Self.makeContainer(frame: frame) - let scrollView = Self.makeScrollView(in: containerView) + let scrollView = Self.makeScrollView( + in: containerView, scrollsVertically: frame.height > cellFrame.height + ) let textView = NSTextView(frame: scrollView.bounds) textView.isEditable = false textView.isSelectable = true textView.isRichText = false - textView.font = ThemeEngine.shared.valueFont + textView.font = font textView.textColor = .labelColor textView.backgroundColor = .textBackgroundColor Self.applyCellTextLayout(to: textView) + Self.configureCellTextGeometry(of: textView, rowHeight: cellFrame.height, font: font) textView.delegate = self textView.string = value textView.selectAll(nil) diff --git a/TablePro/Views/Results/Cells/DataGridCellRenderer.swift b/TablePro/Views/Results/Cells/DataGridCellRenderer.swift index d17d8f47f..6ad8eda90 100644 --- a/TablePro/Views/Results/Cells/DataGridCellRenderer.swift +++ b/TablePro/Views/Results/Cells/DataGridCellRenderer.swift @@ -74,8 +74,9 @@ final class DataGridCellRenderer { ? (CTLineCreateTruncatedLine(fullLine, Double(availableWidth), .end, ellipsis) ?? ellipsis) : fullLine - let font = appearance.font - let baselineOffset = (rect.height - font.ascender + font.descender - font.leading) / 2 + font.ascender + let baselineOffset = DataGridCellTextGeometry.baselineY( + rowHeight: rect.height, font: appearance.font + ) context.saveGState() context.textMatrix = CGAffineTransform(scaleX: 1, y: -1) diff --git a/TablePro/Views/Results/Cells/DataGridCellTextGeometry.swift b/TablePro/Views/Results/Cells/DataGridCellTextGeometry.swift new file mode 100644 index 000000000..628ec1182 --- /dev/null +++ b/TablePro/Views/Results/Cells/DataGridCellTextGeometry.swift @@ -0,0 +1,39 @@ +// +// DataGridCellTextGeometry.swift +// TablePro +// +// The one owner of where a cell's glyphs sit, shared by the CoreText draw path and the +// overlay editor so the two cannot disagree. Before it existed each side had its own +// numbers and an inline edit visibly shifted the value it was editing. +// + +import AppKit + +@MainActor +enum DataGridCellTextGeometry { + /// Baseline questions go through a detached layout manager, never a text view's + /// `layoutManager` property: one read of that property downgrades a TextKit 2 view to + /// TextKit 1, which reverts the overlay's no-wrap layout fix (#2381). Measured: the + /// detached answer equals the TextKit 2 first-fragment glyph origin. + private static let baselineMeasurer = NSLayoutManager() + + /// The centered baseline the renderer draws at, floored to a whole point because that + /// is where TextKit puts it: measured at 1x and 2x backing, TextKit floors a rendered + /// baseline to integral points while `CTLineDraw` honors fractions. Flooring the shared + /// target is what lets the editor land on the drawn glyphs exactly at every scale. + static func baselineY(rowHeight: CGFloat, font: NSFont) -> CGFloat { + ((rowHeight - font.ascender + font.descender - font.leading) / 2 + font.ascender) + .rounded(.down) + } + + /// The symmetric `textContainerInset.height` that puts an overlay text view's first + /// baseline on `baselineY`. Negative when the font outgrows the row; AppKit accepts a + /// negative inset and parity holds. + static func textContainerTopInset(rowHeight: CGFloat, font: NSFont) -> CGFloat { + baselineY(rowHeight: rowHeight, font: font) - baselineMeasurer.defaultBaselineOffset(for: font) + } + + static func lineHeight(for font: NSFont) -> CGFloat { + baselineMeasurer.defaultLineHeight(for: font) + } +} diff --git a/TableProTests/Views/Results/DataGridCellTextGeometryTests.swift b/TableProTests/Views/Results/DataGridCellTextGeometryTests.swift new file mode 100644 index 000000000..4e377fd42 --- /dev/null +++ b/TableProTests/Views/Results/DataGridCellTextGeometryTests.swift @@ -0,0 +1,166 @@ +// +// DataGridCellTextGeometryTests.swift +// TableProTests +// + +import AppKit +import Testing + +@testable import TablePro + +/// The drawn cell and the overlay editor read their geometry from one owner, and these tests +/// are what keeps a second owner from growing back. The baseline test measures the overlay's +/// real TextKit 2 layout through `textLayoutManager`, never `layoutManager`: reading that +/// property downgrades the view to TextKit 1 and would validate an engine production never +/// runs (#2381). +@Suite("Data grid cell text geometry") +@MainActor +struct DataGridCellTextGeometryTests { + private static let rowHeights: [CGFloat] = [20, 24, 28, 32] + private static let fonts: [NSFont] = [ + .monospacedSystemFont(ofSize: 12, weight: .regular), + .systemFont(ofSize: 13), + .monospacedSystemFont(ofSize: 18, weight: .regular), + ] + + @Test("The shared baseline is the centered formula, floored to a whole point") + func baselineIsFlooredCenter() { + for font in Self.fonts { + for rowHeight in Self.rowHeights { + let fractional = (rowHeight - font.ascender + font.descender - font.leading) / 2 + + font.ascender + let shared = DataGridCellTextGeometry.baselineY(rowHeight: rowHeight, font: font) + #expect(shared == fractional.rounded(.down)) + #expect(shared == shared.rounded(.down)) + } + } + } + + @Test("A single-line value gets exactly the cell it is editing, at every row height") + func singleLineOverlayIsTheCell() { + for rowHeight in Self.rowHeights { + let cellFrame = NSRect(x: 117, y: 66, width: 140, height: rowHeight) + #expect(CellOverlayBase.overlayFrame(for: cellFrame, value: "completed") == cellFrame) + #expect(CellOverlayBase.overlayFrame(for: cellFrame, value: "") == cellFrame) + } + } + + @Test("The overlay text view puts its first baseline on the drawn baseline") + func overlayBaselineMatchesTheDrawnBaseline() throws { + for font in Self.fonts { + for rowHeight in Self.rowHeights { + let textView = NSTextView(frame: NSRect(x: 0, y: 0, width: 140, height: rowHeight)) + CellOverlayBase.applyCellTextLayout(to: textView) + CellOverlayBase.configureCellTextGeometry(of: textView, rowHeight: rowHeight, font: font) + textView.font = font + textView.string = "completed" + + let layoutManager = try #require( + textView.textLayoutManager, "the overlay has to stay on TextKit 2" + ) + layoutManager.ensureLayout(for: layoutManager.documentRange) + let fragment = try #require( + layoutManager.textLayoutFragment(for: layoutManager.documentRange.location) + ) + let line = try #require(fragment.textLineFragments.first) + + let renderedBaseline = textView.textContainerInset.height + + fragment.layoutFragmentFrame.minY + + line.glyphOrigin.y + let drawnBaseline = DataGridCellTextGeometry.baselineY(rowHeight: rowHeight, font: font) + #expect( + abs(renderedBaseline - drawnBaseline) < 0.001, + "\(font.fontName) \(font.pointSize)pt in a \(rowHeight)pt row: rendered \(renderedBaseline), drawn \(drawnBaseline)" + ) + } + } + } + + @Test("The overlay glyphs start at the drawn cell's horizontal inset") + func overlayHorizontalInsetMatchesTheRenderer() throws { + let textView = NSTextView(frame: NSRect(x: 0, y: 0, width: 140, height: 28)) + CellOverlayBase.applyCellTextLayout(to: textView) + CellOverlayBase.configureCellTextGeometry( + of: textView, rowHeight: 28, font: .systemFont(ofSize: 13) + ) + let container = try #require(textView.textContainer) + #expect(container.lineFragmentPadding == DataGridMetrics.cellHorizontalInset) + #expect(textView.textContainerInset.width == 0) + } + + /// The expectation is rebuilt from a raw `NSLayoutManager` and the written-out formula, + /// never from `DataGridCellTextGeometry`, so a drift in the shared geometry fails here + /// instead of being copied into the expectation. + @Test("A multiline value grows by the geometry the text view is configured with") + func multilineHeightComesFromTheSharedGeometry() { + let cellFrame = NSRect(x: 0, y: 0, width: 140, height: 32) + let font = ThemeEngine.shared.valueFont + let measurer = NSLayoutManager() + let flooredBaseline = ((32 - font.ascender + font.descender - font.leading) / 2 + + font.ascender).rounded(.down) + let inset = flooredBaseline - measurer.defaultBaselineOffset(for: font) + let expected = min(max(2 * measurer.defaultLineHeight(for: font) + 2 * inset, 32), 120) + + let twoLines = CellOverlayBase.overlayFrame(for: cellFrame, value: "a\nb") + #expect(twoLines.height == expected) + #expect(twoLines.height > cellFrame.height) + #expect(twoLines.origin == cellFrame.origin) + #expect(twoLines.width == cellFrame.width) + + let manyLines = CellOverlayBase.overlayFrame( + for: cellFrame, value: Array(repeating: "x", count: 200).joined(separator: "\n") + ) + #expect(manyLines.height == CellOverlayBase.maximumOverlayHeight) + } + + /// TextKit starts a new line fragment on LF, lone CR, NEL and the Unicode line and + /// paragraph separators, and treats CRLF as one break. Counting only LF classified a + /// "line1\rline2" value as single-line and hid its second line behind a one-row overlay. + @Test("Line breaks are counted the way TextKit lays them out") + func lineBreaksCountLikeTextKit() { + #expect(CellOverlayBase.lineBreakCount(in: "one line") == 0) + #expect(CellOverlayBase.lineBreakCount(in: "") == 0) + #expect(CellOverlayBase.lineBreakCount(in: "a\nb") == 1) + #expect(CellOverlayBase.lineBreakCount(in: "a\rb") == 1) + #expect(CellOverlayBase.lineBreakCount(in: "a\r\nb") == 1) + #expect(CellOverlayBase.lineBreakCount(in: "a\u{85}b") == 1) + #expect(CellOverlayBase.lineBreakCount(in: "a\u{2028}b") == 1) + #expect(CellOverlayBase.lineBreakCount(in: "a\u{2029}b") == 1) + #expect(CellOverlayBase.lineBreakCount(in: "a\r\n\r\nb") == 2) + #expect(CellOverlayBase.lineBreakCount(in: "a\r\rb") == 2) + + let cellFrame = NSRect(x: 0, y: 0, width: 140, height: 28) + #expect(CellOverlayBase.overlayFrame(for: cellFrame, value: "line1\rline2") != cellFrame) + #expect(CellOverlayBase.overlayFrame(for: cellFrame, value: "a\u{2028}b") != cellFrame) + } + + /// The overlay frames itself from `frameOfCell` while the renderer draws into + /// `rect(ofColumn:)`; this pins the measured fact that their x origins agree on every + /// column a user can edit (attached index 0 is the row-number column, never editable). + @Test("frameOfCell and rect(ofColumn:) agree on x for editable columns") + func overlayRectSourceMatchesTheRendererRectSource() { + let tableView = NSTableView(frame: NSRect(x: 0, y: 0, width: 400, height: 200)) + tableView.intercellSpacing = NSSize(width: 1, height: 0) + tableView.rowHeight = 28 + for i in 0..<3 { + let column = NSTableColumn(identifier: .init("c\(i)")) + column.width = 100 + tableView.addTableColumn(column) + } + let dataSource = FixedRowCountDataSource() + tableView.dataSource = dataSource + tableView.reloadData() + tableView.layoutSubtreeIfNeeded() + + for column in 1..<3 { + let cellFrame = tableView.frameOfCell(atColumn: column, row: 1) + let columnRect = tableView.rect(ofColumn: column) + #expect(cellFrame.minX == columnRect.minX) + #expect(cellFrame.height == tableView.rect(ofRow: 1).height) + } + } +} + +private final class FixedRowCountDataSource: NSObject, NSTableViewDataSource { + func numberOfRows(in tableView: NSTableView) -> Int { 5 } +}