fix: exclude descendants when finding a clear tap point (#199) - #435
Open
8crsk wants to merge 1 commit into
Open
fix: exclude descendants when finding a clear tap point (#199)#4358crsk wants to merge 1 commit into
8crsk wants to merge 1 commit into
Conversation
Element indices are assigned by a pre-order walk of the accessibility tree
(IndexedFormatter._flatten_with_index), so every descendant of an element has a
higher index than the element itself.
get_clear_point treats any higher-indexed overlapping element as something
drawn on top of the target. Descendants match that test, but a descendant is
painted *inside* its ancestor rather than over the top of it. The result is
that a container whose children happen to fill it is reported as obscured and
cannot be tapped at all.
This is an ordinary Android layout, not a corner case -- a clickable row
holding an icon and a label:
LinearLayout index=5 bounds=0,100,1000,300 <- the tappable row
ImageView index=6 bounds=0,100,200,300
TextView index=7 bounds=200,100,1000,300
Before: get_clear_point(5) raised
"Element 5 is fully obscured by overlapping elements"
After: get_clear_point(5) returns (500, 200), the centre of the row.
Skipping descendants is enough to fix it. Ancestors are already excluded by the
existing index comparison, since an ancestor always has a lower index, and
genuine overlays -- a snackbar, dialog or FAB drawn after the target -- keep
their higher index and are still treated as blockers.
Also corrects the docstring, which promised a fallback to the centre that the
implementation has never done; it raises instead. Raising is the right
behaviour for an element that really is covered, so the docstring was the part
that was wrong.
Adds tests/test_clear_point_nesting.py covering nested children, deeply nested
descendants, partial and full overlays, ancestor exclusion, and elements
missing an index or bounds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #199.
The report says overlapping elements cause incorrect clicking and that the tree
needs to respect hierarchy. Here's the specific mechanism, and it's a bit worse
than mis-clicking: any container whose children fill it can't be tapped at all.
Why it happens
IndexedFormatter._flatten_with_indexassigns indices with a pre-order walk, soevery descendant ends up with a higher index than its ancestor.
UIState.get_clear_pointtreats any higher-indexed overlapping element assomething drawn on top of the target:
Descendants satisfy that test. But a descendant is painted inside its ancestor,
not over the top of it, so an element's own children get treated as obstructions
and
find_clear_pointruns out of room.Reproduction
An ordinary clickable row with an icon and a label, not a corner case:
The two children exactly cover the parent.
Before:
get_clear_point(5)raisesElement 5 is fully obscured by overlapping elementsAfter:
get_clear_point(5)returns(500, 200), the centre of the rowTapping the leaf children (6, 7) worked before and still works, which is probably
why this survived. It only bites when the agent targets the container, and the
container is often the only node carrying the click handler.
The fix
Skip descendants when building the blocker list. That's enough:
higher index and are still treated as blockers.
I also corrected the docstring, which promised a fallback to the centre that the
implementation has never done. It raises instead. Raising seems right for an
element that really is covered, so I assumed the docstring was the wrong half.
Happy to flip it if you'd rather have the documented behaviour.
Tests
tests/test_clear_point_nesting.py, 10 cases: nested children, deeply nesteddescendants, a partial overlay the point has to dodge, a full overlay that should
still raise, an overlay over a container that also has children, ancestor
exclusion, and elements missing an index or bounds.
I checked the tests actually catch the bug. Three of them fail against current
main, and the seven covering overlay behaviour pass both before and after, soexisting behaviour is unchanged.
Full suite: 653 passed. Four tests fail on my machine
(
test_grok_oauth,test_manager_prompt_selection,test_visual_remote_connection) but they're a pre-existing Windowscp1252decode issue and fail identically on a clean checkout of
main.ruffandblackare clean.