Skip to content

Conversation

hbenl
Copy link
Contributor

@hbenl hbenl commented Aug 26, 2025

Playwright expects page.close({ runBeforeUnload: true }) to resolve while the beforeunload dialog is still open but the BiDi command browsingContext.close only resolves after the dialog has been accepted or dismissed, so the implementation of page.close() should not await the response for browsingContext.close in this case.

Fixes:

  • tests/library/beforeunload.spec.ts:
    • "should run beforeunload if asked for smoke"
    • "should access page after beforeunload"
  • tests/library/headful.spec.ts:
    • "should close browsercontext with pending beforeunload dialog"

Copy link
Contributor

Test results for "tests 1"

3 flaky ⚠️ [firefox-library] › library/browsercontext-reuse.spec.ts:114 › reuse launch › should reset serviceworker `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [chromium-library] › library/inspector/cli-codegen-pick-locator.spec.ts:35 › should update locator highlight `@ubuntu-22.04-chromium-tip-of-tree`

32446 passed, 623 skipped


Merge workflow run.

@whimboo
Copy link
Collaborator

whimboo commented Aug 26, 2025

Please see #34281 when we as well tried to fix that. @juliandescottes do you remember where we ended-up with that?

@juliandescottes
Copy link
Contributor

Not much to add compared to what can be read on the other PR. For the promptUnload bit, this PR is a quite similar to mine, but let's see.

BiDi spec issue is at w3c/webdriver-bidi#852 . We can put it to the agenda, for a next meeting to get feedback from the group. My opinion is that the library using the protocol is the best layer to actually decide whether to wait or not when promptUnload is true. The protocol being opinionated here wouldn't bring much added value.

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.

3 participants