perf+fix(ios): faster text entry (readiness + typed-query field resolve) + fix fill mis-navigation#633
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6509d0d356
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isKeyboardVisible(app: app) { | ||
| return latest |
There was a problem hiding this comment.
On iOS when the software keyboard is already visible from a previous text field, this branch is true immediately after tapping a different field, before XCUITest has necessarily moved focus to the new target. Because typeTextReliably then uses app.typeText(...), the text can be sent to the previously focused field instead of the requested target during back-to-back fills; the old wait avoided this by giving the focus/tap state time to settle. Only take this fast path when the keyboard was not already visible for another target, or after confirming the target/focus changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The isKeyboardVisible early-exit in both stabilizeTextInputBeforeTyping and waitForTextEntryReadiness is now gated on a keyboard hidden→visible TRANSITION (shared keyboardBecameVisible(wasVisibleAtEntry:)). When the keyboard is already up (back-to-back fills) it falls back to the prior settle/timeout, so text isn't sent to the previously-focused field; the fresh case keeps the fast path. Also fixed a related secure-field bug the review surfaced: clearTextInput used editableTextValue(...) ?? "" and skipped clearing secure fields (editableTextValue returns nil there) → replace concatenated; now distinguishes nil (clear) from "" (skip). Device-validated.
thymikee
changed the title
thymikee
changed the title
The XCUITest text-entry focus/readiness loops keyed their fast-exit on focusedTextInput(), which is intentionally hardcoded to return nil on iOS (focus predicates are stale there). As a result stabilizeTextInputBeforeTyping always burned its full focusTimeout (0.4s) and waitForTextEntryReadiness burned its full readinessTimeout (2.0s) in the normal case where the software keyboard appears — ~2.4s of dead wait before a single keystroke on every type/fill. The software keyboard becoming visible is the reliable iOS readiness signal, so both loops now return as soon as isKeyboardVisible() is true. The warmup-first-char echo check and post-type verify/repair remain as drop safety nets. Measured on iPhone 17 sim (Settings search field), median type time: 25 chars: 3342ms -> 1379ms (2.4x) 52 chars: 3969ms -> 2190ms 313 chars: 10.3s -> 8.6s (remainder is genuine per-char XCUITest typing) Reliability unchanged: 64/65 trials exact (incl. a 50-word lorem ipsum, verified by read-back + screenshot); the lone miss triggered the existing verify/repair.
…igation) clearTextInput unconditionally ran moveCaretToEnd (an edge-tap computed from the element frame) + a 24-key delete burst, even when the field was empty. On a field that repositions on focus — e.g. the Settings search bar jumping bottom->top and revealing a 'Suggestions' list — that edge-tap used a stale frame and landed on an adjacent row (Developer), navigating away instead of clearing. fill (replace) into the search field went to the Developer pane (0/3 correct). Skip the clear entirely when the field's value is already empty (placeholder treated as empty): replacing into an empty field is a no-op, and skipping avoids the stray edge-tap. fill into the Settings search now types correctly and stays put: 5/5 exact (read-back + screenshot).
…ration textInputAt used app.descendants(.any).allElementsBoundByIndex (snapshots EVERY element) to find the text input at a point. fill drove this repeatedly: once it has coordinates, resolveTextEntryElement re-runs textInputAt on every verify/repair poll iteration whenever the focused-field reference goes stale (e.g. the Settings search bar repositioning bottom->top), so the full-tree enum dominated fill latency. Query the text-input element types directly (app.textFields/secureTextFields/ searchFields/textViews) instead. Same matches, but XCUITest resolves typed queries without snapshotting the whole tree. Measured (iPhone 17 sim, warm runner): fill 25 chars ~14.5s -> ~4.5s (3.2x), 6/6 exact. Same primitive #632 killed for get text.
Review P1 (focus race): the isKeyboardVisible early-exit in stabilizeTextInputBeforeTyping and waitForTextEntryReadiness fired the instant the keyboard was visible — but when it was ALREADY up from a previous field (back-to-back fills), that is before first-responder moves to the newly-tapped field, so app.typeText could target the old field. Gate the fast-path on a keyboard hidden->visible TRANSITION via a shared keyboardBecameVisible(wasVisibleAtEntry:) helper; when the keyboard was already up, fall back to the settle/timeout (the prior, correct behavior) instead of the ~2.4s dead wait the fresh case avoids. Review P1 (F2): clearTextInput used editableTextValue(...) ?? "" and skipped clearing on empty — but editableTextValue returns nil for secure (and unknown) fields, so secure fields were NEVER cleared and replace concatenated stale+new. Distinguish nil (clear) from "" (skip). Device-validated: fresh fill fast-path preserved + exact; a second fill with the keyboard already up still types into the correct field and replaces (not concatenates).
thymikee
force-pushed
the
perf/ios-text-entry
branch
from
346d2eb to
296364f
Compare
|
1 participant
Three iOS XCUITest text-entry fixes, each found and verified on-device (read-back of the field
value+ screenshots). Implements Wave-2 item #4 of the iOS perf plan.Summary of measured impact (iPhone 17 sim, Settings search)
type25 charstype50-word lorem ipsum (313 ch)fill25 chars (warm runner)fillinto Settings search1. perf: early-exit text-entry readiness on keyboard-visible (~2.4s/op)
Focus/readiness loops keyed their fast-exit on
focusedTextInput(), which is intentionallynilon iOS. SostabilizeTextInputBeforeTypingalways burnedfocusTimeout(0.4s) andwaitForTextEntryReadinessburnedreadinessTimeout(2.0s) whenever the keyboard appeared. Both now return as soon asisKeyboardVisible()(the real readiness signal). Warmup-first-char + verify/repair safety nets unchanged; reliability 64/65 exact incl. the 50-word lorem ipsum.2. fix: don't clear an already-empty field (fixes
fillmis-navigation)clearTextInputalways ranmoveCaretToEnd(an edge-tap from the element frame) + a 24-key delete burst, even on an empty field. On the Settings search bar (repositions bottom→top on focus, revealing a "Suggestions" list), the stale-frame edge-tap landed on the Developer suggestion → navigated away (fillwas 0/3 correct). Skip the clear entirely when the value is already empty (placeholder treated as empty) — a no-op semantically, and it removes the stray tap.3. perf: resolve text fields via typed queries, not full-tree enumeration (
fill3.2× faster)This is why
fillwas ~14.5s whiletypewas ~1.6s (it is not repair churn or "app busy" — verified).textInputAtusedapp.descendants(.any).allElementsBoundByIndex(snapshots every element).filldrove it repeatedly: once it has coordinates,resolveTextEntryElementre-runstextInputAton every verify/repair poll when the focused-field reference goes stale (search bar repositioning). Switched to typed queries (app.textFields/secureTextFields/searchFields/textViews) — same matches, no whole-tree snapshot (the same primitive #632 fixed forget text).fill25 chars warm: ~14.5s → ~4.5s, 6/6 exact.Diagnosis notes (for reviewers)
open/relaunch (one-time iOS-runner startup ~10s + a per-relaunch first-AX-query settle ~4s).typemeasured fast only because apresspreceded it;fillmeasured slow because it was that first interaction. (feat: e2e command perf benchmark harness + nightly CI #630's harness now runs an untimed warmup after each open so benchmarks don't charge this to a measured command.)fill(~4.5s) is now close topress+type(~3s). Further headroom: plan item #4b — resolve the text-entry element once per verify/repair window instead of per poll.Validation
Per trial:
open --relaunch→ focus →type/fill <string>(timed) → read fieldvaluefromsnapshot --raw --json, compare byte-for-byte → screenshot. 50-word lorem ipsum rendered exactly in the "No Results for …" header. Swift-only; needsbuild:xcuitest:ios. Branched offmain.