From 8ee12037777cce11ed1bf4c09671924720fd72d4 Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Wed, 17 Jun 2026 19:24:22 +0200 Subject: [PATCH] fix(drivers): reject malformed (negative-area) rects in isElementOnScreen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scrollUntilVisible's success predicate (isElementOnScreen) only rejected zero-area bounds (Width == 0 || Height == 0), not negative ones. A clipped below-the-fold ScrollView child is reported by findElement with top > bottom — a negative-height rect (raw [270,2300][1080,2274] => h=-26, centre (675,2287)). isElementOnScreen accepted it as "on screen", so scrollUntilVisible short-circuited to success without ever scrolling the element into view. tapOn then correctly rejected the same rect via boundsTappable's #94 guard (Width <= 0 || Height <= 0) and re-polled to the deadline: ✓ scrollUntilVisible id=…/extintoresInstalacionParte (729ms) ✗ tapOn id=…/extintoresInstalacionParte (12.0s) element rect not tappable (w=810 h=-26 center=(675,2287) screen=1080x2340) Align isElementOnScreen with boundsTappable: treat non-positive width/height as off-screen so the scroll loop keeps going until the element has a real, on-screen rect. This is independent of the usable-vs-physical height fix in PR #101 — the inverted rect comes from clipping, not from the screen-size value, so it reproduces even against the physical height. Fixed in both the devicelab and uiautomator2 drivers (shared, duplicated predicate; only the devicelab path was observed failing in the field, but the predicate is identical and must reject the same degenerate rects). Regression cases added to both TestIsElementOnScreen tables, including the field repro (w=810 h=-26). Verified end-to-end on a local API 33 arm64 emulator: a previously-failing extinguisher-list flow now passes 21/21 (tapOn 1.3s vs the prior 12s deadline). --- pkg/driver/devicelab/commands.go | 9 +++++++-- pkg/driver/devicelab/commands_test.go | 7 +++++++ pkg/driver/uiautomator2/commands.go | 6 ++++-- pkg/driver/uiautomator2/commands_test.go | 4 ++++ 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/pkg/driver/devicelab/commands.go b/pkg/driver/devicelab/commands.go index b0bb789..5ecd4cd 100644 --- a/pkg/driver/devicelab/commands.go +++ b/pkg/driver/devicelab/commands.go @@ -845,10 +845,15 @@ func (d *Driver) scrollByAdb(direction string, screenWidth, screenHeight int, pe const scrollDurationMs = 300 // isElementOnScreen reports whether an element's bounds overlap the visible -// viewport. Zero-area bounds count as off-screen. +// viewport. Malformed bounds — non-positive width/height, e.g. a clipped +// below-the-fold ScrollView child reported with top>bottom (negative height) — +// count as off-screen, matching boundsTappable's #94 guard. Without this, +// scrollUntilVisible short-circuits to success on a degenerate rect that tapOn +// then refuses to tap ("element rect not tappable"), looping to the deadline +// instead of scrolling the element fully into view. func isElementOnScreen(info *core.ElementInfo, screenWidth, screenHeight int) bool { b := info.Bounds - if b.Width == 0 || b.Height == 0 { + if b.Width <= 0 || b.Height <= 0 { return false } return b.X+b.Width > 0 && b.X < screenWidth && b.Y+b.Height > 0 && b.Y < screenHeight diff --git a/pkg/driver/devicelab/commands_test.go b/pkg/driver/devicelab/commands_test.go index 8f69225..6da0ec4 100644 --- a/pkg/driver/devicelab/commands_test.go +++ b/pkg/driver/devicelab/commands_test.go @@ -180,6 +180,13 @@ func TestIsElementOnScreen(t *testing.T) { {"entirely right of screen", core.Bounds{X: 1080, Y: 100, Width: 200, Height: 200}, false}, {"zero width", core.Bounds{X: 100, Y: 100, Width: 0, Height: 200}, false}, {"zero height", core.Bounds{X: 100, Y: 100, Width: 200, Height: 0}, false}, + {"negative width", core.Bounds{X: 100, Y: 100, Width: -50, Height: 200}, false}, + // Repro from the field: a clipped below-the-fold ScrollView button the + // agent reports with top>bottom (raw [270,2300][1080,2274] → h=-26, + // centre (675,2287)). Overlaps the viewport numerically, but is a + // degenerate rect tapOn refuses (boundsTappable's #94 guard); the scroll + // success predicate must reject it too so the loop keeps scrolling. + {"negative height (clipped below-fold)", core.Bounds{X: 270, Y: 2300, Width: 810, Height: -26}, false}, } for _, tt := range tests { diff --git a/pkg/driver/uiautomator2/commands.go b/pkg/driver/uiautomator2/commands.go index 054248a..8700ebe 100644 --- a/pkg/driver/uiautomator2/commands.go +++ b/pkg/driver/uiautomator2/commands.go @@ -523,10 +523,12 @@ func (d *Driver) scrollByAdb(direction string, screenWidth, screenHeight int, pe } // isElementOnScreen reports whether an element's bounds overlap the visible -// viewport. Zero-area bounds count as off-screen. +// viewport. Malformed bounds (non-positive width/height — e.g. a clipped rect +// with top>bottom) count as off-screen, so scrollUntilVisible keeps scrolling +// instead of declaring success on a degenerate rect. func isElementOnScreen(info *core.ElementInfo, screenWidth, screenHeight int) bool { b := info.Bounds - if b.Width == 0 || b.Height == 0 { + if b.Width <= 0 || b.Height <= 0 { return false } return b.X+b.Width > 0 && b.X < screenWidth && b.Y+b.Height > 0 && b.Y < screenHeight diff --git a/pkg/driver/uiautomator2/commands_test.go b/pkg/driver/uiautomator2/commands_test.go index 744db0e..2270172 100644 --- a/pkg/driver/uiautomator2/commands_test.go +++ b/pkg/driver/uiautomator2/commands_test.go @@ -4180,6 +4180,10 @@ func TestIsElementOnScreen(t *testing.T) { {"entirely above screen", core.Bounds{X: 100, Y: -300, Width: 200, Height: 200}, false}, {"zero width", core.Bounds{X: 100, Y: 100, Width: 0, Height: 200}, false}, {"zero height", core.Bounds{X: 100, Y: 100, Width: 200, Height: 0}, false}, + {"negative width", core.Bounds{X: 100, Y: 100, Width: -50, Height: 200}, false}, + // Malformed clipped rect (top>bottom → negative height): degenerate, must + // count as off-screen so scrollUntilVisible keeps scrolling. + {"negative height (clipped)", core.Bounds{X: 270, Y: 2300, Width: 810, Height: -26}, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {