diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f048234df..ea2ed5ff49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - VoiceOver reading the welcome window and Integrations filter menus as "chevron.pulldown". (#3250) - Database icon filled in the database switcher and query editor, outline in the toolbar and sidebar. (#3250) - Status bar buttons a point or two taller or shorter than each other depending on their icon. (#3250) +- Highlighted row in the database and connection switchers drawn as white text on a grey fill. (#3249) ## [0.77.1] - 2026-10-03 diff --git a/TablePro/Views/Shared/FieldDrivenList.swift b/TablePro/Views/Shared/FieldDrivenList.swift index 1a0e3e5d51..24dd13fccf 100644 --- a/TablePro/Views/Shared/FieldDrivenList.swift +++ b/TablePro/Views/Shared/FieldDrivenList.swift @@ -323,7 +323,7 @@ internal struct FieldDrivenList: NSViewRepresenta /// A chooser's highlight stands for the search field's selection, so it draws emphasized whenever /// the window is key: the field is the thing holding focus. A browser owns its own focus, so AppKit -/// already emphasizes it exactly right and this row leaves the property alone. +/// already emphasizes it exactly right and this row passes its value through. internal final class FieldDrivenRowView: NSTableRowView { internal static let reuseIdentifier = NSUserInterfaceItemIdentifier("FieldDrivenRow") @@ -336,22 +336,59 @@ internal final class FieldDrivenRowView: NSTableRowView { return view } - /// The setter has to forward, because AppKit's own stored value is what a browser row draws - /// from and swallowing the write would leave every browser row permanently unemphasized. + /// The chooser rule sits in the setter, so the value AppKit stores is the value the row draws. + /// In a popover and in a source list the selection fill is a material view, and AppKit + /// configures it only when that stored value changes. A rule answered from the getter left + /// the fill at whatever it read while the row had no window: grey, under cells that had since + /// turned white. The table also rewrites the value on every key and first-responder change, + /// with false for a table that does not hold focus, and substituting here is what keeps that + /// write from switching the highlight off. A browser row forwards, because AppKit's value is + /// already the right one. override internal var isEmphasized: Bool { - get { followsWindowKeyState ? window?.isKeyWindow ?? false : super.isEmphasized } - set { super.isEmphasized = newValue } + get { super.isEmphasized } + set { super.isEmphasized = followsWindowKeyState ? windowHoldsKeyboard : newValue } } - /// AppKit copies `interiorBackgroundStyle` into the cell views from `didAddSubview`, and a row - /// is populated before it is added to the table, so at that moment `window` is still nil and a - /// key-state-derived emphasis reads false. The row then paints its accent fill from the live - /// value while the cells keep the unemphasized foreground: blue fill, dark text, until some - /// later selection change happens to re-run the copy. Repeating it here is the first point the - /// derived value is true, and AppKit keeps the two in step from then on. + private var windowHoldsKeyboard: Bool { window?.isKeyWindow ?? false } + + /// A row is selected and populated before it is added to the table, so its window is still nil + /// and this is the first point the rule can read true. The key notifications are observed for + /// every window, because a popover's window posts none of its own: it reports its parent's key + /// state, and the parent is the object of the notification. A popover that opens with its rows + /// already inside posts nothing at all, and there the table's own write is what reaches the + /// setter. override internal func viewDidMoveToWindow() { super.viewDidMoveToWindow() + let center = NotificationCenter.default + center.removeObserver(self, name: NSWindow.didBecomeKeyNotification, object: nil) + center.removeObserver(self, name: NSWindow.didResignKeyNotification, object: nil) guard followsWindowKeyState else { return } + if window != nil { + center.addObserver( + self, + selector: #selector(windowKeyStateDidChange(_:)), + name: NSWindow.didBecomeKeyNotification, + object: nil + ) + center.addObserver( + self, + selector: #selector(windowKeyStateDidChange(_:)), + name: NSWindow.didResignKeyNotification, + object: nil + ) + } + syncEmphasisWithWindow() + } + + @objc private func windowKeyStateDidChange(_ notification: Notification) { + syncEmphasisWithWindow() + } + + /// AppKit's setter repaints the fill, but it restyles only the cells the table registered, and + /// on one path not before the next layout. Copying the style here moves the fill and the + /// content in the same call. + private func syncEmphasisWithWindow() { + isEmphasized = windowHoldsKeyboard let style = interiorBackgroundStyle for case let cell as NSTableCellView in subviews where cell.backgroundStyle != style { cell.backgroundStyle = style diff --git a/TableProTests/Views/FieldDrivenListEmphasisTests.swift b/TableProTests/Views/FieldDrivenListEmphasisTests.swift index 81db7d2be5..f989222775 100644 --- a/TableProTests/Views/FieldDrivenListEmphasisTests.swift +++ b/TableProTests/Views/FieldDrivenListEmphasisTests.swift @@ -12,15 +12,42 @@ struct FieldDrivenListEmphasisTests { /// The headless test host never gives a window the keyboard, so the one input the chooser rule /// reads is stated here instead of hoping for real focus. private final class KeyWindow: NSWindow { - override var isKeyWindow: Bool { true } + var holdsKeyboard = true + + override var isKeyWindow: Bool { holdsKeyboard } + } + + @MainActor + private final class ChooserTableSource: NSObject, NSTableViewDataSource, NSTableViewDelegate { + func numberOfRows(in tableView: NSTableView) -> Int { 3 } + + func tableView(_ tableView: NSTableView, viewFor tableColumn: NSTableColumn?, row: Int) -> NSView? { + NSTableCellView(frame: NSRect(x: 0, y: 0, width: 280, height: 28)) + } + + func tableView(_ tableView: NSTableView, rowViewForRow row: Int) -> NSTableRowView? { + let rowView = FieldDrivenRowView.make() + rowView.followsWindowKeyState = true + return rowView + } + } + + private static let windowFrame = NSRect(x: 0, y: 0, width: 300, height: 200) + + private func makeKeyWindow() -> KeyWindow { + let window = KeyWindow( + contentRect: Self.windowFrame, styleMask: [.titled], backing: .buffered, defer: false + ) + window.contentView = NSView(frame: Self.windowFrame) + return window } private func makeWindow(isKeyWindow: Bool) -> NSWindow { - let frame = NSRect(x: 0, y: 0, width: 300, height: 200) - let window = isKeyWindow - ? KeyWindow(contentRect: frame, styleMask: [.titled], backing: .buffered, defer: false) - : NSWindow(contentRect: frame, styleMask: [.titled], backing: .buffered, defer: false) - window.contentView = NSView(frame: frame) + guard !isKeyWindow else { return makeKeyWindow() } + let window = NSWindow( + contentRect: Self.windowFrame, styleMask: [.titled], backing: .buffered, defer: false + ) + window.contentView = NSView(frame: Self.windowFrame) return window } @@ -35,6 +62,97 @@ struct FieldDrivenListEmphasisTests { return (rowView, cell) } + /// The two shapes that make AppKit draw a row's selection with a material: a source list + /// anywhere, and an inset table with a material behind it, which is what a popover is. + enum MaterialShape { + case sourceList + case insetOverMaterial + } + + @MainActor + private struct StagedChooser { + let source: ChooserTableSource + let host: NSView + let tableView: FieldDrivenTableView + + var selectedRowView: NSTableRowView? { + tableView.rowView(atRow: 1, makeIfNecessary: false) + } + + var selectionMaterial: NSVisualEffectView? { + selectedRowView?.subviews + .compactMap { $0 as? NSVisualEffectView } + .first { $0.material == .selection } + } + } + + /// The list selects its row from a SwiftUI update, before the table has a window, and the + /// layout pass is what builds the row there. Without it no row exists until the table is in + /// the window, and the order the bug needs never happens. + private static func stageChooserSelectedOutsideAWindow(_ shape: MaterialShape) -> StagedChooser { + let source = ChooserTableSource() + let tableView = FieldDrivenTableView() + tableView.headerView = nil + tableView.rowHeight = 28 + tableView.backgroundColor = .clear + tableView.style = shape == .sourceList ? .sourceList : .inset + let column = NSTableColumn(identifier: NSUserInterfaceItemIdentifier("FieldDrivenColumn")) + column.resizingMask = .autoresizingMask + tableView.addTableColumn(column) + tableView.dataSource = source + tableView.delegate = source + + let scrollView = NSScrollView(frame: windowFrame) + scrollView.documentView = tableView + scrollView.drawsBackground = false + var host: NSView = scrollView + if shape == .insetOverMaterial { + let backdrop = NSVisualEffectView(frame: windowFrame) + backdrop.material = .popover + backdrop.addSubview(scrollView) + host = backdrop + } + + tableView.reloadData() + tableView.selectRowIndexes(IndexSet(integer: 1), byExtendingSelection: false) + tableView.scrollRowToVisible(1) + host.layoutSubtreeIfNeeded() + return StagedChooser(source: source, host: host, tableView: tableView) + } + + /// On the macOS 26 CI runner a row selected and laid out outside a window has no selection + /// material, so the order the material tests stage cannot happen there. Asked of the host + /// rather than read off an OS version, because nothing documents which hosts build it early. + static func buildsSelectionMaterialOutsideAWindow(_ shape: MaterialShape) -> Bool { + stageChooserSelectedOutsideAWindow(shape).selectionMaterial != nil + } + + /// What AppKit stored, read through `NSTableRowView`'s own getter so a row that answers from + /// an override cannot stand in for it. The selection fill follows this value on every host. + private func storedEmphasis(of rowView: NSTableRowView) -> Bool { + typealias Getter = @convention(c) (NSTableRowView, Selector) -> Bool + let selector = #selector(getter: NSTableRowView.isEmphasized) + guard let implementation = class_getMethodImplementation(NSTableRowView.self, selector) else { + return false + } + return unsafeBitCast(implementation, to: Getter.self)(rowView, selector) + } + + private func expectEmphasizedSelectionMaterial(_ shape: MaterialShape) throws { + let chooser = Self.stageChooserSelectedOutsideAWindow(shape) + let rowView = try #require(chooser.selectedRowView) + let material = try #require(chooser.selectionMaterial) + #expect(material.isEmphasized == false) + + let window = makeWindow(isKeyWindow: true) + window.contentView?.addSubview(chooser.host) + window.contentView?.layoutSubtreeIfNeeded() + + #expect(chooser.selectedRowView === rowView) + #expect(rowView.isEmphasized) + #expect(material.isEmphasized) + } + /// The regression. AppKit copies `interiorBackgroundStyle` into the cell views while the row is /// still outside the window, where a key-state-derived emphasis can only read false, and it /// never repeats the copy when the row arrives. The row then painted its accent fill from the @@ -49,6 +167,53 @@ struct FieldDrivenListEmphasisTests { #expect(cell.backgroundStyle == .emphasized) } + /// The order the bug lived in, checked on every host: selected outside a window, then added to + /// a key one, with no key change after it. A rule answered from the getter stored nothing. + @Test("A chooser row that joins a key window stores its emphasis where AppKit reads it") + func chooserStoresItsEmphasis() { + let (rowView, _) = makeRow(followsWindowKeyState: true, isSelected: true) + #expect(storedEmphasis(of: rowView) == false) + + makeWindow(isKeyWindow: true).contentView?.addSubview(rowView) + + #expect(storedEmphasis(of: rowView)) + } + + /// The gate on the material tests reads a missing material as a host difference. A stage that + /// built no row has no material either, so the row is checked here, on every host, where the + /// gate cannot hide it. + @Test("The material tests' stage builds the selected row before it reaches a window") + func stageBuildsTheSelectedRowOutsideAWindow() { + let sourceList = Self.stageChooserSelectedOutsideAWindow(.sourceList) + let overMaterial = Self.stageChooserSelectedOutsideAWindow(.insetOverMaterial) + #expect(sourceList.selectedRowView != nil) + #expect(overMaterial.selectedRowView != nil) + } + + /// The other half of the same regression. AppKit configures the selection material only when + /// the selection, the stored emphasis or the key state changes. None of the three follows a + /// row that was selected outside the window, so the material kept the unemphasized look it was + /// built with, under cells that had turned white. + @Test( + "A source list row selected before it reaches the window emphasizes its selection material", + .enabled("The host builds no selection material outside a window") { + await FieldDrivenListEmphasisTests.buildsSelectionMaterialOutsideAWindow(.sourceList) + } + ) + func sourceListChooserEmphasizesTheSelectionMaterial() throws { + try expectEmphasizedSelectionMaterial(.sourceList) + } + + @Test( + "A popover row selected before it reaches the window emphasizes its selection material", + .enabled("The host builds no selection material outside a window") { + await FieldDrivenListEmphasisTests.buildsSelectionMaterialOutsideAWindow(.insetOverMaterial) + } + ) + func popoverChooserEmphasizesTheSelectionMaterial() throws { + try expectEmphasizedSelectionMaterial(.insetOverMaterial) + } + @Test("A chooser row leaves an unselected row's cells alone") func chooserLeavesUnselectedCellsAlone() { let (rowView, cell) = makeRow(followsWindowKeyState: true, isSelected: false) @@ -79,6 +244,60 @@ struct FieldDrivenListEmphasisTests { #expect(rowView.interiorBackgroundStyle == .emphasized) } + @Test("A chooser row follows its window when the keyboard leaves and comes back") + func chooserFollowsKeyStateChanges() { + let (rowView, cell) = makeRow(followsWindowKeyState: true, isSelected: true) + let window = makeKeyWindow() + window.contentView?.addSubview(rowView) + + window.holdsKeyboard = false + NotificationCenter.default.post(name: NSWindow.didResignKeyNotification, object: window) + + #expect(rowView.isEmphasized == false) + #expect(cell.backgroundStyle == .normal) + + window.holdsKeyboard = true + NotificationCenter.default.post(name: NSWindow.didBecomeKeyNotification, object: window) + + #expect(rowView.isEmphasized) + #expect(cell.backgroundStyle == .emphasized) + } + + /// The popover shape. A popover's window posts no key notification of its own: it reports its + /// parent's key state, and the parent is the object of the notification. + @Test("A chooser row follows a key change that another window posts") + func chooserFollowsAKeyChangePostedByAnotherWindow() { + let (rowView, cell) = makeRow(followsWindowKeyState: true, isSelected: true) + let window = makeKeyWindow() + window.contentView?.addSubview(rowView) + let parent = makeWindow(isKeyWindow: false) + + window.holdsKeyboard = false + NotificationCenter.default.post(name: NSWindow.didResignKeyNotification, object: parent) + + #expect(rowView.isEmphasized == false) + #expect(cell.backgroundStyle == .normal) + } + + /// A row waits in the reuse queue with no window, and must not carry the last window's + /// emphasis into the next one. + @Test("A chooser row drops its emphasis when it leaves the window") + func chooserDropsEmphasisOutsideAWindow() { + let (rowView, cell) = makeRow(followsWindowKeyState: true, isSelected: true) + let window = makeKeyWindow() + window.contentView?.addSubview(rowView) + #expect(rowView.isEmphasized) + + rowView.removeFromSuperview() + + #expect(rowView.isEmphasized == false) + #expect(cell.backgroundStyle == .normal) + + NotificationCenter.default.post(name: NSWindow.didBecomeKeyNotification, object: window) + + #expect(rowView.isEmphasized == false) + } + /// A browser holds its own focus, so AppKit's stored value is the right answer and the setter /// has to forward. Swallowing the write would leave every row of the query history drawer /// permanently unemphasized. @@ -97,9 +316,21 @@ struct FieldDrivenListEmphasisTests { #expect(rowView.isEmphasized == false) } - /// A chooser answers from the window, not from what AppKit stored, because AppKit drops - /// emphasis the moment the search field rather than the table holds the keyboard. - @Test("A chooser row ignores the emphasis AppKit stored") + @Test("A browser row leaves key changes to AppKit") + func browserIgnoresKeyChanges() { + let (rowView, cell) = makeRow(followsWindowKeyState: false, isSelected: true) + let window = makeKeyWindow() + window.contentView?.addSubview(rowView) + + NotificationCenter.default.post(name: NSWindow.didBecomeKeyNotification, object: window) + + #expect(rowView.isEmphasized == false) + #expect(cell.backgroundStyle == .normal) + } + + /// A chooser answers from the window, not from what AppKit hands it, because AppKit writes + /// false the moment the search field rather than the table holds the keyboard. + @Test("A chooser row outside a window refuses the emphasis AppKit hands it") func chooserIgnoresStoredEmphasis() { let rowView = FieldDrivenRowView.make() rowView.followsWindowKeyState = true @@ -108,4 +339,17 @@ struct FieldDrivenListEmphasisTests { #expect(rowView.isEmphasized == false) } + + /// The table rewrites every row on each key and first-responder change, always with false for + /// a table that does not hold focus. In a key window that write must not land. + @Test("A chooser row in a key window keeps its emphasis through AppKit's own write") + func chooserKeepsEmphasisThroughAppKitWrites() { + let (rowView, cell) = makeRow(followsWindowKeyState: true, isSelected: true) + makeWindow(isKeyWindow: true).contentView?.addSubview(rowView) + + rowView.isEmphasized = false + + #expect(rowView.isEmphasized) + #expect(cell.backgroundStyle == .emphasized) + } }