Android: never walk an InteropAutomationPeer during accessibility traversal - #22123
Conversation
…versal
InteropAutomationPeer - the peer of a native interop control - throws
NotImplementedException from almost every member (GetOrCreateChildrenCore,
GetNameCore, GetBoundingRectangleCore, IsEnabledCore...), because it is meant to
be special-cased by each platform backend. macOS does so through
AvnAutomationPeer.IsInteropPeer, Windows through AutomationNode.InteropAutomationNode.
Android did not, and AvaloniaAccessHelper called those members without any guard.
Any accessibility traversal reaching a screen that hosts a native control - a
NativeControlHost, so a WebView or an embedded media surface - therefore threw from
inside an accessibility callback, which is not recoverable: the process goes down.
It is reproducible in one command on such a screen:
adb shell uiautomator dump
Reported by any accessibility client walking the tree, so a screen reader or an MDM
agent would take the same path.
Interop peers are now filtered at every point where a virtual view ID could be
handed out: the point hit test, the visible-views enumeration, the focused peer, and
the children walk. A virtual view ID is therefore never allocated for one, and
OnPopulateNodeForVirtualView never has to deal with one - which matters, because it
may not answer with an unpopulated node: ExploreByTouchHelper.createNodeForChild
rejects that and throws too.
Skipping is the correct behaviour here rather than merely the safe one: a native
control is a real Android View, already exposed to the accessibility framework on
its own. Presenting it a second time as an Avalonia virtual view would duplicate it.
InteropAutomationPeer is internal, hence the InternalsVisibleTo entry - Avalonia.Native
and Avalonia.Win32.Automation already have one for the same reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
We have a PR template can you use that? |
| } | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
This comment is not useful for any maintainer. Comments should not reflect issue context unless they are part of a unit test. This is not. No comment is needed.
There was a problem hiding this comment.
Removed, along with the one further down for the same reason. The diff is code only now, and the rationale lives in the pull request description instead.
I will do the same on #22122.
The rationale belongs in the pull request, not in the source.
|
Done — description rewritten against the template, and I have added how to reproduce it from Sorry for both, I should have looked for the template before opening. Applying the same to #22122. |
The rationale belongs in the pull request, not in the source. Only the two lines a reader cannot infer from the code are kept.
|
You can test this PR using the following package version. |
…UI#22122) * Android: never leave an accessibility node without text or content description `ExploreByTouchHelper.createNodeForChild` validates every virtual view the callback produces and throws Callbacks must add text or a content description in populateNodeForVirtualViewId() when the node carries neither. It throws from inside an accessibility callback, so the exception is not recoverable by the application: the process goes down. `TextUtils.isEmpty` treats `""` as empty, so an empty string does not satisfy the contract - only a non-empty value does. Two paths in `OnPopulateNodeForVirtualView` could produce such a node: * **A live peer with no accessible name.** `nodeInfo.Text ??= peer.GetName()` looks like it only assigns when nothing was set, but `AutomationPeer.GetName()` and `GetHelpText()` never return null - they collapse a missing value to `string.Empty` - so both assignments always run and can both assign `""`. This happens for any peer that is a pure container: a `Panel`, a `Border`, or the `TextSelectorLayer` that is added when a text selection starts. On a device with an accessibility service running, the first touch on a text field was enough to bring the application down. * **A stale virtual view id.** Since AvaloniaUI#22024 peers are unregistered when their control leaves the visual tree, and the platform can still ask for a node it obtained earlier - it caches them, and it re-queries the accessibility focused one. The lookup then fails and the node was left untouched, which the same validation rejects. Both are fixed by guaranteeing a non-empty content description. A single space is used deliberately: it satisfies the platform contract without inventing a label that screen readers would announce. Note that `AutomationPeer.GetClassName()` has the same `?? string.Empty` shape, so it cannot serve as the fallback. ### Testing `Avalonia.Android` builds clean. The crash paths need a device with an accessibility service enabled and are not reachable from the unit test projects; the fix was verified on hardware (Android 13 kiosk) where the first touch on a text field used to take the application down on every armed process. * Android: default an unlabelled node to its type name, and correct the attribution Review feedback: a single space only satisfies the platform contract, it does not describe anything, and it hides an unnamed control instead of surfacing it. The fallback is available - GetClassNameCore() is abstract and ControlAutomationPeer returns Owner.GetType().Name - so a peer always has a type name, which is also what nodeInfo.ClassName already carries. This also corrects a claim made in the first revision of this change. The androidx guard does not use TextUtils.isEmpty: in androidx.customview 1.1.0 and 1.2.0, createNodeForChild tests getText() and getContentDescription() for null, strictly, and the class contains no TextUtils reference at all (the isEmpty guard sits on the event path, which throws populateEventForVirtualViewId()). Since AutomationPeer.GetName() collapses a missing name to string.Empty, a node built for an unnamed container was therefore never rejected. Only the stale virtual view id path - reachable since AvaloniaUI#22024 unregisters peers on detach - produced a node the platform refuses. The placeholder there is now string.Empty, and the type name default is presented for what it is: an accessibility improvement, not a crash fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cut the explanatory comments down, per review on AvaloniaUI#22123 The rationale belongs in the pull request, not in the source. Only the two lines a reader cannot infer from the code are kept. * Drop the type name default, as requested in review Keeps this PR to the crash fix alone. The default label for an unnamed node is a separate discussion. --------- Co-authored-by: ronnycohen <19652995+ronnycohen@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…versal (AvaloniaUI#22123) * Android: never walk an InteropAutomationPeer during accessibility traversal InteropAutomationPeer - the peer of a native interop control - throws NotImplementedException from almost every member (GetOrCreateChildrenCore, GetNameCore, GetBoundingRectangleCore, IsEnabledCore...), because it is meant to be special-cased by each platform backend. macOS does so through AvnAutomationPeer.IsInteropPeer, Windows through AutomationNode.InteropAutomationNode. Android did not, and AvaloniaAccessHelper called those members without any guard. Any accessibility traversal reaching a screen that hosts a native control - a NativeControlHost, so a WebView or an embedded media surface - therefore threw from inside an accessibility callback, which is not recoverable: the process goes down. It is reproducible in one command on such a screen: adb shell uiautomator dump Reported by any accessibility client walking the tree, so a screen reader or an MDM agent would take the same path. Interop peers are now filtered at every point where a virtual view ID could be handed out: the point hit test, the visible-views enumeration, the focused peer, and the children walk. A virtual view ID is therefore never allocated for one, and OnPopulateNodeForVirtualView never has to deal with one - which matters, because it may not answer with an unpopulated node: ExploreByTouchHelper.createNodeForChild rejects that and throws too. Skipping is the correct behaviour here rather than merely the safe one: a native control is a real Android View, already exposed to the accessibility framework on its own. Presenting it a second time as an Avalonia virtual view would duplicate it. InteropAutomationPeer is internal, hence the InternalsVisibleTo entry - Avalonia.Native and Avalonia.Win32.Automation already have one for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Drop the explanatory comments, per review The rationale belongs in the pull request, not in the source. --------- Co-authored-by: ronnycohen <19652995+ronnycohen@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…UI#22122) * Android: never leave an accessibility node without text or content description `ExploreByTouchHelper.createNodeForChild` validates every virtual view the callback produces and throws Callbacks must add text or a content description in populateNodeForVirtualViewId() when the node carries neither. It throws from inside an accessibility callback, so the exception is not recoverable by the application: the process goes down. `TextUtils.isEmpty` treats `""` as empty, so an empty string does not satisfy the contract - only a non-empty value does. Two paths in `OnPopulateNodeForVirtualView` could produce such a node: * **A live peer with no accessible name.** `nodeInfo.Text ??= peer.GetName()` looks like it only assigns when nothing was set, but `AutomationPeer.GetName()` and `GetHelpText()` never return null - they collapse a missing value to `string.Empty` - so both assignments always run and can both assign `""`. This happens for any peer that is a pure container: a `Panel`, a `Border`, or the `TextSelectorLayer` that is added when a text selection starts. On a device with an accessibility service running, the first touch on a text field was enough to bring the application down. * **A stale virtual view id.** Since AvaloniaUI#22024 peers are unregistered when their control leaves the visual tree, and the platform can still ask for a node it obtained earlier - it caches them, and it re-queries the accessibility focused one. The lookup then fails and the node was left untouched, which the same validation rejects. Both are fixed by guaranteeing a non-empty content description. A single space is used deliberately: it satisfies the platform contract without inventing a label that screen readers would announce. Note that `AutomationPeer.GetClassName()` has the same `?? string.Empty` shape, so it cannot serve as the fallback. ### Testing `Avalonia.Android` builds clean. The crash paths need a device with an accessibility service enabled and are not reachable from the unit test projects; the fix was verified on hardware (Android 13 kiosk) where the first touch on a text field used to take the application down on every armed process. * Android: default an unlabelled node to its type name, and correct the attribution Review feedback: a single space only satisfies the platform contract, it does not describe anything, and it hides an unnamed control instead of surfacing it. The fallback is available - GetClassNameCore() is abstract and ControlAutomationPeer returns Owner.GetType().Name - so a peer always has a type name, which is also what nodeInfo.ClassName already carries. This also corrects a claim made in the first revision of this change. The androidx guard does not use TextUtils.isEmpty: in androidx.customview 1.1.0 and 1.2.0, createNodeForChild tests getText() and getContentDescription() for null, strictly, and the class contains no TextUtils reference at all (the isEmpty guard sits on the event path, which throws populateEventForVirtualViewId()). Since AutomationPeer.GetName() collapses a missing name to string.Empty, a node built for an unnamed container was therefore never rejected. Only the stale virtual view id path - reachable since AvaloniaUI#22024 unregisters peers on detach - produced a node the platform refuses. The placeholder there is now string.Empty, and the type name default is presented for what it is: an accessibility improvement, not a crash fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cut the explanatory comments down, per review on AvaloniaUI#22123 The rationale belongs in the pull request, not in the source. Only the two lines a reader cannot infer from the code are kept. * Drop the type name default, as requested in review Keeps this PR to the crash fix alone. The default label for an unnamed node is a separate discussion. --------- Co-authored-by: ronnycohen <19652995+ronnycohen@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
What does the pull request do?
Stops the Android accessibility backend from walking
InteropAutomationPeer, the peer of a nativeinterop control (
NativeControlHost).That peer throws
NotImplementedExceptionfrom almost every member —GetOrCreateChildrenCore,GetNameCore,GetBoundingRectangleCore,IsEnabledCore— because it is meant to be special-casedby each platform backend. macOS does that through
AvnAutomationPeer.IsInteropPeer, Windows throughAutomationNode.InteropAutomationNode. Android did not, andAvaloniaAccessHelpercalled thosemembers with no guard.
What is the current behavior?
Any accessibility traversal of a screen hosting a
NativeControlHost— a WebView, an embedded mediasurface — throws from inside an accessibility callback. That is not recoverable: the process goes
down.
Reproduced on an Android 13 device, four times in a row, on a screen with a WebView:
uiautomatoris only the cheapest trigger — any accessibility client walking the tree takes the samepath, so a screen reader or an MDM agent does too.
What is the updated/expected behavior with this PR?
The same traversal completes, and the native control is left to the accessibility framework, which
already exposes it as the real Android
Viewthat it is.To test: run
samples/ControlCatalog.Android, open the Native Embed page, enable anaccessibility service, then
adb shell uiautomator dump. Before this change the app dies; after it,the dump completes.
How was the solution implemented (if it's not obvious)?
Interop peers are filtered at the four places where a virtual view ID can be handed out: the point
hit test, the visible-views enumeration, the focused peer, and the children walk. A virtual view ID
is therefore never allocated for one, and
OnPopulateNodeForVirtualViewnever receives one.Filtering at allocation rather than at population is deliberate.
OnPopulateNodeForVirtualViewmaynot simply return early either:
ExploreByTouchHelper.createNodeForChildrejects a node carryingneither text nor content description and throws in turn — that is #22122. Guarding inside the
populate callback would have traded one crash for another.
Skipping is also the semantically correct answer, not just the safe one: presenting a native control
a second time as an Avalonia virtual view would duplicate a node the platform already publishes.
InteropAutomationPeerisinternal, hence theInternalsVisibleToentry —Avalonia.NativeandAvalonia.Win32.Automationalready have one for the same reason.Checklist
service running and a native control on screen.
left undocumented as requested in review.
— not applicable.
Breaking changes
None.
Obsoletions / Deprecations
None.
Fixed issues
Touches the same file as #22122 but not the same lines; either merge order is fine.