From ad412ec5edf6cac909c22c5cfd0d62d43d392917 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Fri, 16 Oct 2026 23:39:51 +0700 Subject: [PATCH 1/5] fix(settings): let the Themes list scroll and take keyboard focus --- .../Appearance/ThemeLibrarySection.swift | 21 ++++-- .../Settings/AppearanceSettingsView.swift | 13 +++- .../Settings/AppearancePaneListTests.swift | 66 +++++++++++++++++++ .../AppearanceSettingsUITests.swift | 57 ++++++++-------- 4 files changed, 122 insertions(+), 35 deletions(-) create mode 100644 TableProTests/Views/Settings/AppearancePaneListTests.swift diff --git a/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift b/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift index ac41ca7b59..1124d9ebd7 100644 --- a/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift +++ b/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift @@ -29,11 +29,17 @@ internal struct ThemeLibrarySection: View { let fileName: String } - // Fits the Settings pane's default 500 pt height under the two sections above it. - private static let listHeight: CGFloat = 176 + private static let minimumListHeight: CGFloat = 160 + /// A grouped Form centers its sections at most this wide (measured on macOS 27), so the list keeps + /// the edges of the sections above it. + private static let formContentWidth: CGFloat = 704 + private static let formMargin: CGFloat = 20 internal var body: some View { - Section { + VStack(alignment: .leading, spacing: 8) { + Text("Themes") + .font(.headline) + .accessibilityAddTraits(.isHeader) list .alert( model.alert?.title ?? "", @@ -66,9 +72,11 @@ internal struct ThemeLibrarySection: View { contentType: .json, defaultFilename: exporting?.fileName ) { _ in } - } header: { - Text("Themes") } + .frame(maxWidth: Self.formContentWidth) + .padding(.horizontal, Self.formMargin) + .padding(.bottom, Self.formMargin) + .frame(maxWidth: .infinity) } // MARK: - List @@ -110,7 +118,8 @@ internal struct ThemeLibrarySection: View { } } .listStyle(.bordered(alternatesRowBackgrounds: false)) - .frame(height: Self.listHeight) + .frame(minHeight: Self.minimumListHeight, maxHeight: .infinity) + .accessibilityIdentifier("appearance-themes-list") .contextMenu(forSelectionType: ThemeLibraryItem.self) { items in rowMenu(for: items.first) } primaryAction: { items in diff --git a/TablePro/Views/Settings/AppearanceSettingsView.swift b/TablePro/Views/Settings/AppearanceSettingsView.swift index 37d750ee7c..504099bb28 100644 --- a/TablePro/Views/Settings/AppearanceSettingsView.swift +++ b/TablePro/Views/Settings/AppearanceSettingsView.swift @@ -10,6 +10,15 @@ internal struct AppearanceSettingsView: View { private static let modes: [AppAppearanceMode] = [.auto, .light, .dark] internal var body: some View { + VStack(spacing: 0) { + settingsForm + ThemeLibrarySection() + } + } + + // A List inside a scrolling grouped Form neither scrolls nor takes keyboard focus (measured on + // macOS 27), so the Form keeps its own height and the theme list sits below it. + private var settingsForm: some View { Form { Section { Picker(String(localized: "Appearance"), selection: $settingsManager.appearanceMode) { @@ -28,10 +37,10 @@ internal struct AppearanceSettingsView: View { } footer: { Text("Themes color the editor, the data grid, other content views and the window background. Controls, sidebars and Settings follow the system appearance.") } - - ThemeLibrarySection() } .formStyle(.grouped) + .scrollDisabled(true) + .fixedSize(horizontal: false, vertical: true) } private static func title(for mode: AppAppearanceMode) -> String { diff --git a/TableProTests/Views/Settings/AppearancePaneListTests.swift b/TableProTests/Views/Settings/AppearancePaneListTests.swift new file mode 100644 index 0000000000..153309e167 --- /dev/null +++ b/TableProTests/Views/Settings/AppearancePaneListTests.swift @@ -0,0 +1,66 @@ +import AppKit +@testable import TablePro +import SwiftUI +import Testing + +/// A List inside the pane's scrolling grouped Form had its scrolling turned off by SwiftUI, so the +/// themes below the first few rows could never be reached. +@MainActor +struct AppearancePaneListTests { + private static func find(_ type: T.Type, in view: NSView) -> [T] { + var found: [T] = [] + var pending = [view] + while let next = pending.popLast() { + if let match = next as? T { found.append(match) } + pending.append(contentsOf: next.subviews) + } + return found + } + + private static func scrollEvent(over view: NSView, in window: NSWindow) throws -> NSEvent { + let event = try #require(CGEvent( + scrollWheelEvent2Source: nil, + units: .pixel, + wheelCount: 1, + wheel1: -80, + wheel2: 0, + wheel3: 0 + )) + let center = view.convert(NSPoint(x: view.bounds.midX, y: view.bounds.midY), to: nil) + let screenPoint = window.convertPoint(toScreen: center) + let screenHeight = NSScreen.screens.first?.frame.height ?? 0 + event.location = CGPoint(x: screenPoint.x, y: screenHeight - screenPoint.y) + return try #require(NSEvent(cgEvent: event)) + } + + @Test("The Themes list scrolls on its own when it holds more themes than it shows") + func themesListScrolls() throws { + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 640, height: 520), + styleMask: [.titled], + backing: .buffered, + defer: false + ) + window.isReleasedWhenClosed = false + window.contentViewController = NSHostingController(rootView: AppearanceSettingsView(settings: .constant(.default))) + window.setContentSize(NSSize(width: 640, height: 520)) + window.layoutIfNeeded() + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.3)) + defer { window.orderOut(nil) } + + let content = try #require(window.contentView) + let table = try #require( + Self.find(NSTableView.self, in: content).first { $0.numberOfRows > 10 }, + "The pane should show the Themes list" + ) + let scrollView = try #require(table.enclosingScrollView) + #expect(table.frame.height > scrollView.contentView.bounds.height, "The fixture needs more themes than fit") + + let before = scrollView.contentView.bounds.origin.y + scrollView.scrollWheel(with: try Self.scrollEvent(over: scrollView, in: window)) + RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.2)) + + #expect(scrollView.contentView.bounds.origin.y > before, "A scroll over the list should move its rows") + #expect(scrollView.verticalScrollElasticity != .none) + } +} diff --git a/TableProUITests/AppearanceSettingsUITests.swift b/TableProUITests/AppearanceSettingsUITests.swift index 0834c4fcab..9d12e11c05 100644 --- a/TableProUITests/AppearanceSettingsUITests.swift +++ b/TableProUITests/AppearanceSettingsUITests.swift @@ -52,12 +52,37 @@ final class AppearanceSettingsUITests: UITestCase { ) } + func testThemesListScrollsAndTakesArrowKeys() throws { + let app = try launchOpeningURL("tablepro://settings/appearance") + let settings = try appearancePane(in: app) + let list = settings.outlines["appearance-themes-list"] + XCTAssertTrue(list.waitToExist(timeout: 10), "The Themes list should be on the Appearance pane") + + let last = row("Tokyo Night Light", in: list) + XCTAssertTrue(last.waitToExist(timeout: 10)) + let deadline = Date(timeIntervalSinceNow: 10) + while last.frame.maxY > list.frame.maxY, Date() < deadline { + list.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).scroll(byDeltaX: 0, deltaY: -120) + } + XCTAssertLessThanOrEqual(last.frame.maxY, list.frame.maxY, "A scroll over the list should bring its last theme into view") + + let first = outlineRow("Tokyo Night", in: list) + let next = outlineRow("Tokyo Night Light", in: list) + XCTAssertTrue(waitUntilHittable(first, timeout: 5)) + first.click() + XCTAssertTrue(waitForPredicate(timeout: 5) { first.isSelected }, "A click should select the theme") + settings.typeKey(.downArrow, modifierFlags: []) + XCTAssertTrue( + waitForPredicate(timeout: 5) { next.isSelected && !first.isSelected }, + "Down Arrow should move the selection, so the click must have given the list keyboard focus" + ) + } + // MARK: - Steps private func createTheme(basedOn source: String, in window: XCUIElement) { let add = element("appearance-theme-add", in: window) XCTAssertTrue(add.waitToExist(timeout: 10), "The Themes list should offer its add menu") - revealInForm(add, in: window) XCTAssertTrue(waitUntilHittable(add, timeout: 10), "The Themes list should offer its add menu") XCTAssertTrue( add.label == "Add Theme" || add.title == "Add Theme", @@ -108,7 +133,6 @@ final class AppearanceSettingsUITests: UITestCase { let actions = element("appearance-theme-actions", in: window) XCTAssertTrue(actions.waitToExist(timeout: 10)) - revealInForm(actions, in: window) XCTAssertTrue(waitUntilHittable(actions, timeout: 10)) XCTAssertTrue( actions.label == "Theme Actions" || actions.title == "Theme Actions", @@ -134,7 +158,6 @@ final class AppearanceSettingsUITests: UITestCase { themeRow.click() let remove = window.buttons["appearance-theme-remove"] XCTAssertTrue(remove.waitToExist(timeout: 5)) - revealInForm(remove, in: window) XCTAssertEqual(remove.label, "Move to Trash") XCTAssertTrue(waitForPredicate(timeout: 5) { remove.isEnabled }, "A selected custom theme can be trashed") remove.click() @@ -157,30 +180,6 @@ final class AppearanceSettingsUITests: UITestCase { return settings } - /// The Settings pane scrolls over its top sections, because a scroll over the theme list - /// scrolls the list instead. - private func revealInForm(_ control: XCUIElement, in window: XCUIElement) { - let form = window.scrollViews.firstMatch - XCTAssertTrue(form.waitToExist(timeout: 5)) - reveal(control, in: form, scrollingAt: CGVector(dx: 0.5, dy: 0.05)) - } - - /// A control clipped by its scroll view still reads as hittable, and a click on it lands on - /// whatever is drawn there, so the frames decide when it is in view. - private func reveal(_ control: XCUIElement, in scrollView: XCUIElement, scrollingAt point: CGVector) { - let deadline = Date(timeIntervalSinceNow: 10) - while control.frame.maxY > scrollView.frame.maxY || control.frame.minY < scrollView.frame.minY, Date() < deadline { - let overshoot = control.frame.maxY > scrollView.frame.maxY - ? scrollView.frame.maxY - control.frame.maxY - 20 - : scrollView.frame.minY - control.frame.minY + 20 - scrollView.coordinate(withNormalizedOffset: point).scroll(byDeltaX: 0, deltaY: overshoot) - } - XCTAssertTrue( - control.frame.minY >= scrollView.frame.minY && control.frame.maxY <= scrollView.frame.maxY, - "\(control.identifier) should scroll into view" - ) - } - private func choose(_ title: String, in popup: XCUIElement) { XCTAssertTrue(waitUntilHittable(popup, timeout: 10)) popup.click() @@ -198,6 +197,10 @@ final class AppearanceSettingsUITests: UITestCase { window.staticTexts.matching(NSPredicate(format: "value == %@", name)).firstMatch } + private func outlineRow(_ name: String, in list: XCUIElement) -> XCUIElement { + list.outlineRows.containing(NSPredicate(format: "value == %@", name)).firstMatch + } + private func themeDocument(named name: String) -> [String: Any]? { guard let root = sandboxRoot else { return nil } let directory = root.appendingPathComponent("TablePro/Themes", isDirectory: true) From 2c514594d53ad27724e5c61a849121b0b8d4593b Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 00:01:50 +0700 Subject: [PATCH 2/5] test(settings): host the Appearance pane the way the Settings window does --- .../Views/Settings/AppearancePaneListTests.swift | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/TableProTests/Views/Settings/AppearancePaneListTests.swift b/TableProTests/Views/Settings/AppearancePaneListTests.swift index 153309e167..1ec4be82b4 100644 --- a/TableProTests/Views/Settings/AppearancePaneListTests.swift +++ b/TableProTests/Views/Settings/AppearancePaneListTests.swift @@ -35,15 +35,19 @@ struct AppearancePaneListTests { @Test("The Themes list scrolls on its own when it holds more themes than it shows") func themesListScrolls() throws { + let paneSize = SettingsPaneTabViewController.paneSize let window = NSWindow( - contentRect: NSRect(x: 0, y: 0, width: 640, height: 520), + contentRect: NSRect(origin: .zero, size: paneSize), styleMask: [.titled], backing: .buffered, defer: false ) window.isReleasedWhenClosed = false - window.contentViewController = NSHostingController(rootView: AppearanceSettingsView(settings: .constant(.default))) - window.setContentSize(NSSize(width: 640, height: 520)) + // As the Settings window hosts a pane: the window keeps its size whatever the content asks. + let hosting = NSHostingController(rootView: AppearanceSettingsView(settings: .constant(.default))) + hosting.sizingOptions = [] + window.contentViewController = hosting + window.setContentSize(paneSize) window.layoutIfNeeded() RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.3)) defer { window.orderOut(nil) } From ff1633ec62d4bbcbdd6f6c3fb2494c04949e38c3 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 00:05:14 +0700 Subject: [PATCH 3/5] test(settings): keep the scroll helper the editor flow uses --- .../AppearanceSettingsUITests.swift | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/TableProUITests/AppearanceSettingsUITests.swift b/TableProUITests/AppearanceSettingsUITests.swift index 9d12e11c05..e1e1a5525f 100644 --- a/TableProUITests/AppearanceSettingsUITests.swift +++ b/TableProUITests/AppearanceSettingsUITests.swift @@ -60,11 +60,7 @@ final class AppearanceSettingsUITests: UITestCase { let last = row("Tokyo Night Light", in: list) XCTAssertTrue(last.waitToExist(timeout: 10)) - let deadline = Date(timeIntervalSinceNow: 10) - while last.frame.maxY > list.frame.maxY, Date() < deadline { - list.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).scroll(byDeltaX: 0, deltaY: -120) - } - XCTAssertLessThanOrEqual(last.frame.maxY, list.frame.maxY, "A scroll over the list should bring its last theme into view") + reveal(last, in: list, scrollingAt: CGVector(dx: 0.5, dy: 0.5)) let first = outlineRow("Tokyo Night", in: list) let next = outlineRow("Tokyo Night Light", in: list) @@ -180,6 +176,22 @@ final class AppearanceSettingsUITests: UITestCase { return settings } + /// A control clipped by its scroll view still reads as hittable, and a click on it lands on + /// whatever is drawn there, so the frames decide when it is in view. + private func reveal(_ control: XCUIElement, in scrollView: XCUIElement, scrollingAt point: CGVector) { + let deadline = Date(timeIntervalSinceNow: 10) + while control.frame.maxY > scrollView.frame.maxY || control.frame.minY < scrollView.frame.minY, Date() < deadline { + let overshoot = control.frame.maxY > scrollView.frame.maxY + ? scrollView.frame.maxY - control.frame.maxY - 20 + : scrollView.frame.minY - control.frame.minY + 20 + scrollView.coordinate(withNormalizedOffset: point).scroll(byDeltaX: 0, deltaY: overshoot) + } + XCTAssertTrue( + control.frame.minY >= scrollView.frame.minY && control.frame.maxY <= scrollView.frame.maxY, + "\(control.identifier) should scroll into view" + ) + } + private func choose(_ title: String, in popup: XCUIElement) { XCTAssertTrue(waitUntilHittable(popup, timeout: 10)) popup.click() From c18a857101b7d86bb1080009f210848ea5157e1d Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 00:09:31 +0700 Subject: [PATCH 4/5] test(settings): bound the visible list rows by the bar under them --- .../AppearanceSettingsUITests.swift | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/TableProUITests/AppearanceSettingsUITests.swift b/TableProUITests/AppearanceSettingsUITests.swift index e1e1a5525f..dbd356f936 100644 --- a/TableProUITests/AppearanceSettingsUITests.swift +++ b/TableProUITests/AppearanceSettingsUITests.swift @@ -55,21 +55,28 @@ final class AppearanceSettingsUITests: UITestCase { func testThemesListScrollsAndTakesArrowKeys() throws { let app = try launchOpeningURL("tablepro://settings/appearance") let settings = try appearancePane(in: app) - let list = settings.outlines["appearance-themes-list"] + let list = settings.outlines.containing(NSPredicate(format: "value == %@", "Default Light")).firstMatch XCTAssertTrue(list.waitToExist(timeout: 10), "The Themes list should be on the Appearance pane") + // The bar under the list marks where its visible rows end; an outline's own frame spans every row. + let footer = element("appearance-theme-add", in: settings) + XCTAssertTrue(footer.waitToExist(timeout: 10)) let last = row("Tokyo Night Light", in: list) XCTAssertTrue(last.waitToExist(timeout: 10)) - reveal(last, in: list, scrollingAt: CGVector(dx: 0.5, dy: 0.5)) + let deadline = Date(timeIntervalSinceNow: 10) + while last.frame.maxY > footer.frame.minY, Date() < deadline { + list.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.1)).scroll(byDeltaX: 0, deltaY: -60) + } + XCTAssertLessThanOrEqual(last.frame.maxY, footer.frame.minY, "A scroll over the list should bring its last theme into view") - let first = outlineRow("Tokyo Night", in: list) + let previous = outlineRow("Tokyo Night", in: list) let next = outlineRow("Tokyo Night Light", in: list) - XCTAssertTrue(waitUntilHittable(first, timeout: 5)) - first.click() - XCTAssertTrue(waitForPredicate(timeout: 5) { first.isSelected }, "A click should select the theme") + XCTAssertTrue(waitUntilHittable(previous, timeout: 5), "Tokyo Night should be visible above the last row") + previous.click() + XCTAssertTrue(waitForPredicate(timeout: 5) { previous.isSelected }, "A click should select the theme") settings.typeKey(.downArrow, modifierFlags: []) XCTAssertTrue( - waitForPredicate(timeout: 5) { next.isSelected && !first.isSelected }, + waitForPredicate(timeout: 5) { next.isSelected && !previous.isSelected }, "Down Arrow should move the selection, so the click must have given the list keyboard focus" ) } From cad8ccf4db1f2c738321b246a359a3db1f7f3acb Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 17 Oct 2026 00:17:54 +0700 Subject: [PATCH 5/5] fix(settings): indent the Themes header like the Form's section headers --- TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift b/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift index 1124d9ebd7..16ce33c852 100644 --- a/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift +++ b/TablePro/Views/Settings/Appearance/ThemeLibrarySection.swift @@ -34,11 +34,14 @@ internal struct ThemeLibrarySection: View { /// the edges of the sections above it. private static let formContentWidth: CGFloat = 704 private static let formMargin: CGFloat = 20 + /// How far a grouped Form indents its section headers from the section's edge. + private static let headerInset: CGFloat = 10 internal var body: some View { VStack(alignment: .leading, spacing: 8) { Text("Themes") .font(.headline) + .padding(.horizontal, Self.headerInset) .accessibilityAddTraits(.isHeader) list .alert(