Skip to content

fix: exclude withKeyDown from the expectation retry loop - #300

Open
likemusic wants to merge 1 commit into
pestphp:5.xfrom
likemusic:fix/withkeydown-not-awaitable
Open

likemusic wants to merge 1 commit into
pestphp:5.xfrom
likemusic:fix/withkeydown-not-awaitable

Conversation

@likemusic

Copy link
Copy Markdown
Contributor

withKeyDown() is the one mutating action still routed through Execution::waitForExpectation(). It holds a key down, runs its callback (which performs its own actions), then releases the key. When an attempt overruns the fixed 1000 ms per-attempt budget, the loop retries it — so the key is held and the callback's actions replay a second time.

#269 took the other state-changing actions out of $nonAwaitableMethods (click, rightClick, press, pressAndWaitFor, keys, drag, append), and the navigation methods (navigate, refresh, back, forward) followed. withKeyDown is the one left. This adds it, so a single direct call gets Playwright's actionability wait on the whole configured timeout instead of a retried one-second attempt. It closes the remaining gap from #249 (the rest of that PR is already covered by #269).

The fixed-1000 ms-per-attempt root cause for whatever legitimately stays in the loop (e.g. submit) is a separate concern, tracked in #276.

Test

tests/Browser/Webpage/WithKeyDownRetryTest.php makes the first attempt overrun 1000 ms on a busy main thread, then asserts the callback ran once. On 5.x it fails (window.presses is 2); with this change it passes (repeat(3), 3/3).

withKeyDown() holds a key down, runs its callback (which performs its own
actions), then releases the key. Routed through Execution::waitForExpectation(),
an attempt that overruns the fixed 1000 ms budget is retried, so the key is held
and the callback's actions replay a second time.

This is the one mutating action still missing from $nonAwaitableMethods after
pestphp#269 took out click/press/drag/keys/append and the navigation methods. A single
direct call gets Playwright's actionability wait on the whole configured timeout.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant