fix: release held modifiers when press_key key event fails - #2347
Merged
OrKoN merged 2 commits intoJul 12, 2026
Merged
Conversation
OrKoN
reviewed
Jul 11, 2026
press_key pressed each modifier down in a loop, then pressed the main key, then released the modifiers in a second loop. If the main keyboard.press() rejected (a CDP hiccup, target crash, or dropped connection), the release loop was never reached and the modifiers were left logically held down in the browser — an unpaired keyDown, the class of defect asked about in ChromeDevTools#2309. Wrap the down/press sequence in try/finally and track which modifiers were actually pressed, so every held modifier is released even when the main press throws. Only modifiers whose keyboard.down() succeeded are released, so a failure while pressing a modifier does not trigger a spurious keyUp. Adds a regression test that injects a press() failure mid-sequence and asserts both modifiers are still released.
Liohtml
force-pushed
the
fix/press-key-release-modifiers-on-error
branch
from
July 11, 2026 14:11
4c7465c to
7f436be
Compare
Contributor
Author
|
Rebased on main. The CI red was the McpContext getter refactor (#2342 and friends) landing after I branched — |
OrKoN
approved these changes
Jul 11, 2026
OrKoN
enabled auto-merge
July 11, 2026 14:14
Collaborator
|
Thanks! |
auto-merge was automatically disabled
July 11, 2026 14:18
Head branch was pushed to by a user without write access
Liohtml
force-pushed
the
fix/press-key-release-modifiers-on-error
branch
from
July 11, 2026 14:18
7f436be to
391384b
Compare
Liohtml
added a commit
to Liohtml/chrome-devtools-mcp
that referenced
this pull request
Jul 13, 2026
Narrows this change to what is provably correct after a second audit. press_key still left the *main* key held: Puppeteer's Keyboard.press() is a bare down() followed by up() with no guard, so a rejecting up() leaves the key down in the browser. ChromeDevTools#2347 released only the modifiers. The press is now split into down()/up() so a failed release can be retried, without dispatching a key up for a key whose down() never landed. down(key, {}) keeps the CDP payload identical to press(). Each release is also guarded so a failing up() can neither abort the remaining releases nor replace the error that triggered them. type_text's submitKey goes through the same helper. Dropped from this change after review: - The drag fix. ElementHandle.drag() presses the mouse button and only drop() releases it, so a failed drop does leave the button down. But drop() clears page._isDragging only on success, so releasing the mouse without also clearing that flag makes the *next* drag skip mouse.down() and fail with "'left' is not pressed." — worse than the bug. Clearing it means reaching into a Puppeteer internal, so it needs its own change. - Keyboard.type() presses each character through the same unguarded press(). Locator.fill() types short values that way too, so fill and fill_form are exposed as well. Guarding it here would mean reimplementing type() on top of the non-public charIsKey(); the fix belongs in Puppeteer's press().
OrKoN
reviewed
Jul 13, 2026
| await page.pptrPage.keyboard.down(modifier); | ||
| heldModifiers.push(modifier); | ||
| } | ||
| await page.pptrPage.keyboard.press(key); |
Collaborator
There was a problem hiding this comment.
@Liohtml I have seen #2353 and I just want to clarify something here: this code is an improvement but generally if press fails also keyboard.ups would fail because the usual cause is something like the page is closed/crashed. Do you actually have realistic scenarios when this would fail on healthy pages?
kku1993
pushed a commit
to kku1993/chrome-devtools-mcp
that referenced
this pull request
Jul 14, 2026
🤖 I have created a release *beep* *boop* --- ## [1.6.0](ChromeDevTools/chrome-devtools-mcp@chrome-devtools-mcp-v1.5.0...chrome-devtools-mcp-v1.6.0) (2026-07-14) ### 🎉 Features * add experimentalGcfFormat flag for GCF-encoded tool responses ([ChromeDevTools#2235](ChromeDevTools#2235)) ([3d21389](ChromeDevTools@3d21389)) * Print object count and total sizes in get_heapsnapshot_details ([ChromeDevTools#2325](ChromeDevTools#2325)) ([15a6b78](ChromeDevTools@15a6b78)) * support --allow-unrestricted-paths configuration ([ChromeDevTools#2296](ChromeDevTools#2296)) ([6e56c02](ChromeDevTools@6e56c02)) * Support filter with heap snapshots aggregates ([ChromeDevTools#2323](ChromeDevTools#2323)) ([2812902](ChromeDevTools@2812902)) * update Lighthouse to 13.4.0 ([ChromeDevTools#2317](ChromeDevTools#2317)) ([ffc6060](ChromeDevTools@ffc6060)) ### 🛠️ Fixes * enforce .gz instead of json.gz in performance tools ([ChromeDevTools#2305](ChromeDevTools#2305)) ([b06e39b](ChromeDevTools@b06e39b)) * keep a still-open selected page instead of falling back to the first page ([ChromeDevTools#2328](ChromeDevTools#2328)) ([c645eee](ChromeDevTools@c645eee)), closes [ChromeDevTools#2304](ChromeDevTools#2304) * keep page ids unique across browser reconnects ([ChromeDevTools#2345](ChromeDevTools#2345)) ([3e8d922](ChromeDevTools@3e8d922)) * paginate page 0 in list_network_requests and list_console_messages ([ChromeDevTools#2359](ChromeDevTools#2359)) ([d0025b3](ChromeDevTools@d0025b3)) * release held modifiers when press_key key event fails ([ChromeDevTools#2347](ChromeDevTools#2347)) ([78ccb19](ChromeDevTools@78ccb19)) * report when the selected page was auto-replaced by the fallback ([ChromeDevTools#2308](ChromeDevTools#2308)) ([2c16ac3](ChromeDevTools@2c16ac3)), closes [ChromeDevTools#2304](ChromeDevTools#2304) * resolve page ids only among listed pages ([ChromeDevTools#2332](ChromeDevTools#2332)) ([eb04951](ChromeDevTools@eb04951)), closes [ChromeDevTools#2304](ChromeDevTools#2304) * **snapshot:** resolve element ids on the correct snapshot ([ChromeDevTools#2295](ChromeDevTools#2295)) ([b703f2c](ChromeDevTools@b703f2c)) * **telemetry:** resolve enum values through nested schema wrappers ([ChromeDevTools#2315](ChromeDevTools#2315)) ([c065fd9](ChromeDevTools@c065fd9)) * Wait until daemon is started ([ChromeDevTools#2327](ChromeDevTools#2327)) ([ed7e95d](ChromeDevTools@ed7e95d)) ### 📄 Documentation * add Grok Build CLI configuration section ([ChromeDevTools#2294](ChromeDevTools#2294)) ([aa4be07](ChromeDevTools@aa4be07)) * update memory leak debugging skill ([ChromeDevTools#2330](ChromeDevTools#2330)) ([c1736a0](ChromeDevTools@c1736a0)) ### ⚡ Performance * concurrent I/O in Root Path Resolution ([ChromeDevTools#2279](ChromeDevTools#2279)) ([b2c63e6](ChromeDevTools@b2c63e6)) ### 🏗️ Refactor * clean up McpContext getters ([ChromeDevTools#2340](ChromeDevTools#2340)) ([5b33deb](ChromeDevTools@5b33deb)) * clean up more of the context interface ([ChromeDevTools#2335](ChromeDevTools#2335)) ([9cd734b](ChromeDevTools@9cd734b)) * clean up page management ([ChromeDevTools#2333](ChromeDevTools#2333)) ([16db01f](ChromeDevTools@16db01f)) * clean up page snapshot generation ([ChromeDevTools#2348](ChromeDevTools#2348)) ([68cfce2](ChromeDevTools@68cfce2)) * make collectors work per page ([ChromeDevTools#2324](ChromeDevTools#2324)) ([9bc61b4](ChromeDevTools@9bc61b4)) * move and rename files ([ChromeDevTools#2355](ChromeDevTools#2355)) ([9c3542b](ChromeDevTools@9c3542b)) * move DevTools universe to McpPage ([ChromeDevTools#2341](ChromeDevTools#2341)) ([c006c9b](ChromeDevTools@c006c9b)) * move remaining McpContext getters ([ChromeDevTools#2342](ChromeDevTools#2342)) ([58ba174](ChromeDevTools@58ba174)) * remove isolated context getter ([ChromeDevTools#2336](ChromeDevTools#2336)) ([8a4ddb3](ChromeDevTools@8a4ddb3)) * Use array instead of Map for idToClassKey ([ChromeDevTools#2321](ChromeDevTools#2321)) ([ff53b7b](ChromeDevTools@ff53b7b)) * use helper for Dialog handle ([ChromeDevTools#2334](ChromeDevTools#2334)) ([64005f9](ChromeDevTools@64005f9)) * use response page in formatting ([ChromeDevTools#2349](ChromeDevTools#2349)) ([c53c1ec](ChromeDevTools@c53c1ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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
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.
What
press_keycurrently presses each modifier down, presses the main key, then releases the modifiers — in three sequential steps with notry/finally:If
keyboard.press(key)rejects — a CDP hiccup, a target crash, or a dropped connection — the release loop is skipped and the modifier keys are left logically held down in the browser.waitForEventsAfterActionre-throws the action error, so nothing downstream releases them either.This is the unpaired-
keyDownclass of defect asked about in #2309 ("whether any code path sends a keyDown without a guaranteed matching keyUp on an error/timeout branch"). This PR fixes the one concrete instance of it in this repo.Fix
Wrap the down/press sequence in
try/finallyand track which modifiers were actually pressed, releasing each held modifier even when the main press throws. Only modifiers whosekeyboard.down()succeeded are released, so a failure while pressing a modifier doesn't emit a spuriouskeyUp.Test
Adds a regression test (real browser, keydown/keyup logging) that injects a
press()failure mid-sequence and asserts both modifiers are still released. It fails onmain(['dControl','dShift']— no keyups) and passes with the fix (['dControl','dShift','uShift','uControl']).Verified locally: full
tests/tools/input.test.tssuite passes,npm run typecheck, eslint, and prettier all clean.Scope note re: #2309
I want to be precise about what this does and does not address. This closes a browser-level stuck-key path: leaked keys here live in Chromium's input state (CDP
Input.dispatchKeyEventis injected into the renderer), so the observable effect is a modifier stuck within the driven page. The report in #2309 is a bareSpacethat repeats system-wide and survives physically unplugging the keyboard — that symptom is at the OS input layer, which CDP-injected input doesn't route through, so I don't claim this fully explains that case (details and a non-reboot workaround are in a comment on the issue). Still, an unpaired keyDown on an error branch is a real defect worth closing on its own, and it's exactly the code path the issue asked to audit.Prepared with AI assistance (Claude Code) and verified against a local build before submission.