fix(devicelab): re-sample the keyboard-blocking check until IME-resize geometry settles#127
Open
MarioRial22 wants to merge 2 commits into
Open
Conversation
…e geometry settles The keyboard-blocking guard added to the tap paths reads element and keyboard bounds exactly once, right after an input step. Windows with SOFT_INPUT_ADJUST_RESIZE (e.g. a plain AlertDialog whose body is a ScrollView) relayout a few frames after the IME appears or after typing: on that first frame the target still reports covered bounds, then the window shrinks and the target rises above the keyboard. The single-shot check reads the stale first frame and rejects a perfectly tappable element with "Element found but keyboard is covering it", suggesting a hideKeyboard step that is not needed (and on Android falls back to BACK, which apps may intercept). Real-world hit: tapOn android:id/button1 (dialog positive button) right after inputText — reported covered at centerY=1627 vs keyboard top 1560 on the first frame, tappable at ~1400 a few frames later. Worked on v1.1.19, regressed by the new guard. Fix: keep the guard's purpose (fail with the actionable hint when a tap would genuinely land on the IME) but re-sample element vs keyboard geometry every 50ms for up to 2s, returning the error only when the overlap PERSISTS through the whole window. Bail out early (proceed with the tap) as soon as the keyboard is dismissed or the element clears it. The verdict per sample is extracted to keyboardStillCovering and covered by tests with the real-world geometry, including both edges of the 50px suggestion-strip margin.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Extract the re-sampling loop into settleKeyboardBlocking with injected samplers so the settle behavior is testable without a live driver, and cover it end to end: element missing, clear on first frame, relayout lifting the element mid-settle, keyboard dismissed mid-settle, and a persistent overlap failing with the geometry hint, plus a pass through checkKeyboardBlocking with the mock client. No behavior change. Addresses the Codecov patch-coverage warning.
Contributor
|
Thanks @MarioRial22 — this is an excellent fix, and an important one: the guard it's correcting is a regression we introduced in v1.1.20, so this is squarely on us. The write-up is exemplary — precise root cause ( The implementation is clean, too:
I built and ran it locally — green. One thing before we merge — the same bug lives in the UIAutomator2 driver - I ll do it |
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
2 participants
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.
Summary
Since v1.1.20, the new keyboard-blocking guard in the tap paths (
checkKeyboardBlocking) samples element and keyboard geometry exactly once, immediately after an input step, and fails with:Windows with
SOFT_INPUT_ADJUST_RESIZE(any stockAlertDialogwith a scrollable body) relayout a few frames after the IME appears or after typing: on that first frame the target still reports covered bounds, then the window shrinks and the target rises above the keyboard (center Y 1627 → ~1400 in our trace). The single-shot check reads the stale first frame and rejects an element that is tappable an instant later. Our flows submitting a search dialog viatapOn: android:id/button1right afterinputTextpass on v1.1.19 and fail 3/6 CI shards on v1.1.20;hideKeyboardis not a usable workaround on Android (falls back to BACK, which apps may intercept).Same family as #100/#101/#103: a geometric guard rejecting a legitimately tappable element.
This PR keeps the guard's purpose — fail with the actionable hint when a tap would genuinely land on the IME — but re-samples element vs keyboard geometry every 50ms for up to 2s, returning the error only when the overlap persists through the whole window. It bails out early (proceeds with the tap) as soon as the keyboard is dismissed or the element clears it. True positives still fail fast (2s, well under the tap deadline) with the exact same message.
Type of Change
Changes Made
pkg/driver/devicelab/keyboard.go:checkKeyboardBlockingre-samples untilkeyboardSettleWindow(2s, package var so tests can shrink it) at a 50ms cadence; per-sample verdict extracted tokeyboardStillCovering.pkg/driver/devicelab/keyboard_test.go(new): table-driven tests for the per-sample verdict with the real-world repro geometry (covered at first frame, clear after relayout, keyboard dismissed mid-settle, both edges of the 50px suggestion-strip margin).Related Issues
Related: #100, #101, #103 (same subsystem; no open issue filed for this one — happy to open one if you prefer).
Testing
go test ./pkg/driver/devicelab/ -count=1green — the touched package. Fullgo test ./...in my environment shows 3 failing packages —pkg/device(TestFindFreePort_InvalidRange),pkg/reportandpkg/driver/browser/cdp— all of which fail identically on unmodifiedmainhere: environmental (sandboxed network / no real browser), unrelated to this change)golangci-lint run ./pkg/driver/devicelab/...— 0 issues in the touched files; the pre-existingerrcheckfindings inwebview.goare identical onmain)android:id/button1with the IME open — fail on v1.1.20, pass with this settle behavior, matching v1.1.19)Checklist
gofmt/go vetclean)docs(changelog): add 1.1.x release notes); happy to add a line if you'd like it in the PRAdditional Notes
No latency is added to the happy path: the first sample that finds the element clear (or the keyboard gone) returns immediately, so only genuinely-covered taps wait out the 2s window before failing.