Add ui audit command for per-view accessibility checks#601
Add ui audit command for per-view accessibility checks#601yeelam-gordon wants to merge 1 commit into
Conversation
Adds a new `winapp ui audit` command that evaluates the currently visible app view for accessibility and contrast issues across names, keyboard, screen-reader-readiness, contrast, and roles, with `--area` and `--level` controls. The command description now explicitly states that it audits one view at a time and should be paired with other UI automation to move through additional pages/tabs/states. Also updates the generated CLI schema and UI automation skill/docs to surface the new command. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new winapp ui audit subcommand that runs static accessibility and WCAG contrast checks over the currently visible view of a running Windows app. It is built as a modular subsystem: a pure rule engine (UiAuditEngine) plus a ContrastAnalyzer, fronted by per-area engines (names, keyboard, screen-reader, contrast, roles, and a reserved no-op events) that are composed by a DI-registered UiAuditOrchestrator. The command reuses the existing inspect tree walk and screenshot pipeline (refactored to expose a full-window pixel capture), scopes checks via --area and depth via --level (basic=AA / thorough=AAA), de-duplicates cross-area findings, and exits non-zero on any FAIL so it can gate CI. Docs, the CLI schema, and the Copilot/Claude skill files are regenerated accordingly.
Changes:
- New
ui auditcommand with a modular area-engine/orchestrator architecture and a pure, unit-testable contrast/accessibility rule engine. - New
IUiAutomationService.CaptureWindowAsync(with a refactor extractingCaptureWindowRawAsync) and a non-serializedUiElement.IsKeyboardFocusable, both consumed by the audit. - Extensive new unit tests plus regenerated docs/schema/skill artifacts.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
Commands/UiAuditCommand.cs |
New command: option parsing, contrast capture, orchestration, JSON/human output. |
Commands/UiCommand.cs |
Registers audit as a ui subcommand. |
Helpers/UiAuditEngine.cs |
Pure accessibility/contrast rule engine over flat element list. |
Helpers/ContrastAnalyzer.cs |
WCAG contrast computation from BGRA buffers (Otsu split). |
Helpers/UiAudit/*.cs |
Area/profile definitions, area engines, orchestrator, context, interface. |
Helpers/HostBuilderExtensions.cs |
DI registration of area engines + orchestrator + command handler. |
Helpers/UiJsonContext.cs |
Audit result JSON models + source-gen registration (contains an unrelated merged-line edit). |
Helpers/CliSchema.cs |
Adds float to schema context (appears unused/unrelated). |
Services/UiAutomationService*.cs, IUiAutomationService.cs |
New CaptureWindowAsync + screenshot refactor; IsKeyboardFocusable extraction. |
Models/UiElement.cs |
Adds non-serialized IsKeyboardFocusable. |
WinApp.Cli.Tests/* |
New audit/engine/analyzer/orchestrator tests; fake/stub updated for the new interface method. |
docs/*, .github/plugin/*, .claude/*, scripts/generate-llm-docs.ps1, .gitignore |
Regenerated schema/skill docs, usage docs, command map, and scratch-dir ignores. |
Notable points beyond the inline comments: the new command is not reflected in docs/ui-automation.md (which has a ### section per ui subcommand) nor in the hand-written "Key subcommands" list in .github/plugin/agents/winapp.agent.md; both are outside this diff so no inline comment was made, but they should be updated for completeness.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| internal sealed class UiScrollResult | ||
| }internal sealed class UiScrollResult |
| [JsonSerializable(typeof(ManifestTemplates))] | ||
| [JsonSerializable(typeof(float))] |
| { | ||
| var msg = $"Invalid --area '{invalidArea}'. Allowed values: {string.Join(", ", AuditArea.Selectable)}."; | ||
| logger.LogError("{Symbol} {Message}", UiSymbols.Error, msg); | ||
| UiJsonError.Emit(json, UiJsonError.CodeInternalError, msg); |
| { | ||
| var msg = $"Invalid --level '{parseResult.GetValue(LevelOption)}'. Allowed values: {string.Join(", ", AuditProfile.All)}."; | ||
| logger.LogError("{Symbol} {Message}", UiSymbols.Error, msg); | ||
| UiJsonError.Emit(json, UiJsonError.CodeInternalError, msg); |
| private static (string Markup, string PlainText) BuildHumanReport( | ||
| UiAuditResult result, UiSessionInfo session, string level, | ||
| IReadOnlyList<string> scope, bool needsContrast, bool contrastMeasured) |
Build Metrics ReportBinary Sizes
Test Results✅ 1605 passed, 1 skipped out of 1606 tests in 389.0s (+60 tests, -89.0s vs. baseline) Test Coverage❌ 18.5% line coverage, 38.1% branch coverage · ✅ +0.4% vs. baseline CLI Startup Time42ms median (x64, Updated 2026-07-06 03:16:10 UTC · commit |
nmetulev
left a comment
nmetulev
left a comment
There was a problem hiding this comment.
🤖 AI-generated review — produced by GitHub Copilot CLI at a maintainer's request. Every finding comes from an automated, multi-dimensional review plus a hands-on build-and-test of this branch in an isolated worktree. Treat
file:linereferences as pointers to verify, not gospel. Nothing was pushed. Posting as a comment (no approve / request-changes).
Verdict: 🟡 SHIP-WITH-CHANGES — Sound architecture and a genuinely useful idea, but two correctness bugs make the current output misleading, and there's a scope question worth a deliberate decision.
What I actually ran
Built (~41s, 0 warnings); 23 engine + 13 orchestrator + 126 ui tests pass. Audited Notepad and Calculator:
--areascoping,--level basic/thorough,--json, exit codes (0 pass / 1 fail): all work; the{summary, issues}JSON envelope is stable.- Contrast AA on Notepad's status bar returned PASS at a measured ~3.15–3.34:1 — a false negative (see H1).
--level thorough(AAA) correctly tightened thresholds and flagged more.- Calculator returned 0 elements but printed "✓ No accessibility issues found" (M5 — indistinguishable from genuinely clean).
Must-fix (High)
- H1 — DPI-dependent large-text false negatives. "Large text" is decided by bounding-box height (
LargeTextHeightPx = 24,UiAuditEngine.cs:91), not font size in points. At 125–150% DPI, normal ~12pt text is classified "large" and judged against the lenient 3.0:1 threshold, so ~3.2:1 text passes AA. Real false negatives on the majority of displays; the unit tests use a fixed height of 16 and miss it. Read font size fromTextPattern(UIA_FontSizeAttributeId), or normalize the pixel height by the window DPI scale. - H2 — Pane false positives.
Pane + IsInvokableis flagged "interactive but has no accessible name" (UiAuditEngine.cs:128-136). Shell/framework panes exposeInvokePatternstructurally, producing name-FAIL + role-WARN noise on nearly every app (confirmed on Notepad). Exclude container types (Pane/Group/Window) unless also keyboard-focusable.
Medium / Low (selected)
- M1 contrast epsilon
r + 0.05 < thresholdsilently passes ratios up to 0.05 below AA (e.g.#777on white = 4.478:1 not flagged). User < threshold. - M2 the
screen-readerarea is a static UIA proxy; the name over-promises AT-driven testing. - M3 the pass counter sums per-check hits ("103 passed" for ~30 elements).
- M4
AutomationId == NameWARN false-positives on standard Win32 menu items (File/Edit/View). - M5 0-element result is reported as clean.
- L1 sRGB linearization uses the WCAG 2.1-errata
0.03928vs the corrected0.04045. L3--level aa/aaaaliases are rejected (onlybasic/thorough).
Fit & recommendation
This is the largest of the five PRs and the one that moves furthest from packaging/automation into accessibility certification — a space owned by mature dedicated tools (Accessibility Insights for Windows, Arc, Axe). The value-add here is the CLI/agent-composable interface, not the checks themselves. The risk: an incomplete or incorrect auditor gives false confidence for Store submission. If it ships, the DPI + contrast bugs are non-negotiable, and the docs should state plainly that it's a quick lint, not a WCAG cert. I'd treat inclusion as a deliberate scope decision rather than an automatic yes.
|
I believe Accessibility Insights has "Fast Pass" which runs this same set of checks on whatever page of an app you point it to. It would be good to investigate if we can reuse Fast Pass here in some way so we don't have to maintain our own dictionary of accessibility rules and can lean on accessibility insights to update and maintain the correct accessibility policies. |
Summary
winapp ui auditfor names, keyboard, screen-reader-readiness, contrast, and role checks--areaand--levelto scope the audit and choose AA/basic vs AAA/thorough depthWhy
This adds a high-signal accessibility/contrast lint over the current UI view while keeping whole-app coverage composable with the rest of the UI automation primitives.