feat(a11y): complete WASM and Appium validation - #23755
feat(a11y): complete WASM and Appium validation#23755morning4coffe-dev wants to merge 10 commits into
Conversation
github-actions
Bot
added
platform/macos 🍏
|
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 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| try | ||
| { | ||
| var psi = new ProcessStartInfo("/usr/bin/defaults", | ||
| $"read \"{plist.Replace("\"", "\\\"")}\" CFBundleIdentifier") |
There was a problem hiding this comment.
Argument-splitting bug / potential injection via Arguments string property.
ReadBundleId uses the Arguments string overload of ProcessStartInfo, not ArgumentList. The CLR splits that string on whitespace/double-quotes, so a path with spaces will break even with the \" escaping applied, and characters like `, $, etc. will be passed through to defaults verbatim (no shell, but still mis-split).
Use ArgumentList instead:
var psi = new ProcessStartInfo("/usr/bin/defaults")
{
UseShellExecute = false,
RedirectStandardOutput = true,
};
psi.ArgumentList.Add("read");
psi.ArgumentList.Add(plist);
psi.ArgumentList.Add("CFBundleIdentifier");This is safe for any path the OS considers valid. Fix this →
| => path.EndsWith(".app", StringComparison.OrdinalIgnoreCase) | ||
| || path.TrimEnd('/').EndsWith(".app", StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| private static Process SpawnDotnet(string dllPath, string sampleQuery) |
There was a problem hiding this comment.
Dead code: SpawnDotnet and WaitForWindow are defined but never called.
SpawnDotnet (line 396) and WaitForWindow (line 451) are defined but have no call sites in this file. CreateDriver switched to the wrapper-bundle strategy and the code paths that would have called these were removed. Delete both methods to prevent confusion about whether the .dll-launch flow is still supported. Fix this →
|
|
||
| const string executableName = "SamplesAppAppium"; | ||
| const string bundleId = "io.platform.uno.SamplesAppAppium"; | ||
|
|
There was a problem hiding this comment.
WrapperBundleId constant is duplicated here as a local constant.
The class-level constant WrapperBundleId = "io.platform.uno.SamplesAppAppium" (line 22) and the local const string bundleId = "io.platform.uno.SamplesAppAppium" here must be kept in sync manually. Reuse the class constant:
| const string executableName = "SamplesAppAppium"; | |
| string bundleId = WrapperBundleId; |
| // `do shell script` routes through the user's login shell, escaping | ||
| // the sandboxed launchd domain we inherited from VS Code's helper | ||
| // process tree. | ||
| psi.ArgumentList.Add($"do shell script \"/usr/bin/open -n '{bundlePath}'\""); |
There was a problem hiding this comment.
Shell script in LaunchBundle embeds bundlePath without single-quote escaping.
The osascript argument is:
$"do shell script \"/usr/bin/open -n '{bundlePath}'\""If bundlePath contains a ' (single quote), the AppleScript string breaks. The temp path from Path.GetRandomFileName() won't contain one, but this is fragile. Escape the path for AppleScript string context:
var escapedPath = bundlePath.Replace("'", "'\\''");
psi.ArgumentList.Add($"do shell script \"/usr/bin/open -n '{escapedPath}'\"");
| ["menubar"] = "menubar", | ||
| ["separator"] = "separator", | ||
| ["status"] = "statusbar", | ||
| ["tab"] = "tab", |
There was a problem hiding this comment.
Semantic mismatch: tabpanel should not map to tabitem.
A tabpanel is the content pane associated with a tab, not the tab trigger itself. Mapping it to "tabitem" will silently collapse two structurally distinct elements into the same canonical role, making cross-platform diffs misleading.
"tabitem" is the correct mapping for the "tab" role (the clickable tab header). "tabpanel" should map to a distinct canonical role — or if the snapshot suite doesn't model it, it should fall through to the verbatim lowercased value so it surfaces clearly in diffs.
| ["tab"] = "tab", | |
| ["tabpanel"] = "tabpanel", |
| return Path.Combine(root, flavor, snapshotId + ".json"); | ||
| } | ||
|
|
||
| private static string SourceTreeSnapshotsDir([CallerFilePath] string? thisFile = null) |
There was a problem hiding this comment.
[CallerFilePath] embeds the build-machine path, not the runtime path — unreliable on CI.
[CallerFilePath] is a compile-time constant injected by the compiler. On a CI agent that builds on machine A and tests on machine B (or in a Docker container), thisFile will be the absolute path on machine A, which won't exist at runtime on machine B.
The env-var override (UNO_APPIUM_SNAPSHOTS_DIR) works around this for CI, but relying on [CallerFilePath] as a "preferred" runtime path is misleading. Consider flipping the precedence: use the env var unconditionally, and fall back to a path relative to the test output directory (TestContext.CurrentContext.TestDirectory) when the env var is absent. Fix this →
|
|
||
| // Give the SamplesApp shell a moment to navigate and lay out the | ||
| // requested sample before we snapshot. | ||
| System.Threading.Thread.Sleep(TimeSpan.FromSeconds(3)); |
There was a problem hiding this comment.
Hardcoded 3-second sleep is fragile for CI and over-waits on fast machines.
On a slow CI agent the sample might not be ready in 3 s; on a fast machine you waste 3 s every run. The base class already has WaitForAutomationId which polls via WebDriverWait. Use it to wait for a known stable element from the target sample instead:
// Wait until a known root element of the sample is visible before snapshotting.
var wait = new OpenQA.Selenium.Support.UI.WebDriverWait(driver, TimeSpan.FromSeconds(15))
{
PollingInterval = TimeSpan.FromMilliseconds(250),
};
wait.IgnoreExceptionTypes(typeof(OpenQA.Selenium.NoSuchElementException));
wait.Until(d => d.FindElement(adapter.ByAutomationId("<known-root-element-id>")));If no stable root element ID exists for the sample yet, add one to the XAML. Fix this →
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
AutomationIdon macOS Skia through-accessibilityIdentifier/AXIdentifierand add a native update hook. - Add
SamplesApp.AppiumTestswith 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.
| var automationId = owner is not null | ||
| ? AutomationProperties.GetAutomationId(owner) | ||
| : null; | ||
| if (string.IsNullOrEmpty(automationId)) | ||
| { | ||
| automationId = peer.GetAutomationId(); | ||
| } | ||
| if (!string.IsNullOrEmpty(automationId)) | ||
| { | ||
| NativeUno.uno_accessibility_update_identifier(handle, automationId); | ||
| } |
| public By ByAutomationId(string automationId) | ||
| { | ||
| // Mac2's MobileBy.AccessibilityId maps to XCUIElement.identifier, which | ||
| // only works for elements XCUITest has fully snapshotted into its own | ||
| // element store. Custom NSAccessibilityElement subclasses (like Uno's | ||
| // UNOAccessibilityElement) are reachable via XPath but not always via | ||
| // the identifier shortcut, so we filter by the underlying AXIdentifier | ||
| // attribute that Uno publishes from AutomationProperties.AutomationId. | ||
| var escaped = automationId.Replace("'", "'"); | ||
| return By.XPath($"//*[@AXIdentifier='{escaped}']"); | ||
| } |
| _spawnedApp.Kill(entireProcessTree: true); | ||
| } | ||
|
|
||
| TerminateBundle(WrapperBundleId); |
| // TextBox template parts | ||
| "TextBox", | ||
| // Other framework-injected template parts that aren't sample content. |
| public string GetRole(IWebElement element) | ||
| { | ||
| // UIAutomation surfaces the control type as LocalizedControlType / ControlType. | ||
| var localized = element.GetAttribute("LocalizedControlType"); | ||
| return string.IsNullOrWhiteSpace(localized) ? string.Empty : localized.ToLowerInvariant(); | ||
| } |
| // We're not asserting a side-effect (the button is purely a target for | ||
| // visibility toggles by sibling buttons); the assertion is that | ||
| // Click() round-trips through the driver without throwing. | ||
| Assert.Pass(); |
There was a problem hiding this comment.
Assert.Pass() means this test can never fail — it verifies nothing.
The comment says "the assertion is that Click() round-trips without throwing." That is the implicit behavior of any unguarded call; NUnit will fail the test automatically if Click() throws. The explicit Assert.Pass() call is redundant and signals to readers that something was verified, which is misleading.
Either remove the Assert.Pass() and add a meaningful assertion (e.g. re-query the element's enabled state or check a side effect), or add a [Description] attribute that clearly documents what is being exercised so reviewers don't assume the absence of a side-effect check is an oversight. Fix this →
| <RollForward>Major</RollForward> | ||
| <Nullable>enable</Nullable> | ||
| <IsPackable>false</IsPackable> | ||
| <TreatWarningsAsErrors>false</TreatWarningsAsErrors> |
There was a problem hiding this comment.
TreatWarningsAsErrors=false and silent NoWarn additions.
TreatWarningsAsErrors=false is never set like this in other test projects in the repo — they inherit the CI gate from Directory.Build.props. Setting it to false here permanently weakens the warning policy for this project on CI.
NU1903/NU1902 are vulnerability-related NuGet warnings; suppressing them silently hides package-vulnerability alerts. If a specific package version is intentionally pinned below the advisory threshold, document that as a comment instead of suppressing globally.
CS1591 (missing XML doc) is fine to suppress in test projects, but prefer a #pragma warning disable CS1591 scoped at the file level, or omit altogether since test assemblies are not published.
Consider removing TreatWarningsAsErrors=false and addressing the NU190x warnings individually if they arise. Fix this →
| // the identifier shortcut, so we filter by the underlying AXIdentifier | ||
| // attribute that Uno publishes from AutomationProperties.AutomationId. | ||
| var escaped = automationId.Replace("'", "'"); | ||
| return By.XPath($"//*[@AXIdentifier='{escaped}']"); |
There was a problem hiding this comment.
XPath attribute injection: ' escaping is incomplete.
The current escaping only replaces ' with ':
var escaped = automationId.Replace("'", "'");
return By.XPath($"//*[@AXIdentifier='{escaped}']");An automationId containing " will break out of the XPath attribute context: @AXIdentifier='foo"bar' is valid XPath, but @AXIdentifier='"' terminates the outer string and mangles the query. A fully correct approach uses XPath concat() to handle both quote types:
// Simple case: no double-quote, use double-quoted attribute
if (!automationId.Contains('"'))
return By.XPath($"//*[@AXIdentifier=\"{automationId.Replace("'", "'")}\"]");
// Fallback: build concat() expression
var parts = automationId.Split('"');
var concat = string.Join(", '\"', ", parts.Select(p => $"'{p}'"));
return By.XPath($"//*[@AXIdentifier=concat({concat})]");In practice, AutomationIds won't contain quotes, but defensive correctness prevents surprise failures. Fix this →
| } | ||
| } | ||
|
|
||
| private static string Escape(string value) => value.Replace("\"", "\\\""); |
There was a problem hiding this comment.
CSS attribute selector injection: Escape only handles ", not ] or \.
private static string Escape(string value) => value.Replace("\"", "\\\"");A value containing ] terminates the CSS attribute selector early: [xamlname="foo]bar"] becomes [xamlname="foo] — the bar"] part is parsed as a tag selector. The CSS spec requires \ to escape ] and \ itself inside attribute selectors.
Minimal fix:
private static string Escape(string value)
=> value.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("]", "\\]");Again, developer-controlled AutomationIds are unlikely to contain these, but defensive correctness avoids obscure test failures. Fix this →
| /// </summary> | ||
| public static class SnapshotComparer | ||
| { | ||
| public static SnapshotDiff Compare(AccessibilityNode expected, AccessibilityNode actual) |
There was a problem hiding this comment.
Extras dict is silently excluded from comparison — this should be documented on the method.
AccessibilityNode.Extras is populated by all three adapters and serialized to JSON, but SnapshotComparer.Compare never touches it. A future maintainer reading the snapshot JSON will see extras present but won't understand why diffs on those fields never fire. Add a note to the Compare XML doc (or an inline comment at the call site) explaining the deliberate exclusion:
/// <remarks>
/// <see cref="AccessibilityNode.Extras"/> is intentionally excluded from the
/// comparison — it holds platform-specific raw attributes for debugging only
/// and should not gate snapshot equality.
/// </remarks>
|
🤖 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 |
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>
github-actions
Bot
added
platform/wasm 🌐
|
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
|
| exec ""{dotnetPath}"" ""{dllPath}"" | ||
| " | ||
| : $@"#!/bin/bash | ||
| exec ""{dotnetPath}"" ""{dllPath}"" ""{sampleQuery}"" |
There was a problem hiding this comment.
Medium: sampleQuery embedded in bash exec line without shell-escaping (fourth pass — still unaddressed).
A sampleQuery value containing ", $, or backtick produces a malformed bash script. For example, sample=Category/My"Test" becomes:
exec "/path/dotnet" "/path/app" "sample=Category/My"Test""The shell splits this as three separate tokens — the argument the app receives is wrong.
| exec ""{dotnetPath}"" ""{dllPath}"" ""{sampleQuery}"" | |
| static string BashSingleQuote(string s) => "'" + s.Replace("'", "'\\''") + "'"; | |
| var script = string.IsNullOrEmpty(sampleQuery) | |
| ? $@"#!/bin/bash | |
| exec ""{dotnetPath}"" ""{dllPath}"" | |
| " | |
| : $@"#!/bin/bash | |
| exec ""{dotnetPath}"" ""{dllPath}"" {BashSingleQuote(sampleQuery)} | |
| "; |
| } | ||
| if (!string.IsNullOrEmpty(automationId)) | ||
| { | ||
| NativeUno.uno_accessibility_update_identifier(handle, automationId); |
There was a problem hiding this comment.
Medium: Stale native identifier when AutomationId is cleared (fourth pass — still unaddressed).
uno_accessibility_update_identifier is only called when automationId is non-empty. If an element's AutomationId is later removed (set to "" or null), the native UNOAccessibilityElement keeps its old identifier indefinitely — Appium/XCUITest can then resolve the wrong element by the stale ID.
Call unconditionally so an empty string clears the identifier on the native side — the same pattern already used by uno_accessibility_update_help and uno_accessibility_update_description:
| NativeUno.uno_accessibility_update_identifier(handle, automationId); | |
| NativeUno.uno_accessibility_update_identifier(handle, automationId ?? string.Empty); |
| <RollForward>Major</RollForward> | ||
| <Nullable>enable</Nullable> | ||
| <IsPackable>false</IsPackable> | ||
| <TreatWarningsAsErrors>false</TreatWarningsAsErrors> |
There was a problem hiding this comment.
Medium: TreatWarningsAsErrors=false weakens the repo-wide CI gate; NU1902/NU1903 silences vulnerability alerts (fourth pass — still unaddressed).
Directory.Build.props sets the repo-wide warning policy. Overriding it per-project with false means this project's warnings never break CI, diverging from every other project in the solution.
NU1902 and NU1903 are NuGet's moderate/high-severity vulnerability warnings. Suppressing them globally means any future dependency audit will silently pass even when a known-vulnerable package version is pinned.
- Remove
TreatWarningsAsErrors=falseand let the root policy apply. - Remove
NU1902;NU1903fromNoWarn. If a specific transitive dependency triggers these today and cannot be upgraded, document the reason with an inline comment on the<PackageReference>entry instead of suppressing globally. CS1591(missing XML doc) is acceptable to suppress in a test assembly.
| @@ -1,4 +1,5 @@ | |||
| using Uno.Extensions; | |||
| using System; | |||
There was a problem hiding this comment.
Low: using System; is unused — CS8019 build failure on CI (fourth pass — still unaddressed).
Nothing in this file references any System type directly. With TreatWarningsAsErrors active on CI, this produces CS8019 and breaks the build.
| using System; | |
| using Uno.Extensions; |
| @@ -1,12 +1,19 @@ | |||
| using System; | |||
| using System.Reflection; | |||
There was a problem hiding this comment.
Low: using System.Reflection; is unused — CS8019 build failure on CI (fourth pass — still unaddressed).
No type from System.Reflection is referenced anywhere in this file. With TreatWarningsAsErrors active on CI, this produces CS8019 and breaks the main solution build.
| using System.Reflection; | |
| using System; |
| }; | ||
|
|
||
| private static string EscapeCssAttribute(string value) | ||
| => value.Replace("\\", "\\\\").Replace("\"", "\\\""); |
There was a problem hiding this comment.
Low: EscapeCssAttribute still does not escape ] — CSS attribute selector injection (fourth pass — still unaddressed).
A xamlautomationid value containing ] terminates the attribute selector early:
[xamlautomationid="foo]bar"] → [xamlautomationid="foo] bar"] (garbage)
The CSS spec requires \ and ] to be escaped inside attribute value strings.
| => value.Replace("\\", "\\\\").Replace("\"", "\\\""); | |
| private static string EscapeCssAttribute(string value) | |
| => value.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("]", "\\]"); |
| if (Session.Options.Platform != AppiumPlatform.Wasm) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"{nameof(WasmAccessibilityStandardsTests)} requires {AppiumTestOptions.EnvVarPlatform}=wasm."); |
There was a problem hiding this comment.
Low: Wrong-platform guard throws InvalidOperationException (test ERROR) instead of Assert.Inconclusive (test SKIP) (fourth pass — still unaddressed).
When this test runs against a non-WASM session, NUnit/MSTest records it as an unexpected ERROR rather than a deliberate skip, polluting the test report with noise.
| $"{nameof(WasmAccessibilityStandardsTests)} requires {AppiumTestOptions.EnvVarPlatform}=wasm."); | |
| Assert.Inconclusive($"{nameof(WasmAccessibilityStandardsTests)} requires {AppiumTestOptions.EnvVarPlatform}=wasm."); |
| } | ||
|
|
||
| return violations.sort().join('\n'); | ||
| """) as string; |
There was a problem hiding this comment.
Low: violations may be null — as string returns null when script returns non-string (fourth pass — still unaddressed).
ExecuteScript returns object?. If the script throws on the JS side or returns null, as string yields null. The .Should().BeEmpty() call on line 114 will then throw NullReferenceException with no useful diagnostic.
| """) as string; | |
| """) as string ?? string.Empty; |
| { | ||
| } | ||
|
|
||
| private static void AddIfTrue(IWebElement element, string attr, string pattern, List<string> sink) |
There was a problem hiding this comment.
Low: AddIfTrue is dead code — no call sites remain (fourth pass — still unaddressed).
This private helper has no call sites in the file after GetSupportedPatterns was refactored to inline the check. Remove it to avoid confusion.
| private static void AddIfTrue(IWebElement element, string attr, string pattern, List<string> sink) |
| { | ||
| if (_isDisposed) | ||
| var generation = ++_politeAnnouncementGeneration; | ||
| _politeDebounceTimer = new Timer( |
There was a problem hiding this comment.
Low: Timer created without disposing the prior instance — potential timer leak (fourth pass — still unaddressed).
_politeDebounceTimer is overwritten without calling ?.Dispose() first. Add defensive disposal before creating the new timer:
| _politeDebounceTimer = new Timer( | |
| private void SchedulePoliteAnnouncement(int delay) | |
| { | |
| var generation = ++_politeAnnouncementGeneration; | |
| _politeDebounceTimer?.Dispose(); | |
| _politeDebounceTimer = new Timer( | |
| _ => RunOnDispatcher(() => FlushPoliteAnnouncement(generation)), | |
| null, | |
| delay, | |
| Timeout.Infinite); | |
| } |
Apply the same pattern to ScheduleAssertiveAnnouncement (line ~890).
| var content = _pendingPoliteContent; | ||
| _pendingPoliteContent = null; | ||
| var generation = ++_politeGeneration; | ||
| _politeDebounceTimer = new Timer( |
There was a problem hiding this comment.
Low: Timer created without disposing the prior instance — same pattern as SkiaAccessibilityBase (fourth pass — still unaddressed).
_politeDebounceTimer is overwritten without ?.Dispose(). Add defensive disposal before assigning the new timer:
| _politeDebounceTimer = new Timer( | |
| private void SchedulePolite(int delay) | |
| { | |
| var generation = ++_politeGeneration; | |
| _politeDebounceTimer?.Dispose(); | |
| _politeDebounceTimer = new Timer( | |
| _ => NativeDispatcher.Main.Enqueue(() => FlushPolite(generation)), | |
| null, | |
| delay, | |
| Timeout.Infinite); | |
| } |
Apply the same pattern to ScheduleAssertive (line ~174).
| => GetOrCreateRelationshipCollection<UIElement>(element, ControlledPeersProperty, AutomationElementIdentifiers.ControlledPeersProperty); | ||
|
|
||
| internal static IList<UIElement> TryGetControlledPeers(DependencyObject element) | ||
| => element.GetValue(ControlledPeersProperty) as IList<UIElement>; |
There was a problem hiding this comment.
Low: TryGetControlledPeers may dereference a null element.
FrameworkElementAutomationPeer.GetControlledPeersCore calls this with Owner as UIElement, which can be null when Owner is not a UIElement. The current implementation would throw NullReferenceException on element.GetValue(...).
| => element.GetValue(ControlledPeersProperty) as IList<UIElement>; | |
| internal static IList<UIElement>? TryGetControlledPeers(DependencyObject? element) | |
| => element?.GetValue(ControlledPeersProperty) as IList<UIElement>; |
| allElements.forEach((el: HTMLElement) => { | ||
| if (el !== modalElement && !modalElement.contains(el)) { | ||
| let current: HTMLElement | null = modalElement; | ||
| while (current && current !== semanticsRoot) { |
There was a problem hiding this comment.
Nit: current && is always true at loop entry — redundant condition (fourth pass — still unaddressed).
current is assigned modalElement on line 35, and the outer if (semanticsRoot && modalElement) guard already ensures it is non-null at entry. At each iteration end, current = parent only executes after the if (!parent) { break; } guard fires — so current is never null when the condition is re-evaluated.
| while (current && current !== semanticsRoot) { | |
| while (current !== semanticsRoot) { |
| Directory.CreateDirectory(macOS); | ||
|
|
||
| const string executableName = "SamplesAppAppium"; | ||
| const string bundleId = "io.platform.uno.SamplesAppAppium"; |
There was a problem hiding this comment.
Low: const string bundleId duplicates the class-level WrapperBundleId constant (fourth pass — still unaddressed).
The literal "io.platform.uno.SamplesAppAppium" is already defined as WrapperBundleId (line 21). If one is updated the other silently diverges.
| const string bundleId = "io.platform.uno.SamplesAppAppium"; | |
| const string executableName = "SamplesAppAppium"; | |
| string bundleId = WrapperBundleId; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 85 out of 86 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Uno.UI/UI/Xaml/Controls/TextBox/TextBox.wasm.cs:2
using System;is unused in this file (no System types referenced). With code-style analyzers enabled in CI, unnecessary usings can fail the build; please remove it.
src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Automation/Given_AccessibilityAnnouncements.cs:3using System.Reflection;appears unused in this test file. Please remove it to avoid analyzer failures (unnecessary using directives).
src/Uno.UI.Runtime.Skia.WebAssembly.Browser/ts/Runtime/FocusTrap.ts:54inertis not supported uniformly across all browsers. Switching from the previoustabindex="-1"approach toinertonly can allow keyboard focus to escape the modal on browsers that ignoreinert(background semantic elements remain tabbable/click-focusable even though they’rearia-hidden). Consider feature-detectinginertand falling back to the priortabindex-based hiding (or another cross-browser focus-blocking strategy) wheninertisn’t available.
|
🤖 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 |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23755/docs/index.html |
|
The build 225242 found UI Test snapshots differences: Details
|
|
|
morning4coffe-dev
changed the title
|
Claude finished @morning4coffe-dev's task in 4m 58s —— View job Code Review (pass 6)
SummaryThis pass is a significant improvement over previous rounds. Every issue flagged across the five prior reviews has been addressed in the latest commits. All previously flagged issues are resolved ✅
New in this pass: Remaining issues (two)
The |
| element.SendKeys(value); | ||
| } | ||
|
|
||
| public string GetRole(IWebElement element) => GetAttributeAny(element, "LocalizedControlType", "ControlType") ?? string.Empty; |
There was a problem hiding this comment.
Low: LocalizedControlType is locale-dependent — baselines will diverge on non-English Windows hosts.
LocalizedControlType is tried first, which returns the human-readable control type in the OS UI language ("button" in English, "Schaltfläche" in German). Committed baselines recorded on an English Windows CI agent will fail on any other locale without any real accessibility regression.
ControlType returns the culture-invariant programmatic name (e.g. "ControlType.Button"). Swap the lookup order so it is preferred:
| public string GetRole(IWebElement element) => GetAttributeAny(element, "LocalizedControlType", "ControlType") ?? string.Empty; | |
| public string GetRole(IWebElement element) => GetAttributeAny(element, "ControlType", "LocalizedControlType") ?? string.Empty; |
This also requires adding ControlType.* entries to CanonicalRole.s_win32 (e.g. ["ControlType.Button"] = "button") and keeping the existing LocalizedControlType-keyed entries as case-insensitive fallbacks so existing English recordings do not immediately break.
| /// </summary> | ||
| public static class SnapshotComparer | ||
| { | ||
| public static SnapshotDiff Compare(AccessibilitySnapshot expected, AccessibilitySnapshot actual) |
There was a problem hiding this comment.
Nit: Extras intentionally excluded from comparison — document it.
AccessibilityNode.Extras is populated by all three adapters and serialized to the snapshot JSON, but Compare never inspects it. A future maintainer reading the snapshot file will see extras present and wonder why diffs on those fields never fire.
| public static SnapshotDiff Compare(AccessibilitySnapshot expected, AccessibilitySnapshot actual) | |
| /// <summary> | |
| /// Compares committed canonical snapshots by element id and reports explicit, | |
| /// actionable semantic diffs. <see cref="AccessibilityElementSnapshot.Extras"/> is | |
| /// intentionally excluded — it holds platform-specific raw attributes for debugging | |
| /// only and must not gate snapshot equality. | |
| /// </summary> | |
| public static SnapshotDiff Compare(AccessibilitySnapshot expected, AccessibilitySnapshot actual) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 90 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Uno.UI/UI/Xaml/Automation/AutomationProperties.cs:548
- These
TryGet*helpers can return null viaascasts, but their return types are non-nullable. This makes nullability analysis incorrect and hides the fact callers must handle the unset case.
src/Uno.UI/UI/Xaml/Automation/AutomationProperties.cs:435 TryGetControlledPeersuses anascast that can return null, but its return type is non-nullable. This makes the API contract misleading (especially in nullable-enabled builds) and contradicts theTryGet*naming convention.
This issue also appears on line 538 of the same file.
src/Uno.UI/UI/Xaml/Automation/Peers/PasswordBoxAutomationPeer.cs:92
MaskPasswordValuedefensively handles null (viapassword ?? string.Empty), but its parameter type is non-nullable. Making itstring?better reflects the intended contract and avoids nullable-analysis warnings at call sites like(Owner as PasswordBox)?.Password.
|
🤖 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 |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-23755/docs/index.html |
|
The build 225355 found UI Test snapshots differences: Details
|
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 ✅
Screenshots Compare Test Runresults.