Android: fix crash on a stale accessibility virtual view id - #22122
Conversation
…scription
`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.
|
The question is why we don't enforce a description or name. It should be at least the type name by default. |
|
You can test this PR using the following package version. |
… 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>
ronnycohen
changed the title
|
You're right, and it is the better fix: a space only satisfies the platform contract, it describes While checking that, I have to correct my own PR description. I claimed the guard used That changes the attribution. Since One question before this goes further: should that default live in the Android backend, or in |
The rationale belongs in the pull request, not in the source. Only the two lines a reader cannot infer from the code are kept.
|
Applied your two points from #22123 here as well: description rewritten against the PR template, and the explanatory comments cut down to the two lines a reader cannot infer from the code. The rest of the rationale is in the description. The design question from my previous comment still stands whenever you get to it — Android backend or |
|
Any further comments need to come from a human. We're able to interact with agents by ourselves, thank you. |
Got it, sorry. Next time I'll write them myself. |
Yes please, let's have the fix now and discuss the name change elsewhere.
Thank you |
Keeps this PR to the crash fix alone. The default label for an unnamed node is a separate discussion.
ronnycohen
changed the title
|
You can test this PR using the following package version. |
MrJul
added
bug
os-android
backport-candidate-12.1.x
…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>
MrJul
added
backported-12.1.x
and removed
backport-candidate-12.1.x
4 participants
What does the pull request do?
Fixes a crash on Android when the accessibility framework asks for a virtual view whose peer is no longer registered.
What is the current behavior?
ExploreByTouchHelper.createNodeForChild rejects a node whose text and content description are both null, and throws: "Callbacks must add text or a content description in populateNodeForVirtualViewId()"
OnPopulateNodeForVirtualView returns without touching the node when the id is not found, so androidx gets an empty node and throws. It throws inside an accessibility callback, so the app cannot catch it and the process dies.
That path became reachable with #22024, which unregisters a peer when its control leaves the visual tree. The platform caches nodes it obtained earlier and re-queries the accessibility focused one, so a lookup can fail after a detach.
What is the updated/expected behavior with this PR?
An unknown id gets an inert node: empty description, disabled, not focusable, empty bounds. The traversal completes instead of killing the process.
How was the solution implemented (if it's not obvious)?
The guard in androidx.customview 1.1.0 and 1.2.0 is a strict null check, so string.Empty satisfies it.
Checklist
accessibility service running.
Breaking changes
None.
Obsoletions / Deprecations
None.
Fixed issues
The type name default discussed earlier has been removed from this PR.