Fix keyboard dismiss semantics across iOS and Android#291
Conversation
|
![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: be22029b79
ℹ️ 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 result.wasVisible && !result.dismissed { | ||
| return Response( | ||
| ok: false, | ||
| error: ErrorPayload( | ||
| message: "Unable to dismiss the iOS keyboard without a native dismiss gesture or control" |
There was a problem hiding this comment.
When iOS keyboard dismissal fails (keyboard was visible but no native dismiss gesture/control works), this branch returns ok: false, but parseRunnerResponse in src/platforms/ios/runner-session.ts maps all runner failures to COMMAND_FAILED. That means callers do not receive the explicit UNSUPPORTED_OPERATION signal now documented for non-dismissible keyboards, and automation that branches on unsupported capability will behave differently on iOS vs Android for the same scenario.
Useful? React with 👍 / 👎.
| const result = await runIosRunnerCommand( | ||
| device, | ||
| { command: 'keyboardDismiss', appBundleId: context?.appBundleId }, | ||
| runnerCtx, |
There was a problem hiding this comment.
This call only forwards context?.appBundleId, so selector-only invocations without an active session pass undefined and the runner falls back to its own host app target (executeOnMain resets currentApp when no bundle is provided). In that path, keyboard dismiss --platform ios ... can succeed while never dismissing the AUT keyboard, which is a functional mismatch for explicit-selector usage on iOS.
Useful? React with 👍 / 👎.
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Review
Clean PR. The switch from KEYCODE_BACK (4) to KEYCODE_ESCAPE (111) on Android is the right call to avoid unintended navigation side effects. The explicit UNSUPPORTED_OPERATION error when dismiss fails is a good contract improvement over silently no-oping.
A few observations:
1. Redundant guard in RunnerTests+Interaction.swift. After guard wasVisible returns early, you check if keyboard.exists again on the same app.keyboards.firstMatch. Since isKeyboardVisible already confirmed keyboard.exists && !keyboard.frame.isEmpty, this inner if will always be true. Drop it and call keyboard.swipeDown() directly — removes a nesting level and makes the flow clearer.
2. iOS keyboard status/get throws UNSUPPORTED_OPERATION at dispatch time, but the capability matrix marks iOS as fully supported for keyboard. An agent calling keyboard status on iOS will get past the capability gate and hit the dispatch error. The mismatch between "capability says yes" and "dispatch says no for status" could confuse callers. A comment in the capability matrix noting iOS only supports the dismiss subcommand would help.
3. Minor: hardcoded sleepFor(0.2) calls in the Swift dismiss flow. If XCTest keyboard animation timing varies (iPad vs iPhone, accessibility settings), these could be flaky. A waitForExistence-style poll would be more robust, but low-risk for now.
The redundant guard (#1) is the only thing I'd clean up before merging; the rest are optional.
|
Addressed review feedback:
Tests: pnpm typecheck; pnpm test:unit -- --test-name-pattern "keyboard"; pnpm build:xcuitest:macos |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Re-review
All three items from the previous review are addressed:
- Redundant
keyboard.existsguard removed - Capability matrix now has a comment explaining the iOS-only-dismiss asymmetry
sleepFor(0.2)kept as-is — low risk, agreed
LGTM. Clean to merge.
|
Follow-up fixes pushed on top of the review pass:
Validation: pnpm typecheck, pnpm test:unit, pnpm test:smoke, pnpm build:xcuitest, pnpm format |
thymikee
force-pushed
the
codex/issue-270-keyboard-dismiss
branch
from
643ea98 to
c39f0ca
Compare
thymikee
force-pushed
the
codex/issue-270-keyboard-dismiss
branch
from
c39f0ca to
1cddf87
Compare
1 participant
Summary
Closes #270.
Implement issue #270 by making
keyboard dismissplatform-correct on Android and iOS.Replace Android back-navigation dismissal with a non-navigation keyboard dismiss path and return an explicit unsupported error when the IME cannot hide without navigating.
Add an iOS runner-backed
keyboard dismisspath, update command/docs text, and cover the behavior with tests.Touched files: 15. Scope expanded beyond the session command family into Android/iOS keyboard backends, the iOS runner, and docs because the command contract crosses those boundaries.
Validation
pnpm typecheckpnpm build:xcuitestpnpm test:unitpnpm test:smokepnpm format