feat: add press_key and type_text tools - #153
Open
freema wants to merge 3 commits into
Open
Conversation
The input module had no keyboard capability at all. The only sendKeys
calls in src/ were bound to a specific element inside fill_by_uid and
upload_file_by_uid, plus one on a prompt dialog, so an agent could not
submit a form with Enter, close a modal with Escape, walk an autocomplete
list with the arrow keys, or move focus with Tab.
I added two tools to the input module, which is part of the slim preset,
so they are available in every configuration:
- press_key presses a key or combination on the focused element, e.g.
"Enter", "Escape" or "Control+Shift+R".
- type_text types into the focused element with an optional submitKey,
for elements that only react to real typing such as autocomplete
widgets and rich text editors. fill_by_uid stays the right tool for
replacing the value of a known input.
Key names follow the DOM KeyboardEvent.key vocabulary ("Enter",
"ArrowDown", "a") so a model can name keys the same way it would in page
code, and src/utils/keyboard.ts maps them onto the Selenium Key
constants. Unknown names fail with an error that lists what is accepted
rather than silently doing nothing.
Modifiers are held only for the duration of the key press. If the action
sequence throws part way through, the recovery path issues a release
actions command, otherwise the modifiers would stay logically held down
for the rest of the session and corrupt every later key press.
Tested with 32 unit tests covering the parser and the handlers, plus 6
integration tests against a real Firefox that assert typing, Enter
submitting a form, Tab moving focus, modifiers reaching the page as
ctrlKey/shiftKey, and modifiers being released afterwards.
…ests Review feedback on this PR. - typeText built two separate action sequences, one for the text and one for the submit key. MCP tool handlers are not serialised, so a concurrent call could move focus between the two and the submit key would land on a different element. Both now go into a single Actions object performed once. - An empty submitKey passed the handler but was dropped by a truthy check, so the tool reported success without pressing anything. It is now rejected, and the schema declares minLength. - The handler tests mocked pressKey/typeText wholesale, so nothing asserted the keyDown/keyUp ordering, the release-on-failure path or how many times perform() runs. Added tests/firefox/dom-keyboard.test.ts, which drives DomInteractions against a mocked actions API. These run in PR Check, unlike the integration tests. I also checked the review's concern that the requestAnimationFrame ping in waitForEventsAfterAction races a navigation triggered by Enter. I could not reproduce it: with a form that really navigates (no preventDefault), press_key and type_text both completed and landed on the target page in 24 out of 24 runs, and clickByUid on the same form behaved identically. The wait is shared by every existing input tool rather than introduced here, so I left it alone. Making it navigation-aware is worth doing on its own.
Review feedback on this PR: form.html cancels its submit with
preventDefault, so nothing here exercised a key press that tears the
document away while the tools are still finishing up.
Added nav-form.html and nav-target.html, a form with no preventDefault,
and two integration tests asserting that press_key("Enter") and
type_text(text, "Enter") both complete and land on the target page. The
second also checks the query string, so it fails if the typed text and
the submit key were separated by the navigation.
This replaces the manual check described in the previous commit with
something CI can verify. Ran the file three times, green each time.
Collaborator
Collaborator
Collaborator
Author
|
Hi @juliandescottes, thanks for taking a look! And sorry — that's a fail on my side, I should have checked the open PRs before starting this one and I completely missed #82. I fully agree with your suggestion: let's finalize and merge #82 first, and once it lands I'll rebase my branch on top of it, so my PR will effectively shrink to For a bit of context: I opened this PR because I need keyboard input for e2e testing of the login flows on my magazine websites, so I'm happy to help with reviewing/testing #82 to move things along. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The
inputmodule has no keyboard capability at all. The onlysendKeyscalls insrc/are bound to a specific element insidefill_by_uidandupload_file_by_uid, plus one on a prompt dialog inpages.ts. There is no way for an agent to press a key on its own, so it cannot submit a form with Enter, close a modal with Escape, walk an autocomplete list with the arrow keys, or move focus with Tab.I did not find an existing bug for this on the meta, so let me know if you would rather I file one on Bugzilla and re-land this under a bug number.
What I added
Two tools in the
inputmodule. That module is part ofslim, so they are available in every preset.press_keypresses a key or combination on the focused element:"Enter","Escape","Control+Shift+R".type_texttypes into the focused element, with an optionalsubmitKey. This is for elements that only react to real typing, such as autocomplete widgets and rich text editors.fill_by_uidstays the right tool for replacing the value of a known input, and I said so in both descriptions so the model does not treat them as interchangeable.Notes on the implementation
Key naming. Names follow the DOM
KeyboardEvent.keyvocabulary ("Enter","ArrowDown","a"), so a model can name keys the same way it would in page code.src/utils/keyboard.tsmaps them onto the SeleniumKeyconstants. Unknown names fail with an error listing what is accepted rather than silently doing nothing. A trailing+is handled, so"Control++"and"+"both mean the plus key.Modifier release. Modifiers are held only for the duration of the key press. If the action sequence throws part way through, the recovery path issues a release-actions command. Without it a failed
Control+…would leave Control logically held down for the rest of the session and corrupt every later key press, which is an unpleasant failure mode because it looks like the page misbehaving.type_textvalidatessubmitKeybefore typing, so an invalid key name fails without leaving the page half-filled.How I verified it
32 unit tests for the parser and the handlers, and 6 integration tests against a real Firefox (152.0.5, headless) in
tests/integration/keyboard.integration.test.ts:type_textwithsubmitKey: "Enter"submits the form fixtureTabmoves focus from#nameto#emailControl+Shift+barrives at the page withctrlKeyandshiftKeysetxpressed afterwards has both flags clear, so the modifiers really were releasedFull suite is green locally: 50 files, 672 tests, including the integration tests.
Follow-ups I did not include here
While reading
dom.tsI noticed the clear-input fallback infillBySelector/fillByUidusesKey.chord(Key.CONTROL, 'a'). On macOS select-all is Cmd+A, and Ctrl+A is the "move to start of line" binding, so that path moves the caret and deletes a single character instead of clearing. It only runs whenel.clear()throws, so it does not fire often, but when it does it silently writes a mixed value. I left it out to keep this PR additive. Happy to send it separately.Thanks for the reviews.