fix: release keys left held when a press_key key event fails

src/tools/input.ts

@@ -7,7 +7,7 @@
 import {logger} from '../logger.js';
 import type {McpContext} from '../McpContext.js';
 import {zod} from '../third_party/index.js';
-import type {ElementHandle, KeyInput} from '../third_party/index.js';
+import type {ElementHandle, Keyboard, KeyInput} from '../third_party/index.js';
 import type {TextSnapshotNode} from '../types.js';
 import {parseKey} from '../utils/keyboard.js';
 import type {WaitForEventsResult} from '../WaitForHelper.js';
@@ -33,6 +33,67 @@ const submitKeySchema = zod
     'Optional key to press after typing. E.g., "Enter", "Tab", "Escape"',
   );
 
+/**
+ * Releases `key`, swallowing any failure: a failed release must not abort the
+ * releases that follow it, nor replace the error that triggered the release.
+ *
+ * If the original `up()` reached the renderer before its promise rejected, this
+ * dispatches a second key up. That is a deliberate trade: a duplicate key up is
+ * inert for a key the renderer no longer considers pressed, whereas a key left
+ * held down is not.
+ */
+async function releaseKey(keyboard: Keyboard, key: KeyInput) {
+  try {
+    await keyboard.up(key);
+  } catch (error) {
+    logger?.('failed to release key', key, error);
+  }
+}
+
+/**
+ * Presses `key` while `modifiers` are held, releasing every key that was
+ * actually pressed down even if a key event rejects mid-sequence.
+ *
+ * Puppeteer's `Keyboard.press()` is a bare `down()` followed by `up()` with no
+ * guard of its own, so a rejecting `up()` leaves the key held down in the
+ * browser. The press is split here so that a failed release can be retried
+ * without dispatching a key up for a key that never went down.
+ *
+ * Modifiers are released even when their own `down()` rejected. `Keyboard.down()`
+ * sets the client-side modifier bit *before* it sends the CDP command and only
+ * `up()` clears it, and that bit is stamped onto every subsequent keyboard,
+ * mouse and touch event — so a latched modifier would silently modify every
+ * later click. Non-modifier keys carry no such bit, which is why the main key is
+ * released only once its `down()` has actually succeeded.
+ */
+async function pressKeyReleasingHeldKeys(
+  keyboard: Keyboard,
+  key: KeyInput,
+  modifiers: KeyInput[] = [],
+) {
+  const heldModifiers: KeyInput[] = [];
+  let keyIsDown = false;
+  try {
+    for (const modifier of modifiers) {
+      heldModifiers.push(modifier);
+      await keyboard.down(modifier);
+    }
+    // Equivalent to keyboard.press(key), which calls down(key, {}) followed by
+    // up(key). Passing {} keeps the dispatched CDP payload identical.
+    await keyboard.down(key, {});
+    keyIsDown = true;
+    await keyboard.up(key);
+    keyIsDown = false;
+  } finally {
+    if (keyIsDown) {
+      await releaseKey(keyboard, key);
+    }
+    for (const modifier of heldModifiers.toReversed()) {
+      await releaseKey(keyboard, modifier);
+    }
+  }
+}
+
 function handleActionError(error: unknown, uid: string) {
   logger?.('failed to act using a locator', error);
   throw new Error(
@@ -354,9 +415,18 @@ export const typeText = definePageTool({
   handler: async (request, response) => {
     const page = request.page;
     const result = await page.waitForEventsAfterAction(async () => {
+      // Note: Keyboard.type() presses each character through the same unguarded
+      // down()/up() pair, so a mid-string failure can still leave a character
+      // key held. Locator.fill() types short values through Keyboard.type() as
+      // well, so `fill` and `fill_form` are exposed the same way. Guarding it
+      // here would mean reimplementing type() on top of charIsKey(), which is
+      // not public API — the release belongs in Puppeteer's press(). A stuck
+      // character key is far less harmful than a stuck modifier: it carries no
+      // modifier bit, so it is not stamped onto subsequent events.
       await page.pptrPage.keyboard.type(request.params.text);
       if (request.params.submitKey) {
-        await page.pptrPage.keyboard.press(
+        await pressKeyReleasingHeldKeys(
+          page.pptrPage.keyboard,
           request.params.submitKey as KeyInput,
         );
       }
@@ -526,21 +596,7 @@ export const pressKey = definePageTool({
     const [key, ...modifiers] = tokens;
 
     const result = await page.waitForEventsAfterAction(async () => {
-      const heldModifiers: KeyInput[] = [];
-      try {
-        for (const modifier of modifiers) {
-          await page.pptrPage.keyboard.down(modifier);
-          heldModifiers.push(modifier);
-        }
-        await page.pptrPage.keyboard.press(key);
-      } finally {
-        // Release every modifier that was successfully pressed, even if a
-        // later key event throws. Otherwise a failed press leaves modifiers
-        // logically held down in the browser (see #2309).
-        for (const modifier of heldModifiers.toReversed()) {
-          await page.pptrPage.keyboard.up(modifier);
-        }
-      }
+      await pressKeyReleasingHeldKeys(page.pptrPage.keyboard, key, modifiers);
     });
 
     response.appendResponseLine(

tests/tools/input.test.ts

@@ -14,6 +14,7 @@ import sinon from 'sinon';
 import type {ParsedArguments} from '../../src/bin/chrome-devtools-mcp-cli-options.js';
 import {McpResponse} from '../../src/McpResponse.js';
 import {TextSnapshot} from '../../src/TextSnapshot.js';
+import type {KeyInput} from '../../src/third_party/index.js';
 import {
   click,
   hover,
@@ -1376,11 +1377,17 @@ describe('input', () => {
           context.getSelectedMcpPage(),
         );
 
-        // Simulate the main key press failing mid-sequence (e.g. a CDP
-        // hiccup) after the modifiers have already been pressed down.
+        // Simulate the main key's key down failing (e.g. a CDP hiccup) after
+        // the modifiers have already been pressed down.
+        const realDown = page.keyboard.down.bind(page.keyboard);
         sinon
-          .stub(page.keyboard, 'press')
-          .throws(new Error('injected press failure'));
+          .stub(page.keyboard, 'down')
+          .callsFake(async (key: KeyInput): Promise<void> => {
+            if (key === 'C') {
+              throw new Error('injected key down failure');
+            }
+            return await realDown(key);
+          });
 
         try {
           await assert.rejects(
@@ -1399,9 +1406,9 @@ describe('input', () => {
           sinon.restore();
         }
 
-        // The modifiers were pressed down; both must be released even though
-        // the main key press threw, otherwise the browser is left with the
-        // modifiers logically stuck down.
+        // The modifiers must be released even though the main key failed,
+        // otherwise they stay logically held down in the browser. "C" is not
+        // released: its key down never landed, so there is no key up to send.
         assert.deepStrictEqual(await page.evaluate('logs'), [
           'dControl',
           'dShift',
@@ -1410,5 +1417,57 @@ describe('input', () => {
         ]);
       });
     });
+
+    it('retries releasing the main key when its key up fails', async () => {
+      await withMcpContext(async (response, context) => {
+        const page = context.getSelectedMcpPage().pptrPage;
+        await page.setContent(
+          html`<script>
+            logs = [];
+            document.addEventListener('keydown', e => logs.push('d' + e.key));
+            document.addEventListener('keyup', e => logs.push('u' + e.key));
+          </script>`,
+        );
+        context.getSelectedMcpPage().textSnapshot = await TextSnapshot.create(
+          context.getSelectedMcpPage(),
+        );
+
+        // A transient CDP failure on the main key's key up. The key down for
+        // "C" already reached the renderer, so if nothing retries the key up
+        // the browser is left with "C" logically held down.
+        const realUp = page.keyboard.up.bind(page.keyboard);
+        let failedOnce = false;
+        sinon
+          .stub(page.keyboard, 'up')
+          .callsFake(async (key: KeyInput): Promise<void> => {
+            if (key === 'C' && !failedOnce) {
+              failedOnce = true;
+              throw new Error('transient CDP failure');
+            }
+            return await realUp(key);
+          });
+
+        try {
+          await assert.rejects(
+            pressKey.handler(
+              {
+                params: {key: 'Control+Shift+C'},
+                page: context.getSelectedMcpPage(),
+              },
+              response,
+              context,
+            ),
+          );
+        } finally {
+          sinon.restore();
+        }
+
+        const logs = (await page.evaluate('logs')) as string[];
+        assert.ok(
+          logs.includes('uC'),
+          `expected "C" to be released, got ${JSON.stringify(logs)}`,
+        );
+      });
+    });
   });
 });