Skip to content

fix: fill a field once, even when an attempt runs out after the text was sent - #302

Open
CharlieEtienne wants to merge 1 commit into
pestphp:5.xfrom
CharlieEtienne:fix/fill-once
Open

CharlieEtienne wants to merge 1 commit into
pestphp:5.xfrom
CharlieEtienne:fix/fill-once

Conversation

@CharlieEtienne

Copy link
Copy Markdown

What happens

fill() and type() still run through Execution::waitForExpectation(), in one-second attempts. Playwright's fill selects the field's text, then inserts the value. When an attempt runs out after the insertion was sent (the page is busy, e.g. right after a click that opened it), the next attempt fills again. If that attempt's selection runs before the first insertion lands, both insertions end up in the field.

On our CI, ->fill('#answer-remarque', 'À finir demain') right after a click that opens the page left À finir demainÀ finir demain in the field (plugin 5.1.2, Playwright 1.62.1).

#269 kept fill and type in the loop because they looked idempotent. This race means they are not.

The change

  • fill and type join the methods that are not retried, next to click and append. Playwright waits for the field to be visible, enabled and editable within that one call.
  • Finding the field is still retried. GuessLocator checks [id] and [name] once, then falls back to getByText(). Taken out of the loop as they are, a fill right after the click that reveals its field would wait the whole timeout on that text locator. Avoid retrying side-effecting browser actions #227 ran into this. type() now retries only the lookup, which is read-only, until the field is on the page, then fills once.

Tests

Two tests in FillTest:

  • An input whose first input event holds the page for 1.5 s counts its input events, for fill() and type(). It counts 2 on 5.x and 1 with this change.
  • A field revealed 500 ms after a click gets filled. This passes on 5.x, and times out if fill and type are only added to the list.

Whether the text ends up doubled depends on which of the two lands first in the renderer, so the test can't force it. The first test counts the second insertion instead.

The same tests in our app's suite, on 5.1.2 (Playwright 1.62.1, PHP 8.5):

input events per fill field revealed by a click
unchanged 2 passes, after one failed attempt on the text locator (1.0 s)
fill/type added to the list only 1 times out
this change 1 passes (0.5 s)

Checks

composer test passes locally on macOS with PHP 8.5 and Playwright 1.63.0: Rector, Pint, profanity, 100% type coverage, PHPStan, and 434 tests. Against unchanged 5.x source, the two "fills only once" tests fail with '2'. The test for the revealed field times out when fill and type are only added to the list.

🤖 Generated with Claude Code

…was sent

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CaioBGalvao

Copy link
Copy Markdown

Independent reproduction on 5.2.0 (Playwright 1.63.0, Chromium headless shell 1243, PHP 8.5), in a different app: an Inertia/React login form. Under CPU contention, type('password', 'password') left passwordpassword in the field. The symptom was only "These credentials do not match our records.", since the field is masked. We found it by logging the field's value length (16) right before press().

Measured on our parallel browser suite, with one busy loop per core:

plugin sign-in failures from a doubled value
5.2.0 unchanged 4 of 20 rounds
fill/type only added to nonAwaitableMethods 0 of 12, but type() right after visit() then times out on the text locator, as your table shows
this PR's diff as a local patch on 5.2.0 0 of 8

One thing that may deserve a note in the description or the upgrade notes: a fill now gets a single attempt of the configured timeout. The retry loop used to stretch that to about twice the timeout. On a CPU-starved runner, a single fill took up to ~8 s, so suites on the 5 s default can see new Timeout 5000ms exceeded on fill/type until they raise pest()->browser()->timeout(). That's still the right trade: a slow fill fails loudly instead of silently typing the value twice.

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