fix: Raise WinUI-faithful ChildAdded/ChildRemoved UIA events#23599
fix: Raise WinUI-faithful ChildAdded/ChildRemoved UIA events#23599morning4coffe-dev wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return provider; | ||
| } | ||
|
|
||
| var peer = element.GetOrCreateAutomationPeer(); |
There was a problem hiding this comment.
Medium — peer created as side-effect in a "TryGetExisting" method
GetOrCreateAutomationPeer() can create a new automation peer on an element that is about to be removed from the tree. The callers of this method (only OnChildRemoved today) want to capture the existing runtime ID, not trigger peer construction. The method name TryGetExistingProviderForElement and the comment just above it ("Only an already-existing provider is meaningful here") both signal this intent.
If a non-creating accessor exists (e.g. a GetAutomationPeer() that returns an existing peer or null), use it here. If no such accessor exists, document why GetOrCreate is safe on a departing element, and consider renaming to GetProviderForElement to avoid the misleading contract.
| var peer = element.GetOrCreateAutomationPeer(); | |
| var peer = element.GetOrCreateAutomationPeer(); |
(Replace with a non-creating accessor if one is available, or add a comment explaining the intended semantics.)
There was a problem hiding this comment.
Pull request overview
This PR updates the Skia/Win32 UIA provider implementation to raise WinUI-faithful StructureChanged events, specifically emitting ChildAdded and ChildRemoved (and bulk/coarse variants) instead of always falling back to ChildrenInvalidated, and gates the more expensive event/provider work behind a “clients are listening” check.
Changes:
- Added
UiaClientsAreListening()interop to detect whether UIA clients are subscribed. - Reworked structure-change coalescing to track Added/Removed/Invalidated and flush WinUI-faithful event types (including bulk thresholds).
- Updated child add/remove handling to raise ChildAdded on the added element and ChildRemoved on the container with the removed child’s runtime id.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/Uno.UI.Runtime.Skia.Win32/Accessibility/Win32UIAutomationInterop.cs |
Adds UiaClientsAreListening() P/Invoke used to gate structure event work. |
src/Uno.UI.Runtime.Skia.Win32/Accessibility/Win32Accessibility.cs |
Implements WinUI-faithful ChildAdded/ChildRemoved structure event queuing, grouping, and flushing (with bulk/coarse fallbacks). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -396,36 +528,103 @@ private void RaiseStructureChanged(Win32RawElementProvider provider) | |||
| { | |||
| _structureChangeFlushQueued = false; | |||
|
|
|||
| // Short-circuit the flush if the window was closed while this callback | |||
| // was queued (edge case "Window close during dispatch"). | |||
| // Short-circuit the flush if the window was closed while this callback was queued. | |||
| if (IsDisposed) | |||
| { | |||
| _pendingStructureChanges.Clear(); | |||
| return; | |||
| } | |||
|
|
|||
| foreach (var pending in _pendingStructureChanges) | |||
| FlushStructureChanges(); | |||
| }); | |||
| // Matches WinUI's AP_BULK_CHILDREN_LIMIT: more than this many add/remove on one container in a | ||
| // single coalescing window collapses to a single ChildrenInvalidated/ChildrenBulk* event. | ||
| private const int BulkChildrenLimit = 20; |
| var ancestorProvider = FindNearestAncestorProvider(parent); | ||
| if (ancestorProvider is not null) | ||
| if (ancestorProvider is null) | ||
| { | ||
| return; | ||
| } |
| var ancestorProvider = FindNearestAncestorProvider(parent); | ||
| if (ancestorProvider is not null) | ||
| if (ancestorProvider is null) | ||
| { | ||
| RaiseStructureChanged(ancestorProvider); | ||
| return; | ||
| } |
| if (group.Invalidated || total > BulkChildrenLimit) | ||
| { | ||
| RaiseStructureChangedCore(container, StructureChangeType.ChildrenInvalidated, null); | ||
| continue; | ||
| } | ||
|
|
||
| if (group.Added.Count >= BulkChildrenLimit) |
There was a problem hiding this comment.
Minor — inconsistent bulk threshold operator
The combined total guard uses > BulkChildrenLimit (strict: > 20), but the per-type guards below use >= BulkChildrenLimit (at least 20). This means a container with exactly 20 adds and 0 removes skips the ChildrenInvalidated path and falls through to the ChildrenBulkAdded path — which may or may not match WinUI's AP_BULK_CHILDREN_LIMIT semantics.
The constant comment says "more than this many", implying >, yet the per-type branches use >=. Align the operators and/or add a note explaining the deliberate asymmetry if it's intentional:
// Too many changes (or an explicit coarse request) -> a single ChildrenInvalidated.
if (group.Invalidated || total > BulkChildrenLimit)
{
RaiseStructureChangedCore(container, StructureChangeType.ChildrenInvalidated, null);
continue;
}
if (group.Added.Count >= BulkChildrenLimit) // >= here but > above — document intent
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23599/wasm-skia-net9/index.html |
|
|
|
Claude finished @morning4coffe-dev's task in 5m 3s —— View job Review complete
SummaryThe second commit (
The core logic (ChildAdded on the entering element, ChildRemoved on the container with the child's runtime ID, bulk thresholding, coalesced dispatcher flush) is correct and faithful to WinUI semantics.
Inline finding: The Cross-cutting concernsMissing runtime tests (unchecked in PR checklist)
Empty "What changed?" section in PR description |
| continue; | ||
| } | ||
|
|
||
| if (group.Added.Count >= BulkChildrenLimit) |
There was a problem hiding this comment.
Minor — deliberate >= / > asymmetry deserves an inline note
The total guard (line 579) uses strict > (total > BulkChildrenLimit), while the per-type guards here use >=. The comment on BulkChildrenLimit was improved in commit 2 to describe "per-type count that reaches this emits Bulk*", which documents the intent — but the single-character difference between > and >= across these three checks is easy to misread as a typo, especially since the constant name and the old comment both said "more than this many". A short annotation at the >= sites would cement it:
| if (group.Added.Count >= BulkChildrenLimit) | |
| if (group.Added.Count >= BulkChildrenLimit) // deliberate >=: exactly BulkChildrenLimit → Bulk* (not Invalidated) |
Same applies at line 598 for group.Removed.Count >= BulkChildrenLimit.
| } | ||
| } | ||
|
|
||
| if (group.Removed.Count >= BulkChildrenLimit) |
There was a problem hiding this comment.
Same >= note as line 585 — a brief comment here would help confirm this is intentional:
| if (group.Removed.Count >= BulkChildrenLimit) | |
| if (group.Removed.Count >= BulkChildrenLimit) // deliberate >=: exactly BulkChildrenLimit → Bulk* (not Invalidated) |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23599/wasm-skia-net9/index.html |
GitHub Issue: part of https://github.com/unoplatform/kahua-private/issues/493
PR Type:
🐞 Bugfix
What changed? 🚀
PR Checklist ✅
Screenshots Compare Test Runresults.