Repository navigation
fix(webdriver): replace text in the confirmed focused element - #3990
bheemreddy-samsara wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
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
|
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 Not blocking, and you can take or leave these: 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 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. |
|
@cubic-dev-ai review this PR |
@bheemreddy-samsara I have started the AI code review. It will take a few minutes to complete. |
|
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. |
13d32da to
531acde
Compare
|
@bheemreddy-samsara is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
531acde to
c8d3888
Compare
|
@cubic review |
@bheemreddy-samsara I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
@thymikee Can you do a quick review ? |
|
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 A simplicity question: keyboard evidence can no longer authorize a fill, so the post-tap keyboard poll in 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 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. |
Summary
Addresses #3985 across 11 files. WebDriver
fillclears/replaces the confirmed focused element; empty fill clears, failed clear stops input, andtypestays append-only. Missing references refuse. No e2e freshness changes.Workflow
Validation
Head:
9d92f13eb4447668a7984cabfcbad66617428d57; rebased base:0b49de4dfd092a86c0d99d5fcb3dcd976d6ccc9b(includes #3997).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.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):
Unproven: live rejected-clear/WebView-password. Historical iOS readonly/zoom mis-target remains. All four head sessions closed.