Skip to content

Rely on native Playwright action waiting - #172

Open
Toflar wants to merge 7 commits into
playwright-php:mainfrom
Toflar:fix/native-playwright-action-waiting
Open

Toflar wants to merge 7 commits into
playwright-php:mainfrom
Toflar:fix/native-playwright-action-waiting

Conversation

@Toflar

@Toflar Toflar commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Unfortunately, I have seen flaky failures on GitHub Actions jobs which seem limited to 0.25 CPU. The same test could alternately fail with either a JSON-RPC timeout or with Element not actionable, even though the latter was not the underlying problem so this was a nightmare to debug.

Turns out click(), fill() and dragTo() performed PHP-side actionability polling before sending the actual command to Playwright. Each poll issued additional isVisible() and isEnabled() JSON-RPC requests. Under constrained resources, a transport timeout during one of these requests could be caught by the polling loop and eventually replaced with the misleading Element not actionable error.

This PR removes all of that redundant polling and sends locator actions directly to Playwright. Playwright already provides native auto-waiting and complete actionability checks for these operations, so it can determine when an element is ready and return its original error details and call log when an action fails.

The existing optional action timeout parameters are now nullable. That way, they remain available and are forwarded to the native Playwright operation. This preserves backward compatibility without retaining the PHP polling behavior.

Native locator waits also replace PHP polling where Playwright provides an equivalent operation. The remaining PHP condition polling no longer catches PlaywrightException, ensuring JSON-RPC timeouts, strict-selector violations and other operational failures remain visible to the caller. So this should make debugging easier 😇

@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/Transport/JsonRpc/JsonRpcTransport.php 90.90% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Toflar
Toflar force-pushed the fix/native-playwright-action-waiting branch from fc286ca to e792d88 Compare September 21, 2026 10:59
@Toflar

Toflar commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

I just updated the PR with my fork and you can check for yourself that with this PR, the e2e tests pass just fine: https://github.com/contao/contao/actions/runs/36156023262/job/108140649630?pr=10088 😊

@smnandre

Copy link
Copy Markdown
Member

Thanks @Toflar!

Removing the extra PHP polling layer makes perfet sense 👍

I tested this with Chromium and found two "regressions":

  • With timeoutMs: 2000, evaluate('new Promise(() => {})') previously timed out after 2 seconds. It now keeps waiting because commands without an explicit timeout lose the transport deadline. I think we should keep a deadline for operations without a native timeout, while preserving your fix for page/context timeouts.

  • With a page timeout of 100 ms, both waitForEnabled(['timeout' => 1000]) and waitForText('Ready', ['timeout' => 1000]) now fail after about 110 ms. An element appearing after 350 ms is found on main but missed here: the inner query times out before the requested wait expires. These helpers should use the remaining wait budget while still letting operational errors through. WDYT?

And if that's ok for you, could we cover these two cases with regression tests too?

Please tell me if you disagree here or if I missed something :)

@Toflar

Toflar commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for testing this, and yes, I agree with both points.

I restored the transport deadline for commands that do not have a native Playwright timeout, so an evaluation of a promise that never settles is bounded by the configured transport timeout again. Commands with native timeout semantics still defer to Playwright, which preserves page and context defaults as well as explicit operation timeouts.

I also changed waitForEnabled() and waitForText() to pass their remaining wait budget to the inner locator query. This prevents a shorter page default from ending the query before the requested helper timeout. Operational errors are no longer swallowed by the polling loop. Both reported Chromium cases now have regression coverage.

While auditing the surrounding timeout handling, I found two related issues and included fixes for them as well:

  • getAttribute() already accepted timeout options in PHP but the JavaScript handler discarded them.
  • A timeout of zero in the custom locator waits was converted to the 30-second fallback instead of meaning unlimited, as it does in Playwright.

There is, however, a broader API consistency issue affecting all of Playwright PHP that I intentionally left out of this PR. Playwright itself supports timeout options on several other locator operations while our corresponding PHP methods currently have parameterless signatures. Exposing those options would require changing existing methods on LocatorInterface which would be a BC break of course. So that should be handled separately with an explicit backward-compatibility strategy, likely as part of a major release or so.

The new NATIVE_TIMEOUT_ACTIONS constant on JsonRpcTransport is the contained fix for distinguishing those two kinds of commands. At transport level, commands only arrive as flat payloads and the presence of an options array does not tell us whether Playwright owns the timeout. Without changing every command producer or extending the internal protocol with timeout metadata, an explicit allow-list is the only reliable quick fix. It is deliberately conservative, so unknown commands keep the transport safety deadline. A future refactor could make timeout ownership explicit in the command metadata and remove the duplicated list but yeah...well, I had to stop somewhere in this PR 😄

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.

2 participants