fix(dialogs): let click timeout, log information about dialogs (#2781)

We should not stall selector actions because of dialogs
and properly timeout instead. For this, we should not await
the handle.dispose() call because it will never happen
while dialog is shown.

Also, log information about dialogs to make it easier to debug.

src/chromium/crPage.ts

@@ -650,6 +650,7 @@ class FrameSession {
 
   _onDialog(event: Protocol.Page.javascriptDialogOpeningPayload) {
     this._page.emit(Events.Page.Dialog, new dialog.Dialog(
+        this._page._logger,
         event.type,
         event.message,
         async (accept: boolean, promptText?: string) => {

src/dialog.ts

@@ -16,23 +16,27 @@
  */
 
 import { assert } from './helper';
+import { Logger } from './logger';
 
 type OnHandle = (accept: boolean, promptText?: string) => Promise<void>;
 
 export type DialogType = 'alert' | 'beforeunload' | 'confirm' | 'prompt';
 
 export class Dialog {
+  private _logger: Logger;
   private _type: string;
   private _message: string;
   private _onHandle: OnHandle;
   private _handled = false;
   private _defaultValue: string;
 
-  constructor(type: string, message: string, onHandle: OnHandle, defaultValue?: string) {
+  constructor(logger: Logger, type: string, message: string, onHandle: OnHandle, defaultValue?: string) {
+    this._logger = logger;
     this._type = type;
     this._message = message;
     this._onHandle = onHandle;
     this._defaultValue = defaultValue || '';
+    this._logger.info(`  ${this._preview()} was shown`);
   }
 
   type(): string {
@@ -50,12 +54,22 @@ export class Dialog {
   async accept(promptText: string | undefined) {
     assert(!this._handled, 'Cannot accept dialog which is already handled!');
     this._handled = true;
+    this._logger.info(`  ${this._preview()} was accepted`);
     await this._onHandle(true, promptText);
   }
 
   async dismiss() {
     assert(!this._handled, 'Cannot dismiss dialog which is already handled!');
     this._handled = true;
+    this._logger.info(`  ${this._preview()} was dismissed`);
     await this._onHandle(false);
   }
+
+  private _preview(): string {
+    if (!this._message)
+      return this._type;
+    if (this._message.length <= 50)
+      return `${this._type} "${this._message}"`;
+    return `${this._type} "${this._message.substring(0, 49) + '\u2026'}"`;
+  }
 }

src/firefox/ffPage.ts

@@ -188,6 +188,7 @@ export class FFPage implements PageDelegate {
 
   _onDialogOpened(params: Protocol.Page.dialogOpenedPayload) {
     this._page.emit(Events.Page.Dialog, new dialog.Dialog(
+        this._page._logger,
         params.type,
         params.message,
         async (accept: boolean, promptText?: string) => {

src/frames.ts

@@ -726,7 +726,11 @@ export class Frame {
         const task = dom.waitForSelectorTask(info, 'attached');
         const handle = await this._scheduleRerunnableHandleTask(progress, info.world, task);
         const element = handle.asElement() as dom.ElementHandle<Element>;
-        progress.cleanupWhenAborted(() => element.dispose());
+        progress.cleanupWhenAborted(() => {
+          // Do not await here to avoid being blocked, either by stalled
+          // page (e.g. alert) or unresolved navigation in Chromium.
+          element.dispose();
+        });
         const result = await action(progress, element);
         element.dispose();
         if (result === 'error:notconnected') {

src/webkit/wkPage.ts

@@ -519,12 +519,13 @@ export class WKPage implements PageDelegate {
 
   _onDialog(event: Protocol.Dialog.javascriptDialogOpeningPayload) {
     this._page.emit(Events.Page.Dialog, new dialog.Dialog(
-      event.type as dialog.DialogType,
-      event.message,
-      async (accept: boolean, promptText?: string) => {
-        await this._pageProxySession.send('Dialog.handleJavaScriptDialog', { accept, promptText });
-      },
-      event.defaultPrompt));
+        this._page._logger,
+        event.type as dialog.DialogType,
+        event.message,
+        async (accept: boolean, promptText?: string) => {
+          await this._pageProxySession.send('Dialog.handleJavaScriptDialog', { accept, promptText });
+        },
+        event.defaultPrompt));
   }
 
   private async _onFileChooserOpened(event: {frameId: Protocol.Network.FrameId, element: Protocol.Runtime.RemoteObject}) {

test/click.spec.js

@@ -835,6 +835,14 @@ describe('Page.click', function() {
     await page.click('button');
     expect(await page.evaluate(() => result)).toBe('Clicked');
   });
+  it('should timeout when click opens alert', async({page, server}) => {
+    const dialogPromise = page.waitForEvent('dialog');
+    await page.setContent(`<div onclick='window.alert(123)'>Click me</div>`);
+    const error = await page.click('div', { timeout: 3000 }).catch(e => e);
+    expect(error.message).toContain('Timeout 3000ms exceeded during page.click.');
+    const dialog = await dialogPromise;
+    await dialog.dismiss();
+  });
 });
 
 describe('Page.check', function() {

test/dialog.spec.js

@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-const {FFOX, CHROMIUM, WEBKIT} = require('./utils').testOptions(browserType);
+const {FFOX, CHROMIUM, WEBKIT, CHANNEL} = require('./utils').testOptions(browserType);
 
 describe('Page.Events.Dialog', function() {
   it('should fire', async({page, server}) => {
@@ -58,4 +58,31 @@ describe('Page.Events.Dialog', function() {
     const result = await page.evaluate(() => confirm('boolean?'));
     expect(result).toBe(false);
   });
+  it.fail(CHANNEL)('should log prompt actions', async({browser}) => {
+    const messages = [];
+    const context = await browser.newContext({
+      logger: {
+        isEnabled: () => true,
+        log: (name, severity, message) => messages.push(message),
+      }
+    });
+    const page = await context.newPage();
+    const promise = page.evaluate(() => confirm('01234567890123456789012345678901234567890123456789012345678901234567890123456789'));
+    const dialog = await page.waitForEvent('dialog');
+    expect(messages.join()).toContain('confirm "0123456789012345678901234567890123456789012345678…" was shown');
+    await dialog.accept('123');
+    await promise;
+    expect(messages.join()).toContain('confirm "0123456789012345678901234567890123456789012345678…" was accepted');
+    await context.close();
+  });
+  it.fail(WEBKIT)('should be able to close context with open alert', async({browser}) => {
+    const context = await browser.newContext();
+    const page = await context.newPage();
+    const alertPromise = page.waitForEvent('dialog');
+    await page.evaluate(() => {
+      setTimeout(() => alert('hello'), 0);
+    });
+    await alertPromise;
+    await context.close();
+  });
 });