fix(drivers): scrollUntilVisible accepts malformed (negative-area) rects that tapOn rejects

[MarioRial22]

Follow-up to the discussion in #101.

Problem

scrollUntilVisible declares success and stops scrolling, then a tapOn on the same element fails with a deterministic signature:

✓ scrollUntilVisible id=…/extintoresInstalacionParte (729ms)
✗ tapOn              id=…/extintoresInstalacionParte (12.0s)
   element rect not tappable (w=810 h=-26 center=(675,2287) screen=1080x2340)

Note h=-26: the element's bounds are a malformed rect with top > bottom (negative height). Reproduces on the devicelab driver against a button at the bottom of a ScrollView that is still below the fold when the scroll loop stops. uiautomator2 taps the same button fine.

Root cause

The scroll-stop predicate and the tap predicate disagree on what counts as a valid on-screen rect:

• boundsTappable (used by tapOn) rejects non-positive dimensions — the [BUG] Android DeviceLab: FindAndClick taps off-screen centre from a malformed negative-height rect (bottom-sheet first frame) #94 malformed-rect guard:

  if b.Width <= 0 || b.Height <= 0 { return false }

• isElementOnScreen (used by scrollUntilVisible) only rejected exactly zero:

  if b.Width == 0 || b.Height == 0 { return false }

A clipped below-the-fold ScrollView child is reported with top > bottom (raw [270,2300][1080,2274] → h=-26, centre (675,2287)). isElementOnScreen accepts it — it isn't exactly zero, and it numerically "overlaps" the viewport — so scrollUntilVisible short-circuits to success without ever scrolling the button into view. tapOn then correctly refuses the same degenerate rect via the #94 guard and re-polls to the deadline.

This is independent of the usable-vs-physical height fix in #101: the inverted rect comes from clipping, not from the screen-size value, so it reproduces even when isElementOnScreen is given the physical height (as it now is post-#101 via tappableScreenSize()).

Fix

Make isElementOnScreen reject non-positive width/height, matching boundsTappable's #94 guard. The scroll loop then keeps going until the element has a real, on-screen rect, and the following tapOn succeeds. This stays a defensive Go-side guard in the same spirit as #101 — no magic tolerance — so the #94 malformed-rect rejection is now consistent on both the tap side and the scroll side.

Applied to both the devicelab and uiautomator2 drivers: the predicate is duplicated and identical in each. Only the devicelab path was observed failing in the field; the uiautomator2 edit has no independent repro but keeps the two predicates consistent and is unambiguously correct (a negative-area rect is never visible).

Tests

• New regression cases in both TestIsElementOnScreen tables (pkg/driver/devicelab, pkg/driver/uiautomator2): negative width and negative height (clipped …), the latter the exact field repro {X:270, Y:2300, Width:810, Height:-26}. Each must return false; against the old == 0 predicate they return true, so they fail without the fix.
• go build ./..., go vet ./pkg/driver/devicelab/ ./pkg/driver/uiautomator2/, and go test ./pkg/driver/devicelab/ ./pkg/driver/uiautomator2/ all pass.
• End-to-end on a local API 33 arm64 emulator: a flow that scrolls to a bottom-anchored ScrollView button and taps it now passes 21/21 (tapOn 1.3s vs the prior 12s deadline).

[omnarayan]

Thanks @MarioRial22 — clean follow-up, and this closes exactly the predicate gap I had in mind. I traced the repro and it checks out: isElementOnScreen accepted the clipped h=-26 rect (not exactly zero, and it overlaps the viewport numerically), so
scrollUntilVisible declared success without bringing the button into view, and then tapOn correctly refused the same degenerate rect via the #94 guard and re-polled to the deadline. Good call separating this from #101 — the inverted rect comes from
clipping, not the screen-size value, so it's a genuinely independent bug.

The fix is the right one: making isElementOnScreen reject non-positive width/height so the scroll-stop predicate agrees with boundsTappable. A negative-area rect is never actually visible, so there's nothing legitimate this newly excludes — worst case
the loop keeps scrolling instead of falsely succeeding, which is what we want.

A few things I appreciated:

• Applying it to both devicelab and uiautomator2 to keep the two predicates consistent, and being upfront that only devicelab had a field repro while the uiautomator2 edit is correct-by-construction.
• The regression cases in both tables — especially the exact field rect {270, 2300, 810, -26} — which fail against the old == 0 predicate, so they're real guards.
• Tight diff, no unrelated churn.

Looks good to me — merging. Thanks again for the thorough write-up and the e2e numbers.