Skip to content

fix(webdriver): replace text in the confirmed focused element - #3990

Open
bheemreddy-samsara wants to merge 3 commits into
callstack:mainfrom
bheemreddy-samsara:fix/webdriver-fill
Open

bheemreddy-samsara wants to merge 3 commits into
callstack:mainfrom
bheemreddy-samsara:fix/webdriver-fill

Conversation

@bheemreddy-samsara

@bheemreddy-samsara bheemreddy-samsara commented Oct 27, 2026 •

Copy link
Copy Markdown

Summary

Addresses #3985 across 11 files. WebDriver fill clears/replaces the confirmed focused element; empty fill clears, failed clear stops input, and type stays append-only. Missing references refuse. No e2e freshness changes.

Workflow

flowchart LR
  F[fill] --> R[Confirm focused element]
  R --> C[Clear same element]
  C -->|failure| E[Error]
  C -->|nonempty| V[Replace same element]
  C -->|empty| D[Done]
  T[type] --> A[Append]
Loading

Validation

Head: 9d92f13eb4447668a7984cabfcbad66617428d57; rebased base: 0b49de4dfd092a86c0d99d5fcb3dcd976d6ccc9b (includes #3997).

  • Five regressions: base 4 failed/1 passed; head 5 passed.
  • 110 unit + 18 provider/facade passed; early-clear mutation detected; docs passed.
  • Affected receipt run-SUt414, exactCommit:true: format/lint/types/layering/Fallow/build passed; related 2020 passed/1 failed, base 2008/1, same Apple fake-record timeout. Gate failed.
  • Cubic clean; CI failed (web setup, 120s); Apple pending; Vercel authorization-required.

Hosted proof — unmodified head

Public Swag Labs 2.7.1, My Demo App 2.3.0 and Selenium fixtures; dummy data.

Native iPhone17/iOS26.6 + GalaxyS25/Android15: replacement/repeat/empty, existing secure replacement and neighbor isolation passed. Fixture login confirmed exact secure text. Android plain WebView replacement/repeat/empty passed; iOS rerun blocked by focus-read deadline.

Existing SDK wait stable 1000 10000; text-entry budgets unchanged. Native 8 clear/7 value each; Android WebView 4/3, excluding URL setup. HTTP200, same confirmed IDs, zero session POST /keys; empty sent no value.

Normalized iOS UTC, 2026-10-28 (intervening reads omitted):

15:33:55.105 GET /element/active -> A
15:33:55.607 POST /actions
15:33:56.869 GET /element/active -> A
15:33:57.696 POST /element/A/clear
15:33:58.361 POST /element/A/value "new"
15:34:01.267 AX="new"

Unproven: live rejected-clear/WebView-password. Historical iOS readonly/zoom mis-target remains. All four head sessions closed.

@bheemreddy-samsara
bheemreddy-samsara marked this pull request as ready for review October 27, 2026 12:29

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files

Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack

View guided diff | Re-trigger cubic

Comment thread packages/provider-webdriver/src/webdriver-interactor.test.ts Outdated
Comment thread packages/provider-webdriver/src/webdriver-interactor.test.ts Outdated
Comment thread packages/provider-webdriver/src/webdriver-client.test.ts
Comment thread test/integration/provider-scenarios/cloud-webdriver-runtime.test.ts
Comment thread packages/provider-webdriver/src/webdriver-interactor.ts
@bheemreddy-samsara
bheemreddy-samsara marked this pull request as draft October 27, 2026 13:04
@thymikee

Copy link
Copy Markdown
Member

Thanks for the fix. At af38a20 the code looks right, but the fix is not proven on a real grid yet.

Fill now depends on the grid honouring POST /element/{id}/clear and /element/{id}/value for the id from GET /element/active (webdriver-interactor.ts#L250). Every test uses a fake server that clears by assignment and appends with +=. Real WDA and UiAutomator2 differ by element type: WDA clear may refuse or misbehave on secure fields and WebView inputs, and UiAutomator2 /value may append or replace. This interactor also serves hosted Android. Issue #3985 asks for live hosted-device verification, and the PR body says it is still pending. Without it, fill may still append, fail, or erase the wrong content. Please attach a live hosted run through the daemon fill route (BrowserStack iOS, ideally one hosted Android too) with the wire transcript (GET /element/active, tap, /clear, /value, no session /keys) and the post-fill field value for these cases: fill @input "new" on a field holding old leaves exactly new; a second fill with new2 leaves exactly new2; an empty fill leaves the field empty with no /value request; a secure field with an existing value; filling the second field of an open form leaves the first untouched; and a rejected clear returns an error with the old value intact and no /value request.

Not blocking, and you can take or leave these: keyboard-shown has no producer now, yet interactor-types.ts#L154 and CLOUD_TEXT_ENTRY_READINESS still list it, and the Interactor.fill doc near L365 still says to see the webdriver interactor for empty text. The keyboard-only fallback in awaitTextEntryReadiness (L660) can no longer succeed, so the rule could be that fill needs a focused element reference and refuses with one error before tapping when /element/active is unsupported, with the helper returning a named {elementId} result. Focus confirmation still accepts moved || rectContains(...) (L637), so a tap that hands focus to another element would now clear that element. I could not build a concrete case, so is this worth guarding if the live run shows one? The unit world in webdriver-interactor.test.ts ignores the element id, so recording it there would assert ordering and binding in one place.

The open inline threads on keyboard-shown (r4388700192) and the three P3 test-tightening notes (r4388700160, r4388700180, r4388700188) still stand. Two threads do not apply: failed clear is covered through the daemon route by the integration test "sends no replacement and does not retry a rejected clear" (#3990 (comment)), and the element id binding is asserted at the wire by the fake's id routing and webdriver-client.test.ts (#3990 (comment)).

I ran no tests and observed no live device behaviour. The only reported check is Cubic, which passed. Fork workflows await maintainer approval, so no unit or integration job has run on this head. Before merge, please add the live hosted run above, then get fork CI approved.

@bheemreddy-samsara

Copy link
Copy Markdown
Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@bheemreddy-samsara I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 11 files

View guided diff | Re-trigger cubic

@thymikee

Copy link
Copy Markdown
Member

The earlier requests from af38a20 are mostly fixed at 13d32da, but one still needs a live run before this can merge. The earlier review is at #3990 (comment).

Fill now clears and then sets the value on the element returned by GET /element/active (webdriver-interactor.ts#L250). Every test for this uses a fake grid that clears by assignment and appends with +=. Real WDA and UiAutomator2 differ by element type: secure fields, WebView inputs, and whether /value appends or replaces. I did not see a hosted run or a transcript after the earlier request, and #3985 says live hosted verification is still needed. Without it, fill may still append, fail on secure or WebView fields, or clear the wrong content on the exact hosted iOS route that was reported. Please attach one live BrowserStack iOS run, and ideally one Android run, through the daemon fill route. The transcript should show GET /element/active, tap, POST /element//clear, POST /element//value and no session POST /keys, plus the field value after each case: old to "new" leaves exactly new; a second fill with "new2" leaves exactly new2; an empty fill leaves the field empty with no /value request; a secure field with an existing value; a second field in an open form leaves the first untouched; a rejected clear returns an error, keeps the old value and sends no /value request.

Not blocking, take or leave: keyboard-shown is still in CLOUD_TEXT_ENTRY_READINESS (interactor-types.ts#L154) as legacy with two daemon tests, but nothing in the repo produces it and no released result is cited, so consider dropping it or naming the result that needs it; the Interactor.fill doc near L365 also still says a backend refuses empty text "see the webdriver interactor", which is no longer true.

Of the inline threads, the cubic-dev-ai P3 on keyboard-shown (#3990 (comment)) is the same note as above. These are fixed or covered at 13d32da and can be resolved: the P2 on a failed clear is covered through the daemon route by the "sends no replacement and does not retry a rejected clear" test (#3990 (comment)); the P2 on interactor mocks now asserts clear then send on "tapped" (#3990 (comment)); the P3 on the webdriver-client rejects now passes /provider unavailable/ (#3990 (comment)); the P3 on call order now asserts clear before value and the /value body (#3990 (comment)); the P3 on the iOS text-entry test now asserts COMMAND_FAILED, status 503 and the clear rejection (#3990 (comment)).

Checks: only cubic has reported, and it passes. The fork workflows are waiting for maintainer approval, so unit and integration jobs have not run on this head. The PR body reports two failures in the Apple fake record and Android fake app-log tests; they look unrelated to the webdriver route by path, but I did not re-run them. I did not run tests, and the 4 failed and 1 passed base comparison is the author's claim, unverified. I could not build a case where the moved or rectContains check clears the wrong element; only a live run would show one. No conflicts were reported. Before merge, please attach the live hosted run with the transcript and field values above, and get the fork CI approved.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 11 files

View guided diff | Re-trigger cubic

@vercel

vercel Bot commented Oct 28, 2026

Copy link
Copy Markdown

@bheemreddy-samsara is attempting to deploy a commit to the Callstack Team on Vercel.

A member of the Team first needs to authorize it.

@bheemreddy-samsara

Copy link
Copy Markdown
Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

@cubic review

@bheemreddy-samsara I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 11 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread packages/provider-webdriver/src/webdriver-interactor.test.ts
@bheemreddy-samsara

Copy link
Copy Markdown
Author

@thymikee Can you do a quick review ?

@thymikee

Copy link
Copy Markdown
Member

Thanks for the update. At c8d3888 the earlier findings are fixed, and I have no blocking code findings. The new tests cover the rejected clear, the clear-before-value order and the typed 503 error.

Not blocking, and you can take or leave these: nothing in the repo emits keyboard-shown readiness any more, so the legacy value in CLOUD_TEXT_ENTRY_READINESS (https://github.com/callstack/agent-device/blob/c8d3888/packages/contracts/src/interactor-types.ts#L154) and its preservation test could go unless a known out-of-tree provider still sends it.

A simplicity question: keyboard evidence can no longer authorize a fill, so the post-tap keyboard poll in awaitTextEntryReadiness only picks between two refusals, after the device was tapped and the readiness budget was spent. readTextEntryState already reads GET /element/active before the tap. Could fill refuse with text_entry_clear_target_unavailable before the tap when that read is unsupported? Then the keyboard pre-tap read, the keyboard fallback, textEntryFocusUnobservableError and the keyboard-shown value could all go, and a refusal would leave the device untouched.

Open threads: the cubic-dev-ai P2 thread on the unit refusal tests (#3990 (comment)) is fixed in the newer head 9d92f13, which asserts that neither clear nor keys ran in each refusal test, so it can be resolved. The keyboard-shown thread (#3990 (comment)) is covered by the note above. These threads are fixed at this head and can be resolved: #3990 (comment), #3990 (comment), #3990 (comment), #3990 (comment) and #3990 (comment).

I ran no tests. The live hosted run is described in the PR body, and I could not open those sessions. A rejected clear and a WebView password field were not run live, so real WDA and UiAutomator2 behavior for them is untested. CI is still running on this head: Smoke Tests is queued, and Repo Guards and Integration Tests are in progress. A failure in Integration Tests would relate to this PR, because it runs the provider scenarios you changed. The Vercel failure is the fork-deploy authorization prompt and does not touch this diff. There are no conflicts. Before merge, the fork CI must be approved and green on 9d92f13.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants