Closed Bug 1545724 Opened 6 years ago Closed 5 years ago

Handle beforeunload dialogs by implementing Page.handleJavaScriptDialog and Page.javascriptDialogOpening

Categories

(Remote Protocol :: CDP, enhancement, P1)

Product:
Remote Protocol
Component:
CDP
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ochameau, Assigned: jdescottes)

References

Details

Attachments

(1 file, 9 obsolete files)

Bug 1545724 - Add support for javascriptDialog APIs in Page domain
47 bytes, text/x-phabricator-request
Details | Review

When the test page registers a beforeunload event, a confirmation dialog appears whenever we navigates away from the current document. As a side effect, various APIs ends up being blocked and freeze JS execution call synchronously (for example docshell.loadURI and docshell.reload will freeze).

It looks like Puppeteer/chromium doesn't disable them.
Instead, there is some API to know when a dialog is emitted and another one to control the dialogs.

Here is how Wordpress test suite handles these dialogs:
https://github.com/WordPress/gutenberg/blob/master/packages/e2e-test-utils/src/enable-page-dialog-accept.js#L7-L14

This ends up relying on the two following CDP method and event:

https://chromedevtools.github.io/devtools-protocol/tot/Page#method-handleJavaScriptDialog
Page.handleJavaScriptDialog
Accepts or dismisses a JavaScript initiated dialog (alert, confirm, prompt, or onbeforeunload).

https://chromedevtools.github.io/devtools-protocol/tot/Page#event-javascriptDialogOpening
Page.javascriptDialogOpening
Fired when a JavaScript initiated dialog (alert, confirm, prompt, or onbeforeunload) is about to open.

It would be better if Puppeteer would prevent these from opening
in the first place, as it seems like a decently sized footgun to
push the responsibility of manually handling UA specific chrome-level
popup dialogues on to the user.

In any case, I suppose setting dom.disable_beforeunload will help
you circumvent this problem if you don’t want to implement
Page.handleJavaScriptDialog and Page.javascriptDialogOpening
right now.

I hope this helps.

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #1)

It would be better if Puppeteer would prevent these from opening
in the first place, as it seems like a decently sized footgun to
push the responsibility of manually handling UA specific chrome-level
popup dialogues on to the user.

In any case, I suppose setting dom.disable_beforeunload will help
you circumvent this problem if you don’t want to implement
Page.handleJavaScriptDialog and Page.javascriptDialogOpening
right now.

Well, that's what I did locally to make some progress on getting the test greener.
We may do if we want to ignore that API for now.
But I'm not convinced that's what we should do in the long run as Puppeteer:

  • exposes an API to know about and control these dialogs (I imagine that setting this pref will prevent implementing this API?),
  • do not try to disable these dialogs.

You can see puppeteer's test against this API here:
https://github.com/GoogleChrome/puppeteer/blob/e3abb0aa32e38660abffad22d6446473be4b13cd/test/page.spec.js#L54-L64

Now, if you feel Puppeteer shouldn't do that, we should propobly open an issue against Puppeteer?

Indeed, in the long run I don’t see it as a viable option to ignore
a deployed, user-facing API.

Since it is relatively easy for us to support this API, I don’t
feel like making a case for its removal from Puppeteer for no better
reason than “API cleanliness”.

Priority: -- → P3
Priority: P3 → P2

Actually, there is various other tests that depend on this, but for regular dialogs: alert() and prompt().

For example, the two following tests are dismissing one prompt and an alert:
https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/specs/change-detection.test.js#L30-L39
https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/specs/reusable-blocks.test.js#L13-L17

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1

Just an example patch that notifies the page when an alert appears and knows how to close dialogs as well.

Can I get your opinion on the WIP at https://phabricator.services.mozilla.com/D34931 ?
The approach I took here was to listen to dialog events and to allow to interact with the dialog, rather than mocking it.
In terms of interaction for now it only supports closing the dialog.

I had to listen to the events in the parent process. I initially wanted to attach the listeners in the parent page domain, but since it is lazily instantiated, I can't do that. That's why I used the TabSession for now.

Let me know what you think

Flags: needinfo?(poirot.alex)

(In reply to Julian Descottes [:jdescottes] from comment #7)

Can I get your opinion on the WIP at https://phabricator.services.mozilla.com/D34931 ?

Sorry for not getting back to you sooner, I completely ignored this feedback request (I now tend to only look at phabricator).

The approach I took here was to listen to dialog events and to allow to interact with the dialog, rather than mocking it.

I can only agree with that. The mocking done by tests are a pure nightmare. I didn't knew there was existing event to listen for dialogs.
It would be great to have a review from the peers maitaining this code for the additional tweaks.
We may want to expose high level API in order to prevent manipulating DOM elements directly from /remote/ codebase.

I had to listen to the events in the parent process. I initially wanted to attach the listeners in the parent page domain, but since it is lazily instantiated, I can't do that. That's why I used the TabSession for now.

These Page.javascriptDialogOpening events should only be emitted when Page.enable is called.
So you should be able to register the listener from this method.
So far, I've not seen any Domain emitting events before its related enable method is called.

Flags: needinfo?(poirot.alex)

Depends on D36623
Very WIP. All domains are now parent domains by default.
Added a ParentProcessDomain base class. The parent domain is now responsible for checking if the method should be called on parent domain or on content domain.
Detection of unsupported methods slightly regressed here to simplify things. Also don't detect missing content domains at the moment.

Depends on D36625
Not actually related to 1545724, should be 1562740, just here to illustrate usage of parent->content domain communication

Attachment #9075408 - Attachment is obsolete: true
Attachment #9071996 - Attachment is obsolete: true
Blocks: 1562740

Comment on attachment 9075412 [details]
Bug 1545724 - Wait for key event in the content and wait for next tick

Revision D36626 was moved to bug 1562740. Setting attachment 9075412 [details] to obsolete.

Attachment #9075412 - Attachment is obsolete: true

Depends on D36626

In theory the mochitest no longer needs to wait for so many events

Comment on attachment 9075605 [details]
Bug 1545724 - Simplify dispatchKeyEvent test and stop waiting for content events in the test

Revision D36727 was moved to bug 1562740. Setting attachment 9075605 [details] to obsolete.

Attachment #9075605 - Attachment is obsolete: true

Comment on attachment 9075606 [details]
Bug 1545724 - Add test for race condition when using dispatchKeyEvent

Revision D36728 was moved to bug 1562740. Setting attachment 9075606 [details] to obsolete.

Attachment #9075606 - Attachment is obsolete: true

Hi Gijs,

I just added a question for you related to CommonDialog.jsm on https://phabricator.services.mozilla.com/D36678#inline-222164,

Thanks!

Flags: needinfo?(gijskruitbosch+bugs)

I responded on phab

Flags: needinfo?(gijskruitbosch+bugs)

Depends on D36625

Support all dialog types, only works with tabmodal dialogs.
No change needed in CommonDialog.

Depends on D37000

Tests for all supported javascript dialogs (alert, confirm, prompt and beforeunload)

Attachment #9075410 - Attachment is obsolete: true
Attachment #9076087 - Attachment is obsolete: true
Attachment #9076085 - Attachment is obsolete: true
Attachment #9075509 - Attachment is obsolete: true
Blocks: 1569578
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/70e6319a82f7 Add support for javascriptDialog APIs in Page domain r=remote-protocol-reviewers,whimboo,ochameau

https://hg.mozilla.org/mozilla-central/rev/70e6319a82f7

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Component: CDP: Page → CDP
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: