fix(ios): keyboard-dismiss content settle race (#1542) — partial, defect 2 needs a decision - #1559
Conversation
#1542) Dismissing the keyboard can trigger the app's own ScrollView content-offset correction (e.g. releasing the inset it grew to keep a focused field above the keyboard). That correction is a separate, unsynchronized animation that `keyboard.waitForNonExistence` knows nothing about — the keyboard AX element can disappear well before the app visually settles. The very next command is frequently a synthesized, AX-free drag (scroll/gesture, kept AX-free so it still works under #1105-family AX degradation), which has no XCTest quiescence wait of its own, so it can land mid-animation and net to zero — the "scroll does nothing" symptom on the Form screen's checkout-form.ad leg. Add a bounded, AX-free screenshot-stability wait to dismissKeyboard() so the runner only returns once the screen has actually stopped changing (or a generous cap elapses). The stopping decision is a pure function (runnerScreenshotStabilitySettled) covered by unit tests under AGENT_DEVICE_RUNNER_UNIT_TESTS; the surrounding capture/sleep loop is the thin, untestable I/O shell around it. Live-verified on iPhone 17 Pro / iOS 26.2: the scroll now visually lands at the correct position (confirmed via screen-recording frame correlation) instead of leaving content at its pre-scroll offset. Not a full fix for #1542: the checkout-form.ad corpus leg still fails at the same step, now because the daemon's shared post-gesture snapshot stabilization (src/daemon/post-gesture-stabilization.ts) can read a stale-but-internally-consistent AX tree after the AX-free scroll and mistake "unchanged across polls" for "settled", so the following click's off-screen guard sees pre-scroll node positions. That is a cross-platform, cross-command stabilization semantics change and needs a design decision, not a unilateral fix here — see the PR description.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
Review finding:\n\n- [P2] Execute the new screenshot-stability regression in CI. RunnerTests+Keyboard.swift adds six runnerScreenshotStabilitySettled tests behind AGENT_DEVICE_RUNNER_UNIT_TESTS, but .github/workflows/ios.yml uses an explicit -only-testing list that selects none of them. The other runner gate is build-for-testing, so the new logic can regress while every check remains green. Add at least one representative method (or the relevant group) to the targeted iOS test-without-building invocation.\n\nThe production route and scoped defect-1 fix otherwise look sound, and the live evidence supports that partial result. The separate stale-AX defect remains explicitly out of scope, so this PR should not close #1542 as a whole. |
|
Addressed the P2 finding: the six Confirmed the env var actually reaches the tests first (otherwise the selection would be vacuous): Local counterfactual (
Closing-linkage check: Gates: Pushed as Generated by Claude Code |
thymikee
added
the
ready-for-human
|
Re-review at 9d86785: clean and ready for human merge review. The updated iOS workflow selects all six new screenshot-stability tests in the actual test-without-building invocation; the job enables their compile flag, and exact-head CI shows all six executing and passing (16 RunnerTests total, 0 failures). The prior P2 is fully resolved, all checks are green, and no new actionable issue was found. The separate stale-AX defect remains intentionally out of scope. |
|
1 participant
Summary
#1542 describes two defects blocking the iOS
checkout-form.adreplay corpus leg (and #1478 P5's evidence):scroll down 0.6reports success but the Form screen's ScrollView content doesn't move.dismiss-overlayfails.This PR fixes defect 1's root cause and verifies the fix live. It does not make the corpus green — investigating defect 1 surfaced a second, distinct staleness bug in a shared cross-platform module that is now the sole remaining blocker. That one needs a design decision, so per the task brief I'm stopping short of unilaterally changing shared stabilization semantics and reporting it here instead.
Root cause (defect 1, fixed)
Dismissing the keyboard (
keyboard dismiss, which taps the field's "Done" key) triggers the app's own ScrollView content-offset correction — releasing the inset it grew to keep the focused field above the keyboard. That correction is a separate, unsynchronized UIKit/RN animation thatkeyboard.waitForNonExistence(the only thingdismissKeyboard()waited on) knows nothing about: the keyboard's AX element can disappear well before the screen visually settles.The very next command in the fixture is
scroll down 0.6, which — like all iOS scroll/gesture commands — uses the AX-free synthesized drag lane (XCSynthesizedEventRecord/XCPointerEventPath, bypassingXCUIElement) specifically so scrolling keeps working when XCTest can't serialize the accessibility tree (the #1105/#758 AX-degradation lineage). That lane has no XCTest quiescence wait of its own. So the drag can land mid-animation, and its intended net displacement gets absorbed into stopping the residual content-offset correction instead of adding to it — the "scroll does nothing" symptom.Evidence:
xcrun simctl io recordVideocapture correlated frame-by-frame against the daemon's per-request timing (ios_runner_command_sendspans withdurationMs) shows the ScrollView jump to a wildly over-scrolled position during thekeyboardDismisswindow, well before thescrollcommand even executes — confirming the content is still animating when the drag lands. Reproduced deterministically (5/5 fresh-boot runs) viapnpm build && pnpm clean:daemon && node bin/agent-device.mjs replay examples/test-app/replays/checkout-form.ad --platform ios.Fix
apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Keyboard.swift: after the keyboard's AX element vanishes,dismissKeyboard()now polls screenshots (AX-free, same philosophy as the synthesized gesture lane) and only returns once 3 consecutive samples are byte-identical (or a 2s cap elapses). Requiring 3 matches — not 2 — is deliberate: a spring-driven correction can pass through a near-zero-velocity inflection that two adjacent samples can't distinguish from true rest.The stopping decision (
runnerScreenshotStabilitySettled) is extracted as a pure function and covered by unit tests underAGENT_DEVICE_RUNNER_UNIT_TESTS(5 cases: needs-enough-samples, matches-in-window, mid-window-mismatch, failed-capture never counts, only-looks-at-trailing-window). The capture/sleep loop around it is the thin, untestable I/O shell.Scoped to iOS (
#elsebranch ofdismissKeyboard(), tvOS untouched); no cross-platform command contract or shared TS module touched.Live validation (defect 1)
scroll down 0.6moves ScrollView contentgesture-lab.ad(shared runner code, regression check)Video captures and correlated frame extracts, request logs, and JSON failure payloads for every run (fresh-boot repros before the fix, and the fix verification) are preserved outside the worktree at
/private/tmp/ad-1542-artifacts/on the machine this was developed on (not included in the diff — this is evidence, not a repo asset).Root cause (defect 2, NOT fixed — needs a decision)
With defect 1 fixed,
checkout-form.adstill fails at the same step (click id="shipping-pickup"→ "resolved to an off-screen element"), but the mechanism is now different and the on-screen content is actually correct at failure time (verified via video: "Pickup" is visibly on-screen at the moment of the reported failure).src/daemon/post-gesture-stabilization.tsmarks scroll/swipe actions for stabilization, then the next snapshot capture polls up to ~1.5s, comparing consecutive AX-derived "interaction surface signatures," and treats two matching consecutive reads as proof the screen settled. That assumption breaks specifically for the AX-free gesture lane: XCTest's AX tree isn't proactively resynced by a synthesized touch (unlike a normalXCUIElement.tap()), so it can serve a stale-but-internally-consistent tree for a window after the scroll — every poll matches the previous one, "stabilization" declares victory almost immediately (attempt 2, ~200ms in), and the click's off-screen guard then evaluates the pre-scroll node positions. Confirmed via the failure's ownsnapshotDiagnostics: captures are fast (p95 ~230ms,xctestbackend, no slow/deferred flag) — this is stale-but-fast, not literally slow AX, which is why bumping timeouts wouldn't help; the loop exits on match, not on budget exhaustion.This module is shared across iOS/Android (
supportsPostGestureStabilizationgates onisMobilePlatform, not a specific OS), so a correct fix changes cross-platform, cross-command stabilization semantics — e.g., cross-checking AX-signature stability against a screenshot signal, or treating "post-gesture signature identical to the pre-gesture baseline" as a "keep polling, don't trust this as final" signal rather than as proof of stability. Per the task brief, that's a design decision I'm not making unilaterally here.Options for defect 2 (for discussion, not implemented)
runnerScreenshotStabilitySettledpattern this PR introduces, but on the daemon/TS side and for a different trigger.Any of these touches the shared stabilization module and needs sign-off before implementation.
Gates run
pnpm typecheck,pnpm lint,pnpm format:check— clean (no TS touched by this PR; run to confirm no incidental breakage)pnpm build:xcuitest:ios—** TEST BUILD SUCCEEDED **gesture-lab.ad— 2/2 clean fresh-boot runs (regression check on the shared Swift file)checkout-form.ad— still fails at step 11, now due to defect 2 (documented above), not defect 1Test plan
checkout-form.adshould pass 2/2 fresh-boot runspnpm test-app:replay:iosexpecting bothcheckout-form.adandgesture-lab.adgreen