fix(datagrid): give assistive clients a data cell they can read (#2381) - #2392
Merged
Conversation
…ed run never opens (#721) Claude-Session: https://claude.ai/code/session_01NAexgozD2h1j1EetZ6ArMH
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
Main has been red for three runs. Eight UI tests failed in run 32631046978, in two unrelated groups.
The data grid published no accessibility at all
Since #2388 every data cell in every result was a blank
AXCell. VoiceOver read the whole grid as empty, and the two grid UI tests were the symptom.NSTableViewbuilds itsAXCelltree from cell views and from nothing else. TheNSAccessibilityElements the row published never reached that tree, and AppKit put its own placeholder in each one's place. Measured four ways on macOS 27: the element's identifier, value, label and children never appear however they are set; overriding the accessibility methods instead of using the setters changes nothing; an element that declares.staticTextstill shows up asCell; and the frames in the tree follow AppKit's own convention (column width less the intercell spacing) rather than the ones the element reports.So a drawn cell can only speak through a view.
DataGridCellAccessibilityViewis one per data cell that draws nothing, takes no clicks, and reads its text through the coordinator so an edit is spoken with nothing to invalidate. It mounts only onceDataGridAccessibility.isActive, which the first accessibility question asked of the grid sets: nothing reports an attached client, sinceNSWorkspace.isVoiceOverEnabledcovers VoiceOver alone and nothing at all reports Switch Control, Voice Control, an inspector or XCUITest. A session with no assistive client keeps #2381's numbers exactly: 26 views for a 500-column table rather than 12,500.Two smaller fixes ride along. The row now answers an accessibility hit test for the width it covers that no cell does, instead of falling through to the view that draws the cells, which is not in the tree. And that drawing view is out of the tree entirely.
testCommandDeleteDeletesTheEditorLineAfterSelectingAResultRowalso had to stop clicking a row element. A table publishes its columns as siblings of its rows, each as tall as every row it spans and later in the tree, so XCUITest reads every row and every cell in the grid as obscured and refuses to click either. It clicks a point offset from thedata-gridelement now, the same shape #1837 needed.Compare & Sync tested a window that never opens
All five
CompareSyncUITestscases failed, and PR #1968 was merged with them failing. They could not have passed anywhere: the window is Starter-gated, the UI test sandbox carries no license, and choosing the menu item puts up the upgrade alert instead. The assertions were written against an earlier design too, looking for apopUpButtonsvalue of "Choose a connection" that the rebuilt toolbar spells "Choose Source".The suite now covers what an unlicensed run does, the way
QueryInsightsTabUITestsalready does for its own gate: the menu item is reachable and stays enabled, because a feature the user can buy has to be discoverable, and choosing it explains the gate and opens no window. The window's own start-state contract moved toCompareSyncStartStateTests, which reaches it without a license: neither endpoint preselected, Compare and Swap refusing with a reason, the banner saying nothing has been written.This leaves the window itself without UI coverage. Giving it any would mean a way to grant a license in the sandbox, which is a licensing bypass to design rather than a CI fix.
Verification
DrawnCellReachabilityTestsrewritten: no view mounts while accessibility is idle, an active client gets a cell carrying its value, an edited cell reads back without invalidation, the mounted cell takes no clicks, a hit test past the last column lands on the row.QueryTabDeleteLineUITests,QueryHistoryActionsUITests,CompareSyncUITests,ResultStatusBarUITests,SQLiteFirstTableLoadUITestsandSidebarTableTabUITestsall pass.testReturnInTheListLoadsTheSelectedEntryfails on this machine, and fails identically on unmodifiedmain; it passes on CI.https://claude.ai/code/session_01NAexgozD2h1j1EetZ6ArMH