Avoid retrying side-effecting browser actions

[CalorieStar]

Summary

I had an issue where click() was timing out on some pages, even though the click visibly occurred in the browser when using --debug.

This appears to relate to the behaviour described in:

Related to pestphp/pest#1511

What changed

This PR updates AwaitableWebpage so state-changing browser actions are no longer routed through Execution::waitForExpectation().

This includes actions such as:

• click
• press
• fill
• check
• select
• drag
• attach
• similar browser actions that may alter page or application state

Assertions and read-style expectations still use the existing retry loop.

Why

Execution::waitForExpectation() retries the callback using a 1000ms Playwright timeout. That behaviour is useful for assertions, because repeatedly checking whether something is true is usually safe.

For example, retrying an assertion such as assertSee() is useful because the page may need time to update.

However, this retry behaviour is less suitable for browser actions. Actions such as click() and press() can change the page, trigger JavaScript, submit forms, start navigation, or update application state.

In these cases, the action may already have succeeded, but Pest may still report Timeout 1000ms exceeded if the operation did not complete within the 1000ms attempt window. Retrying the action can then cause further issues because:

• the original element may no longer exist
• navigation may already have started or completed
• the page may now be in a different state
• the same action may be attempted more than once
• form submissions or JavaScript handlers may be triggered more than intended

This PR avoids the pattern where a browser action visibly succeeds, but Pest still fails because the action was handled through the expectation retry loop.

Behaviour after this change

State-changing browser actions are attempted once and use the configured Playwright/browser timeout.

Assertions continue to be retried until the configured timeout expires.

This preserves the useful retry behaviour for assertions, while avoiding unsafe retries for actions that may alter browser or application state.

Side-effecting / state-changing actions

A side-effecting action is an operation that changes something.

In browser testing, this means the command does not merely observe the page. It may alter:

• the DOM
• form values
• browser state
• page state
• application state
• server state
• database records
• navigation state

Examples include click, press, fill, check, select, drag, and attach.

These actions should be awaited using the configured action timeout, but not retried in the same way as assertions.

[MrBMT]

Thanks for this, the PR here appears to have solved every issue I’d been having with duplicate clicks, form input etc. with the browser plugin and hasn’t introduced any negative side effects to any of my tests.

Now tests that were sometimes flakey because the browser plugin would double click or type text multiple times sometimes are working as expected every time, hopefully this can be merged soon.

[DanielGSoftware]

Same here, would be amazing if this PR can be looked at.

[DanielGSoftware]

Again thanks for this, it does fix the navigation-hang case. But I'm following up on my previous comment because when I applied this PR to my test-suite I got two deterministic regressions, both on a ->type() immediately after a click that reveals the field.

Both pass on stock 4.x and fail (15s timeout on the type) with this PR, reproducibly in isolation. The retry loop was silently compensating for a race: GuessLocator resolves the target eagerly (via ->count()), and if the reveal-click's effect hasn't landed yet, it falls back to getByText() and hangs. Run-once removes that cushion, so the race surfaces.

