fix: release keys left held when a press_key key event fails#2351
fix: release keys left held when a press_key key event fails#2351Liohtml wants to merge 3 commits into
Conversation
ChromeDevTools#2347 fixed one instance of the unpaired-input defect in this file. I audited the rest of the input tools afterwards and found more, all with the same shape: a key or button is pressed down, and the release is only reachable on the success path. - press_key left the *main* key held. Puppeteer's Keyboard.press() is a bare down() followed by up() with no guard of its own, so a rejection between the two leaves the key logically held down. ChromeDevTools#2347 only released the modifiers. - drag left the *mouse button* held. ElementHandle.drag() presses the button down and only drop() releases it (drag interception is not enabled), so a failed drop leaves the button down for the rest of the session, breaking every later click. - type_text's submitKey went through the same unguarded press(). - The release loop added in ChromeDevTools#2347 was itself unguarded: a failing up() aborted the remaining releases and replaced the original error. Adds pressKeyReleasingHeldKeys(), which releases every key it pressed even if a key event rejects, guarding each release so one failure can neither abort the others nor mask the original error. Keys whose down() rejected are released too: Keyboard.down() latches the key on the client *before* it sends the CDP command and only up() clears it, so a latched modifier would otherwise be stamped onto every later keyboard and mouse event. Tests: a transient key up failure now leaves no key held, and a failed drop no longer leaves the mouse button down. Both fail on main.
Calling keyboard.down(key) directly changed the CDP payload: press()
passes {} to down(), which sends commands: undefined, while calling
down(key) with no options uses Puppeteer's default and sends
commands: []. Keep using press() so the wire payload is unchanged; the
release guarantee is unaffected because the key is recorded as held
before press() runs and only cleared once it resolves.
Also documents why the drag path releases the mouse unconditionally.
|
Self-review pass before you spend time on this — two things I tightened, and one claim I want to be explicit about because I got a mechanism wrong on #2283 and would rather not repeat it. Fixed: I was changing the CDP payload. My first version replaced Verified, not assumed: the justification for releasing a key whose Checked and deliberately not touched: Both regression tests still fail against current |
|
Moving this to draft — I want to finish auditing the remaining input paths myself rather than have you find them. Will mark it ready once I have. Sorry for the churn. |
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().
|
Filed #2353 with the full audit — the root cause is an unguarded |
Follow-up to #2347, which fixed one instance of an unpaired-input defect and claimed to have fixed the class. It hadn't. I audited the rest of the input paths, and then audited my own first draft of this PR — which was wrong in two ways and would have made things worse. This is the narrowed version: only what I can prove.
What this fixes
press_keystill left the main key held. Puppeteer'sKeyboard.press()is a baredown()thenup()with no guard of its own:#2347 released only the modifiers. Verified on
main:["dControl","dShift","dC","uShift","uControl"]— nouC.The press is now split into
down()/up()so a failed release can be retried.down(key, {})keeps the CDP payload byte-identical topress()(which passes{}, socommandsgoes out asundefinedrather than[]). A key whosedown()never landed is not released — there is no key up to send.Each release is guarded so a failing
up()can neither abort the remaining releases nor replace the error that triggered them (a flaw in thefinally#2347 added).type_text'ssubmitKeyuses the same helper.What I removed from this PR after review, and why
The
dragfix — it was worse than the bug.ElementHandle.drag()presses the mouse button and onlydrop()releases it (drag interception is never enabled), so a failed drop really does leave the button down. Butdrop()clearspage._isDraggingonly on success. Releasing the mouse without clearing that flag makes the next drag take theif (!page._isDragging)branch, skipmouse.down()entirely, and die with'left' is not pressed.— I'd have traded a stuck button for a permanently broken drag tool. Clearing the flag means reaching into a Puppeteer internal that isn't intypes.d.ts, so it deserves its own change and your opinion first. The drag leak is real and still open.I also removed a comment justifying a behaviour with a false claim. I had written that a key whose
down()rejected must still be released becausedown()latches the modifier bit before it sends. That's true for modifiers — and false for the main key:#modifierBit()returns0for anything that isn't Alt/Control/Meta/Shift. I was asserting a spuriousuCkeyup as correct behaviour on a fabricated premise. Modifiers are still released on a faileddown()(that part is real, and_modifiersis stamped onto every later keyboard, mouse and touch event); the main key is not.Still open, disclosed rather than claimed fixed
Keyboard.type()presses each character through the same unguardedpress(), so a mid-string failure leaves a character key held.Locator.fill()types short values throughKeyboard.type()too — sofillandfill_formare exposed the same way. Guarding it here would mean reimplementingtype()on top of the non-publiccharIsKey(). The real fix belongs in Puppeteer:press()should release in afinally. That single change would closepress_key,type_text,fillandfill_format once. Happy to send it upstream if you agree.Mouse.click()is not affected for this repo's callers — it eagerly creates both thedown()andup()promises beforePromise.all, andclick/click_atnever pass adelay. It is not a general property: on thedelaypathup()is created after the await, so a rejectingdown()would leak. Left alone deliberately.Tests
One regression test, failing on
mainand passing here: a transient key-up failure no longer leaves the main key held. The #2347 test is kept and updated to stubdown(the code no longer routes throughpress). Fullinput.test.tsgreen (47), typecheck + eslint + prettier clean.