fix(android): stop empty focusable overlays from hiding app content - #1737
Conversation
…1733) The covered-subtree pruner let any `hittable` sibling condemn a lower drawing-order sibling it geometrically covers. `hittable` is `clickable ?? focusable`, so a childless, textless, id-less full-screen focusable View qualified as "agent-visible content" and dropped the sibling holding the real UI. Telegram wraps every screen in exactly such a View, which is why `snapshot` returned 1 node on every Telegram screen while the helper and stock `uiautomator dump` both saw the full tree — the loss was in the TS parser, not the Android helper. Key covering candidacy on `clickable` instead: focusability is an accessibility-traversal property and says nothing about painting over a sibling. Also generalise the existing "never condemn a marker leaf" exemption to any childless, non-clickable sibling that carries its own text or identifier, since drawing order plus geometry cannot distinguish a transparent overlay from an opaque one.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Reviewed exact head 39b9352. P1: preserve hittable when classifying foreground descendants. The helper emits clickable/focusable attributes only when true, so a focusable-only node parses as clickable undefined but hittable true. Changing hasActionableDescendant from child.hittable to child.clickable means a higher drawing-order surface containing focusable-only controls no longer qualifies as covering; covered background selector/ref targets can be retained and acted on, especially in Android TV/D-pad UIs. The Telegram fix needs to exclude the empty focusable wrapper itself, not discard focusable descendants. Keep descendant classification on the established hittable contract (or distinguish the wrapper another way) and add a helper-shaped regression where an omitted-clickable/focusable foreground descendant still suppresses a covered lower target. CI and Telegram/API36–37 evidence are otherwise strong. |
Review feedback on #1733: narrowing hasActionableDescendant from `hittable` to `clickable` was over-reach. The helper emits clickable/focusable only when true, so a focusable-only control parses as clickable=undefined, hittable=true — and D-pad/TV surfaces are built almost entirely from those. A foreground surface containing them would have stopped qualifying as covering, leaving background selector/ref targets retained and actionable behind it. Descendant classification returns to the established `hittable` contract, and the leaf exemption returns to `!hittable` so a genuinely covered affordance stays condemned. The Telegram fix needs only to exclude the empty focusable wrapper itself, which is self-classification in hasOwnAgentVisibleContent — that stays on `clickable`. Replaces the test that asserted the rejected behavior with a helper-shaped regression: a foreground descendant carrying focusable without any clickable attribute must still suppress a covered target.
|
Fixed in c87f621 — you were right, and the TV/D-pad case is the one that would have bitten.
What stays on Replaced the test that asserted the rejected behavior with the helper-shaped regression you asked for: foreground descendant carrying Re-ran the 32-screen A/B: still 0 screens losing content, all non-Telegram screens byte-identical. Re-verified live on API 37 — selector-driven |
|
Reviewed exact head c87f621. The prior focusable-descendant P1 is fixed: only empty covering-node self-classification narrows to clickable, while descendant coverage and the leaf exemption remain on hittable. The helper-shaped regression is load-bearing and preserves Android TV/D-pad foreground occlusion; Telegram and overlapped-label routes remain correct. Exact-head Telegram/API37 evidence plus the 32-screen A/B sweep is strong, all authoritative checks are green, and GitHub is CLEAN/MERGEABLE. No actionable finding remains; ready-for-human. |
|
Supersedes my prior clean verdict on c87f621. P1: the occlusion model remains encoding-dependent. Please normalize this at the owning helper/parser boundary so omitted-false and explicit-false encodings cannot change semantics. If focusability is evidence for TV/D-pad occlusion, carry and use |
…g them `hittable: clickable ?? focusable` collapsed two independent Android facts and made behavior depend on how a producer encodes a false attribute. The snapshot helper omits `clickable`; stock UiAutomator writes `clickable="false"`. For one focusable control those encodings gave opposite answers — verified: identical trees pruned differently and projected opposite `hittable` values. No comment can hold an invariant the representation contradicts, which is what the previous two commits tried to do. The tree now stores `clickable` and `focusable` as separate non-optional booleans, and every decision reads a named predicate: isTouchTarget, isFocusTarget, isAgentTarget, hasSemanticContent, hasDirectOcclusionEvidence, hasDescendantOcclusionEvidence, isPresentationLeaf. `canCoverSibling` consumes one derived classification (hasOcclusionEvidence) rather than choosing between raw attributes, so the self-versus-descendant substitution that caused the last review round is no longer expressible. Removing `hittable` from the tree type made the typechecker find every construction site; the public field is derived once at projection from isAgentTarget. Behavior change: a focusable, non-clickable control encoded by stock UiAutomator now projects hittable=true and participates in occlusion, matching what the helper backend already did for the same control. The Android TV test asserted the old encoding-specific value; it now asserts inclusion plus the unified projection. One encoding-parity regression replaces the patch-specific test added last round.
|
Revised in cc0c7a0. You were right that the comments were load-bearing where the model should have been, and the encoding divergence is real — I reproduced it before changing anything: Same logical tree, opposite pruning and opposite public projection. So this was never just a pruner bug — the projection was already encoding-dependent on main. What changed. The tree stores Most of the comments are gone; the file is down to 12 comment lines total. One behavior change worth your call. A focusable, non-clickable control encoded by stock UiAutomator now projects One encoding-parity regression replaces the patch-specific test I added last round (net −1 test). Re-ran the 32-screen A/B: still 0 screens losing content, all non-Telegram screens byte-identical. Live re-verified the full onboarding flow on API 37. Gates green, changed-line coverage 12/12. |
|
Reviewed exact head cc0c7a0. The prior encoding-dependent P1 is fixed holistically: omitted false attributes and explicit Merge readiness is still gated on the pending checks. Nonblocking PR-body drift: update “Two files touched” to three, and describe the semantic-leaf exemption as applying to non-agent-target leaves rather than every non-clickable leaf. |
|
Summary
On Telegram for Android,
snapshotreturned a single node and the "sparse accessibility snapshot"hint on every screen. The app was completely unautomatable. Same for any app that wraps its screens
in an empty focusable overlay View.
The issue reported this as the helper failing to descend past depth 4. It isn't — the Android helper
and stock
uiautomator dumpboth return the full tree. The loss was in the TypeScript parser'scovered-subtree pruner.
pruneAndroidCoveredSubtreeslets a sibling condemn a lower drawing-order sibling it geometricallycovers, gated on the covering node having "agent-visible content". That gate accepted any
hittablenode, and
hittableisclickable ?? focusable. Telegram wraps every screen in a childless,textless, id-less full-screen
android.view.Viewwithfocusable="true"and a higher drawing orderthan the content container — so it qualified, and took the entire 37-node UI subtree with it.
Two changes:
clickablerather thanhittable. Focusability is anaccessibility-traversal property; it says nothing about painting over a sibling. A node that
announces nothing and does nothing cannot condemn one that does.
any childless, non-clickable sibling carrying its own text or identifier. Drawing order plus
geometry cannot tell a transparent overlay from an opaque one, and exempting a leaf is bounded —
it can never resurrect a covered surface.
Before / after on the Telegram phone-number screen:
Closes #1733.
Validation
Live, API 37 emulator (Android 17), Telegram 12.9.2, helper
android-helperv0.20.8. Reproducedthe 1-node snapshot first, then drove the onboarding flow end-to-end through the built CLI on the
fixed build:
press 'text="Start Messaging"'and a follow-uppress @e6both resolved and tapped byselector, and the phone-number screen exposes both
EditTexts with correct roles. Sessions closed.Isolated root-cause proof. Ran the helper instrumentation directly and captured its XML: 46
Telegram nodes to depth 15, all text present. Feeding that exact XML to
parseUiHierarchyTreegave 6nodes; gating out
pruneAndroidCoveredSubtreesgave 45. That localises the loss to the parser andnames the pruner, independent of any device timing.
Regression sweep across 33 real screens, captured from both a live API 36 and a live API 37
emulator, then replayed through the pre-fix and post-fix parsers on identical input in both
interactiveandrawmodes: Telegram (3 onboarding screens), the repo's Expo Router test apprunning against Metro (5 tabs, bottom-sheet modal with dimming scrim, drill-in detail, native alert),
Bluesky and the Expo dev-launcher, Settings, Clock, Files, Messages, Play Store, Photos, Chrome,
Calendar, Contacts, and a permission dialog.
nothing to the full list).
Google Messages, is a legitimate prune and it still fires post-fix, dropping the same sibling.
Four new parser tests. Three fail against pre-fix code (
3 failed | 26 passed); the fourth pins theboundary that must not move — a genuinely clickable leaf behind a foreground sibling is still
condemned — and passes both ways by design.
Local gates green, including
check:affectedandcheck:fallow.Notes and follow-ups
captured RN screens, including the bottom-sheet modal — Expo Router puts those in separate
accessibility windows. That path is covered by the pre-existing unit test, which still passes, but
the live sweep does not independently exercise it.
Telegram's phone-number field, via
fill,type, or coordinate taps on its in-app keypad. Thefield focuses but stays empty. Unrelated to this change — it was simply unobservable before, since
there were no refs to target. Worth its own issue.