Open Bug 1567476 Opened 2 years ago Updated 2 months ago

Browser.close should not trigger onbeforeunload dialogs

Categories

(Remote Protocol :: CDP, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: jdescottes, Unassigned)

References

(Blocks 1 open bug)

Details

Pending confirmation from Chromium peers, the documentation is not really clear on this topic, but juggler implemented Browser.close in a way that bypasses onbeforeunload:

  async close() {
    Services.startup.quit(Ci.nsIAppStartup.eForceQuit);
  }

https://github.com/puppeteer/juggler/blob/master/testing/juggler/protocol/BrowserHandler.js#L17

Instead we used

  close() {
    Services.startup.quit(Ci.nsIAppStartup.eAttemptQuit);
  }

which will trigger onbeforeunload popups.

This seems to be confirmed when running the same gutenberg test involving onbeforeunload prompts side by side between chrome and firefox. The chrome run will trigger 1 less beforeunload event compared to firefox.

Concerning the impact on Gutenberg, this makes any test that triggers onbeforeunload prompts timeout at the end of the suite. This is much more inconvenient when running tests one by one, because it will make each test fail.

Pending confirmation from Chromium peers,
Quoting Andrey on slack: "browser.close() is expected to close browser unconditionally"

I'll move forward with the change here, although I'm not sure how to write a test for this at the moment.
Any suggestion appreciated!

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Type: task → defect
Priority: P2 → P1

Feel free to pick this up!

Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Whiteboard: [puppeteer-alp
Whiteboard: [puppeteer-alp → [puppeteer-alpha]
Priority: P1 → P2

General issue with test pages which have a beforeunload event handler registered. Given that the Gutenberg editor has such a handler setup, we should make sure to be able to close the browser window. Otherwise the dialog remains open, and would block further interaction with the window. As such keeping it on the radar for the alpha release.

Priority: P2 → P3
Whiteboard: [puppeteer-alpha] → [puppeteer-alpha-reserve]
Priority: P3 → P2
Whiteboard: [puppeteer-alpha-reserve] → [puppeteer-beta-mvp]
Assignee: nobody → mjzffr
Priority: P2 → P1
Whiteboard: [puppeteer-beta-mvp] → [puppeteer-beta-mvp][puppeteer-high-priority]

Ugh, sorry for the noise. This isn't blocking gutenberg tests in the way that I thought, after all.

Assignee: mjzffr → nobody
Whiteboard: [puppeteer-beta-mvp][puppeteer-high-priority] → [puppeteer-beta-mvp]
Priority: P1 → P2
Priority: P2 → P3
Whiteboard: [puppeteer-beta-mvp] → [puppeteer-beta-reserve]
Whiteboard: [puppeteer-beta-reserve]
Component: CDP: Browser → CDP
You need to log in before you can comment on or make changes to this bug.