The underlying issue is really navigation-specific, so disabling retries for every action feels broader than needed. Would an opt-in noWaitAfter (per the suggestion in pestphp/pest#1511) be preferable? Adding ?array $options = null to click()/press() and forwarding to the Playwright locator, leaving the retry loop intact:

  ->press('Submit', ['noWaitAfter' => true])                                                                                                                                                                                                                                                                                
  ->assertPathIs('/next-page');                                                                                                                                                                                                                                                                                             

In my tests that fixes the hang (the navigating press opts out of the nav-settle wait; assertPathIs is still retried and waits for the URL to commit) without touching the type-after-reveal tests.

[DanielGSoftware]

Made a PR for that if it would be the preferred solution

[MrBMT]

Again thanks for this, it does fix the navigation-hang case. But I'm following up on my previous comment because when I applied this PR to my test-suite I got two deterministic regressions, both on a ->type() immediately after a click that reveals the field.

Both pass on stock 4.x and fail (15s timeout on the type) with this PR, reproducibly in isolation. The retry loop was silently compensating for a race: GuessLocator resolves the target eagerly (via ->count()), and if the reveal-click's effect hasn't landed yet, it falls back to getByText() and hangs. Run-once removes that cushion, so the race surfaces.

The underlying issue is really navigation-specific, so disabling retries for every action feels broader than needed. Would an opt-in noWaitAfter (per the suggestion in pestphp/pest#1511) be preferable? Adding ?array $options = null to click()/press() and forwarding to the Playwright locator, leaving the retry loop intact:

  ->press('Submit', ['noWaitAfter' => true])                                                                                                                                                                                                                                                                                
  ->assertPathIs('/next-page');                                                                                                                                                                                                                                                                                             

In my tests that fixes the hang (the navigating press opts out of the nav-settle wait; assertPathIs is still retried and waits for the URL to commit) without touching the type-after-reveal tests.

The noWaitAfter option is already depreciated FYI, and in the case of click at least, will default to true in the future.

However I've found this PR fixes more than just clicking - when watching tests in headed mode often I'd find it was double typing, and this PR fixed that too. Sadly the depreciated noWaitAfter option is not being set true for fill, so that issue will remain without a PR like this one.

In the case if your test failures, I feel like all you'd need is an assertSee() or similar before the the type() (or ideally fill(), as type() is depreciated).

Unfortunately sometimes you have to deal with particularly flakey things like entering text in to CKEditor instances etc. and this PR really helps there.

[CalorieStar]

Thank you @MrBMT and @DanielGSoftware for all the feedback and suggestions.

My original assessment of the problem was that browser actions were being executed through Execution::waitForExpectation().

That method retries the complete callback using a short Playwright timeout. This is appropriate for assertions, where repeatedly checking a condition is normally safe, but it is problematic for state-changing actions such as:

• click()
• press()
• fill()
• type()
• check()
• select()

An action could succeed in the browser but fail to return within the individual retry window. Pest would then execute the complete callback again.

This caused behaviour such as:

• a click visibly succeeding but Pest reporting Timeout 1000ms exceeded
• the same button being clicked more than once
• text being entered more than once
• an action being retried after navigation had already started
• subsequent attempts failing because the original element no longer existed

My initial solution was therefore to classify state-changing browser methods as non-retryable and execute each action once using the full configured Playwright timeout.

That fixed the original navigation and duplicate-action problem. However, it also exposed another issue in the existing locator-resolution behaviour.

The existing retry loop was performing two jobs:

1. retrying the browser action
2. indirectly retrying locator discovery

GuessLocator resolves a target eagerly. For non-explicit selectors, it checks potential id and name locators using count().

When an action immediately follows a click that reveals a field asynchronously, the field may not exist when GuessLocator first runs.

For example:

->click('Show additional fields')
->fill('email', 'person@example.com')

The sequence could be:

1. the reveal click succeeds
2. the page starts inserting the new field
3. fill() begins before the field exists
4. GuessLocator checks the field's id and name
5. neither currently matches
6. it falls back to an exact visible-text locator
7. Playwright waits on the wrong locator and eventually times out

On the original implementation, the outer expectation retry loop reran the entire callback. A later attempt reran GuessLocator after the field had appeared, so it selected the correct input.

My initial run-once change removed this accidental locator retry as well as the unsafe action retry. This caused deterministic regressions for actions such as type() or fill() immediately after revealing a field.

The problem was therefore not that actions should be retried. The problem was that locator resolution and action execution were coupled together.

The noWaitAfter option helps, but this is deprecated by Playwright and only addresses navigation-related clicks, does not address duplicate typing, filling, checking or other actions, leaves the eager locator-resolution race in place and it requires individual tests to know when to opt out of navigation waiting.

The implementation now separates the operation into distinct stages:

1. resolve the correct locator, retrying only this read-only stage when necessary
2. execute the state-changing browser action once
3. continue retrying assertions independently

In other words:

Resolve locator → perform action once → assert the result

rather than:

Retry locator resolution + action + result together

The generic locator-guessing behaviour has been extended with intent-specific resolvers:

• forClickable()
• forField()
• forCheckable()
• forSelectable()

Each resolver understands what type of element the calling operation requires.

Clickable controls

forClickable() is used for operations such as clicking and pressing buttons.

It can consider appropriate targets such as:

• explicit selectors
• data-test selectors
• IDs
• names
• buttons
• links
• visible text where appropriate

Editable fields

forField() is used for operations such as:

• type()
• typeSlow()
• fill()
• append()
• clear()
• attach()

It considers field-appropriate candidates such as:

• explicit selectors
• data-test selectors
• IDs
• names
• labels
• placeholders

It does not fall back to an unrelated visible text node merely because that text matches the field name.

Checkable controls

forCheckable() is used for:

• radio()
• check()
• uncheck()

It resolves checkbox and radio controls using appropriate selectors, labels and roles. Where a value is supplied, such as:

->radio('contact_method', 'email')

the resolver retains the value restriction so the correct control is selected.

Select controls

forSelectable() is used by select().

It resolves an actual selectable control through explicit selectors, data-test attributes, IDs, names or associated labels, rather than falling back to ordinary page text.

Retrying locator discovery safely

Locator discovery is read-only, so it can safely be retried while waiting for asynchronously inserted controls.

During each attempt, the resolver checks all suitable candidates and returns when the correct type of element exists.

The state-changing action is not included inside this retry loop.

This means that a delayed field can appear without requiring Pest to repeat the subsequent fill or type operation.

Explicit selectors and data-test selectors remain lazy Playwright locators where possible, allowing Playwright's own actionability waiting to operate normally.

Executing actions once

After the correct locator has been resolved, the browser action is executed once using the configured Playwright/browser timeout.

This prevents:

• duplicate clicks;
• duplicate form submissions;
• repeated text entry;
• repeated checkbox or radio changes;
• retrying an action after navigation has begun;
• actions being repeated against a page that has already changed.

Assertions and read-style expectations continue to use Execution::waitForExpectation() because retrying an observation is safe and remains necessary for asynchronous page updates.

I have also added Regression tests to cover both the original problem and the regression identified during review.

The tests verify action counts where possible rather than only checking the final page state. This ensures that the fix does not merely reach the correct result after performing an action multiple times.