diff --git a/TablePro/Views/Results/EditorAccessibilityIdentifier.swift b/TablePro/Views/Results/EditorAccessibilityIdentifier.swift new file mode 100644 index 0000000000..595f562bae --- /dev/null +++ b/TablePro/Views/Results/EditorAccessibilityIdentifier.swift @@ -0,0 +1,30 @@ +// +// EditorAccessibilityIdentifier.swift +// TablePro +// + +import AppKit +import TableProEditorKit + +/// Names the editor's `NSTextView` for accessibility, and for the UI tests that reach it that way. +/// +/// `.accessibilityIdentifier(...)` on the SwiftUI side names the representable, not the text view +/// underneath it, so a query for that identifier matches nothing. `UITestCase.editorTextView` says +/// so in as many words and keeps a `firstMatch` fallback because of it, which is only unambiguous +/// while exactly one text view is on screen. The row inspector has several. +/// +/// A coordinator is the documented way in: `prepareCoordinator` runs from `TextViewController` +/// with the text view already built, which is what an identifier has to be set on. +internal final class EditorAccessibilityIdentifier: TextViewCoordinator { + private let identifier: String + + internal init(_ identifier: String) { + self.identifier = identifier + } + + internal func prepareCoordinator(controller: TextViewController) { + controller.textView?.setAccessibilityIdentifier(identifier) + } + + internal func destroy() {} +} diff --git a/TablePro/Views/Results/JSONCodeEditor.swift b/TablePro/Views/Results/JSONCodeEditor.swift index eed5ac7d3d..7c0631d364 100644 --- a/TablePro/Views/Results/JSONCodeEditor.swift +++ b/TablePro/Views/Results/JSONCodeEditor.swift @@ -19,12 +19,19 @@ internal struct JSONCodeEditor: View { @State private var editorState = SourceEditorState() @State private var configuration: SourceEditorConfiguration + /// Held in `@State` so the editor is handed the same coordinator on every update. It names the + /// text view itself, which is where an accessibility identifier has to land: the SwiftUI + /// modifier names the representable and never reaches it. + @State private var coordinators: [any TextViewCoordinator] @Environment(\.colorScheme) private var colorScheme - init(text: Binding, isEditable: Bool) { + init(text: Binding, isEditable: Bool, accessibilityIdentifier: String? = nil) { self._text = text self.isEditable = isEditable self._configuration = State(wrappedValue: Self.makeConfiguration(isEditable: isEditable)) + self._coordinators = State( + wrappedValue: accessibilityIdentifier.map { [EditorAccessibilityIdentifier($0)] } ?? [] + ) } var body: some View { @@ -32,7 +39,8 @@ internal struct JSONCodeEditor: View { $text, language: .json, configuration: configuration, - state: $editorState + state: $editorState, + coordinators: coordinators ) .frame(maxWidth: .infinity, maxHeight: .infinity) .onChange(of: colorScheme) { _ in diff --git a/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift b/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift index d1e83f18de..c591005f92 100644 --- a/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift +++ b/TablePro/Views/RowInspector/FieldEditors/JsonEditorView.swift @@ -40,11 +40,14 @@ internal struct JsonEditorView: View { range: ResizableFieldMetrics.jsonHeightRange, expandedHeight: isExpanded ? ResizableFieldMetrics.expandedHeight : nil ) { - JSONCodeEditor(text: editorText, isEditable: !context.isReadOnly) + JSONCodeEditor( + text: editorText, + isEditable: !context.isReadOnly, + accessibilityIdentifier: "inspector-json-field" + ) .clipShape(RoundedRectangle(cornerRadius: 5)) .overlay(RoundedRectangle(cornerRadius: 5).strokeBorder(Color(nsColor: .separatorColor))) .overlay(alignment: .bottomTrailing) { actionButtons } - .accessibilityIdentifier("inspector-json-field") } /// `newValue`, never a re-read of `context.value.wrappedValue`. An `onChange` action /// closure belongs to the render that registered it, so its captured context is a render diff --git a/TableProTests/Models/UI/JsonFieldEditingModelTests.swift b/TableProTests/Models/UI/JsonFieldEditingModelTests.swift index 8043a7fd49..129cd61efa 100644 --- a/TableProTests/Models/UI/JsonFieldEditingModelTests.swift +++ b/TableProTests/Models/UI/JsonFieldEditingModelTests.swift @@ -13,8 +13,8 @@ import Testing /// /// The editor and the store are two mirrors of one value, and the hang was a fixpoint between /// them: one keystroke produced 6,944 body evaluations in 30 seconds at 100% CPU, and the typed -/// character was thrown away. `settle` is the shape that catches it: drive the pair until nothing -/// moves, and fail if that takes more than a round. +/// character was thrown away. What catches it is ``adoptsRestatement``: the store always answers +/// with a different string than the editor holds, and adopting that answer is the bug. @MainActor @Suite("JSON field editing model") struct JsonFieldEditingModelTests { @@ -29,26 +29,25 @@ struct JsonFieldEditingModelTests { return state } - /// Runs the editor and the store against each other the way SwiftUI does: the editor publishes, - /// the store restates, the editor is told what the store now holds, and round again. Returns - /// how many rounds it took to stop moving. - @discardableResult - private static func settle( + /// Whether the editor adopts the store's restatement of what the editor itself just published. + /// + /// This is the whole of #3051 in one question. `MultiRowEditState` stages a compacted form of + /// the text the editor shows, so the store always answers with a *different string* than the + /// editor holds. Adopting that answer is what overwrote the keystroke and pushed the overwrite + /// back, at about 75 rounds a second. + /// + /// Asserted directly rather than by looping the pair until it settles: `displayText` and + /// `lastSynced` are equal at every exit of both entry points, so a loop that re-publishes + /// `model.displayText` can never take a second turn and would report convergence for any model + /// at all, including one with the bug. + private static func adoptsRestatement( _ model: inout JsonFieldEditingModel, - against state: MultiRowEditState, - limit: Int = 8 - ) -> Int { - for round in 1...limit { - let delivered = state.currentText(at: 0) - guard model.received(delivered) else { return round } - guard let published = model.typed(model.displayText) else { return round } - state.updateField(at: 0, value: published) - } - Issue.record("The editor and the store never agreed within \(limit) rounds") - return limit + from state: MultiRowEditState + ) -> Bool { + model.received(state.currentText(at: 0)) } - @Test("A keystroke survives the round trip and settles in one round") + @Test("A keystroke survives the round trip and the store's echo is refused") func keystrokeSettles() { let original = "{\"a\":1}" let state = Self.makeState(value: original, type: .json(rawType: "jsonb")) @@ -60,15 +59,15 @@ struct JsonFieldEditingModelTests { #expect(published == typed) state.updateField(at: 0, value: published) - let rounds = Self.settle(&model, against: state) - #expect(rounds == 1) + let adopted = Self.adoptsRestatement(&model, from: state) + #expect(!adopted, "the store answers with the compact form of what was just typed") #expect(model.displayText == typed, "the editor keeps its layout, not the compact echo") #expect(state.fields[0].pendingValue == "{\"a\":2}") } /// The document is invalid for as long as the user is halfway through typing a value, which is /// where `JsonReindenter`'s parse-failure fallback used to make the two sides disagree. - @Test("A transiently invalid document settles at every step") + @Test("A transiently invalid document keeps every keystroke") func invalidIntermediateSettles() { let original = "{\"a\": 1}" let state = Self.makeState(value: original, type: .text(rawType: "text")) @@ -78,15 +77,15 @@ struct JsonFieldEditingModelTests { if let published = model.typed(typed) { state.updateField(at: 0, value: published) } - let rounds = Self.settle(&model, against: state) - #expect(rounds == 1, "stalled on \(typed)") + let adopted = Self.adoptsRestatement(&model, from: state) + #expect(!adopted, "the store's restatement was adopted on \(typed)") #expect(model.displayText == typed, "the keystroke must survive") } } /// The store drops a pending value that matches the stored one again, so the binding answers /// with the original. That is still this editor's own echo and must not be adopted. - @Test("Editing back to the stored value settles rather than bouncing") + @Test("Editing back to the stored value does not bounce") func revertToOriginalSettles() { let original = "{\"a\":1}" let state = Self.makeState(value: original, type: .json(rawType: "jsonb")) @@ -97,8 +96,10 @@ struct JsonFieldEditingModelTests { state.updateField(at: 0, value: model.typed(JsonReindenter.reindent(original))) #expect(!state.fields[0].hasEdit) - let rounds = Self.settle(&model, against: state) - #expect(rounds == 1) + /// The store dropped the pending value, so it now answers with `originalValue`. That is + /// still this editor's own write coming back and must not be adopted. + let adopted = Self.adoptsRestatement(&model, from: state) + #expect(!adopted) } @Test("The editor's own write is never adopted back") @@ -112,12 +113,16 @@ struct JsonFieldEditingModelTests { #expect(model.displayText == typed, "the caret's layout must not be rewritten") } - @Test("A value the editor did not write is adopted") + @Test("A value the editor did not write is adopted, and is not published back") func externalChangeIsAdopted() { var model = JsonFieldEditingModel(storedValue: "{\"a\":1}") let adopted = model.received("{\"b\":2}") #expect(adopted) #expect(model.displayText == JsonReindenter.reindent("{\"b\":2}")) + /// The adopted value arrived compact and is displayed laid out, so the two differ by more + /// than nothing. Republishing it would stage an edit the user never made. + let republished = model.typed(model.displayText) + #expect(republished == nil, "adopting is not an edit") } /// Set NULL, Set DEFAULT and Set EMPTY all empty the field's editable text. Adopting that and diff --git a/TableProUITests/RowInspector/InspectorJsonFieldEditUITests.swift b/TableProUITests/RowInspector/InspectorJsonFieldEditUITests.swift index fbbbe93ef6..e3a6789860 100644 --- a/TableProUITests/RowInspector/InspectorJsonFieldEditUITests.swift +++ b/TableProUITests/RowInspector/InspectorJsonFieldEditUITests.swift @@ -2,11 +2,11 @@ // InspectorJsonFieldEditUITests.swift // TableProUITests // -// The row inspector's JSON field kept two mirrors of one value in two textual forms and -// reconciled them from an `onChange` action that re-read a context a render out of date. Typing -// one character made the editor put the previous value back and push it into the store again, at -// about 75 rounds a second: one keystroke produced 6,944 body evaluations in 30 seconds at 100% -// CPU, and the character was thrown away (#3051). +// The row inspector's JSON field kept two mirrors of one value and reconciled them from an +// `onChange` action that re-read a context a render out of date. Typing one character made the +// editor put the previous value back and push it into the store again, at about 75 rounds a +// second: one keystroke produced 6,944 body evaluations in 30 seconds at 100% CPU, and the +// character was thrown away (#3051). // // So the assertion is not "the app is responsive", which is hard to state and easy to pass by // accident. It is that the character is still there afterwards, which the loop could not manage. @@ -26,16 +26,15 @@ final class InspectorJsonFieldEditUITests: UITestCase { let app = try launchWithSampleDatabase(environment: [jsonFixtureVariable: "1"]) let window = app.windows.firstMatch - try openFixtureTable(in: app) + openFixtureTable(in: app) showInspector(in: app) let grid = window.tables.matching(identifier: "data-grid").firstMatch XCTAssertTrue(grid.waitToExist(timeout: 30), "The fixture table must produce a result grid") - XCTAssertTrue( - waitForPredicate(timeout: 30) { grid.tableRows.firstMatch.exists }, - "The fixture table must return a row" - ) - gridPoint(in: grid, of: window, dy: 12).click() + XCTAssertTrue(waitForClickableRows(in: grid), "The fixture table must return a row") + /// Past the header, which is 28pt with no column comments, and inside the first row at + /// every row height the setting offers. + gridPoint(in: grid, of: window, dy: 40).click() let field = window.textViews.matching(identifier: "inspector-json-field").firstMatch XCTAssertTrue(field.waitToExist(timeout: 20), "The JSON column must open the JSON editor") @@ -47,7 +46,7 @@ final class InspectorJsonFieldEditUITests: UITestCase { app.typeText(typedMarker) XCTAssertTrue( - waitForPredicate(timeout: 20) { (field.value as? String)?.contains(self.typedMarker) == true }, + waitForPredicate(timeout: 20) { (field.value as? String)?.contains(self.typedMarker) ?? false }, "Every typed character must survive; the editor holds '\(field.value as? String ?? "nil")'" ) @@ -64,16 +63,22 @@ final class InspectorJsonFieldEditUITests: UITestCase { // MARK: - Helpers - private func openFixtureTable(in app: XCUIApplication) throws { - let browser = app.windows.firstMatch.outlines.firstMatch - XCTAssertTrue(browser.waitToExist(timeout: 30), "The object browser must list the sample's tables") + /// The object browser draws its rows as hosted cells, so the name arrives as the static text's + /// `value` behind the object kind, and the row takes a coordinate click rather than an + /// element one. `objectBrowserRow` carries both facts. + private func openFixtureTable(in app: XCUIApplication) { + let window = app.windows.firstMatch + XCTAssertTrue( + window.outlines.firstMatch.waitToExist(timeout: 30), + "The object browser must list the sample's tables" + ) - let table = browser.staticTexts[fixtureTable].firstMatch + let row = objectBrowserRow(fixtureTable, in: window) XCTAssertTrue( - table.waitToExist(timeout: 30), + row.waitToExist(timeout: 30), "The seeded fixture table must appear in the object browser" ) - table.doubleClick() + row.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).doubleClick() } /// The inspector remembers whether it was open, so the starting state is whatever the previous