feat(a11y): complete WASM and Appium validation - #23755
feat(a11y): complete WASM and Appium validation#23755morning4coffe-dev wants to merge 22 commits into
Conversation
Sorry, something went wrong.
|
Claude finished @morning4coffe-dev's task in 5m 8s —— View job Code Review
SummaryThe PR is a well-structured foundation for cross-platform accessibility smoke tests and snapshot baselines. The adapter abstraction, canonical-role normalization, and tree-dumping pipeline are all sound. The macOS native changes (exposing AutomationId through AXIdentifier / accessibilityIdentifier) are the right approach. Since the PR is explicitly marked draft / not ready for review, findings are filed as early guidance rather than blocking issues. Issues by severity
Cross-cutting observations
|
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds an Appium-driven accessibility validation harness for SamplesApp (smoke tests + golden-file accessibility-tree snapshots) and extends the macOS Skia accessibility backend to expose AutomationProperties.AutomationId via the native NSAccessibility identifier so Appium Mac2/XCUITest can locate elements consistently.
Changes:
- Expose AutomationId on macOS Skia through -accessibilityIdentifier / AXIdentifier and add a native update hook.
- Add SamplesApp.AppiumTests with platform adapters (Win32 UIA, macOS NSAccessibility, WASM DOM/ARIA), tree capture/normalize/serialize infrastructure, and baseline comparison tests.
- Add documentation for running/recording Appium snapshots and line-ending normalization for committed JSON baselines.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file| File | Description |
|---|---|
| src/Uno.UI.Runtime.Skia.MacOS/UnoNativeMac/UnoNativeMac/UNOAccessibility.m | Adds identifier exposure through NSAccessibility and a native update function. |
| src/Uno.UI.Runtime.Skia.MacOS/UnoNativeMac/UnoNativeMac/UNOAccessibility.h | Declares unoIdentifier and the new update API. |
| src/Uno.UI.Runtime.Skia.MacOS/Native/NativeUno.cs | Adds P/Invoke for uno_accessibility_update_identifier. |
| src/Uno.UI.Runtime.Skia.MacOS/Accessibility/MacOSAccessibility.cs | Pushes AutomationId into the native accessibility element. |
| src/SamplesApp/SamplesApp.AppiumTests/Tests/AutomationTreeSmokeTests.cs | Adds cross-platform smoke tests validating tree presence and basic interaction. |
| src/SamplesApp/SamplesApp.AppiumTests/Tests/AccessibilityBaselineTests.cs | Adds golden-file snapshot comparisons with optional record mode. |
| src/SamplesApp/SamplesApp.AppiumTests/Snapshots/.gitattributes | Forces LF for committed snapshot JSON files. |
| src/SamplesApp/SamplesApp.AppiumTests/SamplesApp.AppiumTests.csproj | New Appium/NUnit-based test project. |
| src/SamplesApp/SamplesApp.AppiumTests/README.md | Documents prerequisites, platform setup, and baseline workflows. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/WindowsAdapter.cs | Windows UIA adapter for role/name/value/pattern extraction and locators. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/WasmAdapter.cs | WASM adapter using DOM/ARIA + xamlname-based locators. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/TreeDumper.cs | Walks the automation tree and builds normalized snapshot nodes. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/SnapshotSerializer.cs | Deterministic JSON serialization for snapshots. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/SnapshotComparer.cs | Structural diffing for snapshots. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/MacAdapter.cs | macOS Mac2 adapter with wrapper-bundle launching and AX queries. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/IPlatformAdapter.cs | Common adapter contract. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/CanonicalRole.cs | Normalizes platform-specific roles into shared tokens. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/AppiumPlatform.cs | Platform selection and env-var configuration. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/AppiumFixtureBase.cs | Shared fixture lifecycle and wait helpers. |
| src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/AccessibilityNode.cs | Platform-neutral accessibility node model for snapshots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23755/wasm-skia-net9/index.html |
Sorry, something went wrong.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Claude finished @morning4coffe-dev's task in 5m 0s —— View job Code Review
SummaryThe PR is still in draft state so these are early-guidance findings. The prior review pass covered the main medium-severity issues well; these comments add four new items not previously filed. New findings
Cross-cutting observations
|
Sorry, something went wrong.
|
Claude encountered an error after 39s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (1)Previously missed (1) — in code that hasn't changed since the last review.
src/SamplesApp/SamplesApp.AppiumTests/Infrastructure/TreeDumper.cs:22
- The comment for s_noiseAutomationIds says these IDs are “Excluded from snapshots”, but TreeDumper is used to write the diagnostic *.tree.json artifact (via AppiumTestSession.TryWriteDiagnosticTree), not to build/compare the committed canonical baselines. Updating the wording would avoid confusion about what is actually being filtered here.
/// <summary> /// AutomationIds that come from WinUI control templates / window chrome /// rather than the sample under test. Excluded from snapshots so the /// recorded baselines are stable across host frameworks and sample /// changes that don't touch these parts.
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23755/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
Claude encountered an error after 30s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23755/docs/index.html |
Sorry, something went wrong.
ComboBox light-dismiss audit: now green on both halvesFollowing up on the analysis above with the CI result, since the browser half could only be proven there.
The browser test needed two corrections before it was sound, and both are worth recording because they were my errors, not product defects:
All four WebAssembly Skia Runtime Tests shards are green on 6b3418ff80, as is WebAssembly Skia Accessibility Automation (4m12s). So the adaptation is now pinned from both directions: the peer tree cannot gain a synthesized light-dismiss child without failing the first test, and the browser tree cannot gain a "Close" node or an extra popup wrapper without failing the second. The generic PopupRootAutomationPeer gap remains tracked in #24191. |
Sorry, something went wrong.
|
Claude encountered an error after 38s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
The build 230074 found UI Test snapshots differences: skia-linux-screenshots: 57, skia-windows-screenshots: 2348, wasm: 11 Details
|
Sorry, something went wrong.
|
Claude encountered an error after 30s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
Claude encountered an error after 21s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
Claude encountered an error after 30s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (1)Previously missed (1) — in code that hasn't changed since the last review.
src/SamplesApp/SamplesApp.AppiumTests/Tests/AppiumConfigurationTests.cs:11
- These tests mutate process-wide environment variables via EnvironmentVariableScope. If MSTest runs classes/tests in parallel, this can cause flaky cross-test interference; mark the class (or assembly) as non-parallelizable, matching the existing pattern used in other test projects that touch env vars.
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23755/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
Claude encountered an error after 27s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23755/docs/index.html |
Sorry, something went wrong.
|
Claude encountered an error after 23s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
Claude encountered an error after 26s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
Claude encountered an error after 23s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
The build 230130 found UI Test snapshots differences: skia-linux-screenshots: 57, skia-windows-screenshots: 2348, wasm: 11 Details
|
Sorry, something went wrong.
|
Claude encountered an error after 38s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
Status on e0aebcd862: everything green except review73 success, 1 neutral (Mergify Merge Queue, skipped by design), 1 failure (review), 0 pending. 96/96 review threads resolved. Since the last update:
review — 17th consecutive failure, root cause unchangedRetried on schedule (hourly, never more often) across six head commits — 76ae4aa0ac, d2251277c4, d13c41f566, 3310cf55b1, 6b3418ff80, e0aebcd862 — plus the automatic run on each push. Every attempt ends identically: abort on turn 2, permission_denials_count: 0, no repository content evaluated. The measurement from run 32821649561 still holds and the diff has only grown since:
The agent's only route to the code is Bash(gh pr diff:*) — the tool allowlist grants nothing else that can read the repository — so the request carrying that diff is unservable. No change I can make to this branch reduces that number. I have deliberately not touched claude-code-review.yml. Its header documents an explicit security posture — trigger gate for repo members only, tool allowlist, no id-token: write, SHA-pinned action — and relaxing any of it to turn a check green would trade a real safeguard for a cosmetic result. This needs a maintainer decision. Options, in the order I'd suggest:
I'll keep retrying hourly and watching for new review threads until one of those happens. |
Sorry, something went wrong.
|
Claude encountered an error after 24s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
|
Claude encountered an error after 14s —— View job I'll analyze this and get back to you. |
Sorry, something went wrong.
Post-cooldown re-check (2026-08-26): reviewer config unchanged, blocker unchangedRe-checked the reviewer configuration before spending another attempt, then ran exactly one fresh attempt. No code was pushed — head is still e0aebcd862. Reviewer settings — nothing moved overnight
The workflow file on the PR head is byte-identical to master's, so the merge-ref run uses exactly this configuration. I confirmed the values at runtime from the new job log rather than only from the file — it reports "model": "claude-sonnet-4-6" and the same four-tool allowlist. The one fresh attempt — run 32857939581, attempt 8{ "type": "result", "subtype": "success", "is_error": true,
"duration_ms": 14412, "num_turns": 2, "total_cost_usd": 0.82, "permission_denials_count": 0 }
Same signature as all eighteen previous attempts: aborts on turn 2, no tool denied, no repository content evaluated. Re-measured inputs:
The context shrank slightly (fewer bot comments after the overnight quiet period) and the diff is essentially flat, so the sum is still ~269k tokens against a 200K window. The failure is pre-agent and deterministic; it is not a transient the cooldown could clear. Per instruction I am not looping on retries any further. Everything else is still greene0aebcd862: 73 success, 1 neutral (Mergify Merge Queue, skipped by design), 1 failure (review), 0 pending. The lanes this PR exists to add are green — WebAssembly Skia Accessibility Automation and all four WebAssembly Skia Runtime Tests shards. All 96 review threads resolved; no new threads and no maintainer response since yesterday's request. The decision options from #issuecomment-5414937414 stand unchanged: waive review for this PR, reconfigure the workflow for large PRs (#23924, #24174, #23935 and #22432 fail identically today), or authorize splitting the PR. I'll act on whichever you pick. |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
GitHub Issue: closes #23751
PR Type:
✨ Feature
What changed? 🚀
This completes the Skia WebAssembly accessibility object model, including roles, names, relationships, live state, focus and tab order, landmarks, standalone text, hidden-subtree pruning, and virtualized control behavior.
The Appium validation layer now uses canonical accessibility snapshots, actionable element-based differences, strict driver lifecycle handling, and interaction coverage for invoke, toggle, selection, value, and focus behavior. The adapters consistently escape selectors and normalize platform roles, while the dedicated Skia WebAssembly CI path validates the published SamplesApp through ChromeDriver.
Windows UIA and macOS NSAccessibility adapters remain available for host-specific runs. Their baselines must be recorded on matching hosts before those lanes are enabled; the repository does not use synthetic platform baselines.
Validation
PR Checklist ✅