chore(recorder): unify double click handling, drop action timestamps

packages/injected/src/recorder/recorder.ts

@@ -262,6 +262,14 @@ class RecordActionTool implements RecorderTool {
       return;
     }
 
+    if (event.detail === 1) {
+      // A new click starts here, so the stalled one is not a double click after all.
+      this._commitPendingClickAction();
+    } else {
+      // This click continues a multi-click, which is reported by 'dblclick' instead.
+      this._cancelPendingClickAction();
+    }
+
     const checkbox = asCheckbox(this._recorder.deepEventTarget(event));
     if (checkbox && event.detail === 1) {
       // Interestingly, inputElement.checked is reversed inside this event handler.
@@ -273,8 +281,6 @@ class RecordActionTool implements RecorderTool {
       return;
     }
 
-    this._cancelPendingClickAction();
-
     // Stall click in case we are observing double-click.
     if (event.detail === 1) {
       this._pendingClickAction = {
@@ -741,6 +747,7 @@ class RecordActionTool implements RecorderTool {
 
 class JsonRecordActionTool implements RecorderTool {
   private _recorder: Recorder;
+  private _pendingClickAction: { action: actions.ClickAction, timeout: number } | undefined;
 
   constructor(recorder: Recorder) {
     this._recorder = recorder;
@@ -752,6 +759,7 @@ class JsonRecordActionTool implements RecorderTool {
   }
 
   uninstall() {
+    this._cancelPendingClickAction();
     this._recorder.highlight.install();
   }
 
@@ -767,6 +775,14 @@ class JsonRecordActionTool implements RecorderTool {
     if (this._shouldIgnoreMouseEvent(event))
       return;
 
+    if (event.detail === 1) {
+      // A new click starts here, so the stalled one is not a double click after all.
+      this._commitPendingClickAction();
+    } else {
+      // This click continues a multi-click, which is reported by 'dblclick' instead.
+      this._cancelPendingClickAction();
+    }
+
     const checkbox = asCheckbox(element);
     const { ariaSnapshot, selector, ref } = this._ariaSnapshot(element);
     if (checkbox && event.detail === 1) {
@@ -781,6 +797,35 @@ class JsonRecordActionTool implements RecorderTool {
       return;
     }
 
+    // Stall click in case we are observing double-click.
+    if (event.detail === 1) {
+      this._pendingClickAction = {
+        action: {
+          name: 'click',
+          selector,
+          ref,
+          ariaSnapshot,
+          position: positionForEvent(event),
+          signals: [],
+          button: buttonForEvent(event),
+          modifiers: modifiersForEvent(event),
+          clickCount: event.detail,
+        },
+        timeout: this._recorder.injectedScript.utils.builtins.setTimeout(() => this._commitPendingClickAction(), 200)
+      };
+    }
+  }
+
+  onDblClick(event: MouseEvent) {
+    const element = this._recorder.deepEventTarget(event);
+    if (isRangeInput(element))
+      return;
+    if (this._shouldIgnoreMouseEvent(event))
+      return;
+
+    this._cancelPendingClickAction();
+
+    const { ariaSnapshot, selector, ref } = this._ariaSnapshot(element);
     void this._recorder.recordAction({
       name: 'click',
       selector,
@@ -794,6 +839,18 @@ class JsonRecordActionTool implements RecorderTool {
     });
   }
 
+  private _commitPendingClickAction() {
+    if (this._pendingClickAction)
+      void this._recorder.recordAction(this._pendingClickAction.action);
+    this._cancelPendingClickAction();
+  }
+
+  private _cancelPendingClickAction() {
+    if (this._pendingClickAction)
+      this._recorder.injectedScript.utils.builtins.clearTimeout(this._pendingClickAction.timeout);
+    this._pendingClickAction = undefined;
+  }
+
   onContextMenu(event: MouseEvent): void {
     const element = this._recorder.deepEventTarget(event);
     const { ariaSnapshot, selector, ref } = this._ariaSnapshot(element);

packages/isomorphic/codegen/actions.d.ts

@@ -166,12 +166,9 @@ export type Signal = NavigationSignal | PopupSignal | DownloadSignal | DialogSig
 export type ActionInContext = {
   pageGuid: string;
   action: Action;
-  startTime: number;
-  endTime?: number;
 };
 
 export type SignalInContext = {
   pageGuid: string;
   signal: Signal;
-  timestamp: number;
 };

packages/isomorphic/codegen/language.ts

@@ -31,8 +31,6 @@ export function generateCode(actions: actions.ActionInContext[], languageGenerat
 export function expectSignalAction(actionInContext: actions.ActionInContext, signal: actions.ExpectSignal): actions.ActionInContext {
   return {
     pageGuid: actionInContext.pageGuid,
-    startTime: actionInContext.startTime,
-    endTime: actionInContext.startTime,
     action: {
       name: 'assertVisible',
       selector: signal.selector,

packages/playwright-core/src/server/recorder.ts

@@ -22,7 +22,6 @@ import { stringifySelector } from '@isomorphic/selectorParser';
 import { ManualPromise } from '@isomorphic/manualPromise';
 import { isUnderTest } from '@utils/debug';
 import { eventsHelper } from '@utils/eventsHelper';
-import { monotonicTime } from '@isomorphic/time';
 import { BrowserContext } from './browserContext';
 import { Debugger } from './debugger';
 import { buildFullSelector, generateFrameSelector, metadataToCallLog } from './recorder/recorderUtils';
@@ -501,7 +500,6 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
           name: 'closePage',
           signals: [],
         },
-        startTime: monotonicTime()
       });
       this._filePrimaryURLChanged();
     });
@@ -523,7 +521,6 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
           url: page.mainFrame().url(),
           signals: [],
         },
-        startTime: monotonicTime()
       });
     }
     this._filePrimaryURLChanged();
@@ -551,7 +548,6 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
     const actionInContext: actions.ActionInContext = {
       pageGuid: frame._page.guid,
       action,
-      startTime: monotonicTime(),
     };
     return actionInContext;
   }
@@ -562,12 +558,8 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
       this._signalProcessor.signal(frame, { name: 'expect', selector: buildFullSelector(framePath, preconditionSelector) });
     const actionInContext = this._appendContextToAction(frame, action, framePath);
     this._signalProcessor.addAction(actionInContext);
-    try {
-      if (actionInContext.action.name !== 'openPage' && actionInContext.action.name !== 'closePage')
-        await performAction(progress, frame._page.mainFrame(), actionInContext);
-    } finally {
-      actionInContext.endTime = monotonicTime();
-    }
+    if (actionInContext.action.name !== 'openPage' && actionInContext.action.name !== 'closePage')
+      await performAction(progress, frame._page.mainFrame(), actionInContext);
   }
 
   private async _recordAction(progress: Progress, frame: Frame, action: actions.Action) {

packages/playwright-core/src/server/recorder/recorderSignalProcessor.ts

@@ -29,18 +29,19 @@ export interface ProcessorDelegate {
 export class RecorderSignalProcessor {
   private _delegate: ProcessorDelegate;
   private _lastAction: actions.ActionInContext | null = null;
+  private _lastActionTimestamp = 0;
 
   constructor(actionSink: ProcessorDelegate) {
     this._delegate = actionSink;
   }
 
   addAction(actionInContext: actions.ActionInContext) {
     this._lastAction = actionInContext;
+    this._lastActionTimestamp = monotonicTime();
     this._delegate.addAction(actionInContext);
   }
 
   signal(frame: Frame, signal: Signal) {
-    const timestamp = monotonicTime();
     if (signal.name === 'navigation' && frame._page.mainFrame() === frame) {
       const lastAction = this._lastAction;
       const signalThreshold = isUnderTest() ? 500 : 5000;
@@ -50,7 +51,7 @@ export class RecorderSignalProcessor {
         generateGoto = true;
       else if (lastAction.action.name !== 'click' && lastAction.action.name !== 'press' && lastAction.action.name !== 'fill')
         generateGoto = true;
-      else if (timestamp - lastAction.startTime > signalThreshold)
+      else if (monotonicTime() - this._lastActionTimestamp > signalThreshold)
         generateGoto = true;
 
       if (generateGoto) {
@@ -61,8 +62,6 @@ export class RecorderSignalProcessor {
             url: frame.url(),
             signals: [],
           },
-          startTime: timestamp,
-          endTime: timestamp,
         });
       }
       return;
@@ -71,7 +70,6 @@ export class RecorderSignalProcessor {
     this._delegate.addSignal({
       pageGuid: frame._page.guid,
       signal,
-      timestamp,
     });
   }
 }

packages/playwright-core/src/server/recorder/recorderUtils.ts

@@ -62,10 +62,6 @@ function isSameSelector(action: actions.ActionInContext, lastAction: actions.Act
   return 'selector' in action.action && 'selector' in lastAction.action && action.action.selector === lastAction.action.selector;
 }
 
-function isShortlyAfter(action: actions.ActionInContext, lastAction: actions.ActionInContext): boolean {
-  return action.startTime - lastAction.startTime < 500;
-}
-
 export function shouldMergeAction(action: actions.ActionInContext, lastAction: actions.ActionInContext | undefined): boolean {
   if (!lastAction)
     return false;
@@ -74,8 +70,6 @@ export function shouldMergeAction(action: actions.ActionInContext, lastAction: a
       return isSameAction(action, lastAction) && isSameSelector(action, lastAction);
     case 'navigate':
       return isSameAction(action, lastAction);
-    case 'click':
-      return isSameAction(action, lastAction) && isSameSelector(action, lastAction) && isShortlyAfter(action, lastAction) && action.action.clickCount > (lastAction.action as actions.ClickAction).clickCount;
   }
   return false;
 }
@@ -84,14 +78,10 @@ export function collapseActions(actions: actions.ActionInContext[]): actions.Act
   const result: actions.ActionInContext[] = [];
   for (const action of actions) {
     const lastAction = result[result.length - 1];
-    const shouldMerge = shouldMergeAction(action, lastAction);
-    if (!shouldMerge) {
+    if (shouldMergeAction(action, lastAction))
+      result[result.length - 1] = action;
+    else
       result.push(action);
-      continue;
-    }
-    const startTime = result[result.length - 1].startTime;
-    result[result.length - 1] = action;
-    result[result.length - 1].startTime = startTime;
   }
   return result;
 }

tests/library/debug-controller.spec.ts

@@ -225,8 +225,7 @@ test('should record expect signal', async ({ backend, connectedBrowser }) => {
   `);
 
   await page.getByRole('button', { name: 'Show' }).click();
-  // A click stalls for 200ms to detect a double click, and the next click cancels a pending one.
-  await expect.poll(() => events[events.length - 1]?.actions.length).toBe(2);
+  await expect(page.getByRole('button', { name: 'Saved' })).toBeVisible();
   await page.getByRole('button', { name: 'Other' }).click();
 
   // The signal is attached to the "Show" click, so the assertion renders right after it.

tests/library/inspector/recorder-api.spec.ts

@@ -52,8 +52,7 @@ test('should click', async ({ context, browserName, platform, channel }) => {
   await page.setContent(`<button onclick="console.log('click')">Submit</button>`);
   await page.getByRole('button', { name: 'Submit' }).click();
 
-  const clickActions = log.action('click');
-  expect(clickActions).toEqual([
+  await expect.poll(() => log.action('click')).toEqual([
     expect.objectContaining({
       action: expect.objectContaining({
         name: 'click',
@@ -62,11 +61,10 @@ test('should click', async ({ context, browserName, platform, channel }) => {
         // Safari does not focus after a click: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/button#clicking_and_focus
         ariaSnapshot: (browserName === 'webkit' && (platform === 'darwin' || (platform === 'win32' && channel !== 'webkit-wsl'))) ? '- button "Submit" [ref=e2]' : '- button "Submit" [active] [ref=e2]',
       }),
-      startTime: expect.any(Number),
     })
   ]);
 
-  expect(normalizeCode(clickActions[0].code)).toEqual(`await page.getByRole('button', { name: 'Submit' }).click();`);
+  expect(normalizeCode(log.action('click')[0].code)).toEqual(`await page.getByRole('button', { name: 'Submit' }).click();`);
 });
 
 test('should double click', async ({ context, browserName, platform, channel }) => {
@@ -75,8 +73,7 @@ test('should double click', async ({ context, browserName, platform, channel })
   await page.setContent(`<button onclick="console.log('click')" ondblclick="console.log('dblclick')">Submit</button>`);
   await page.getByRole('button', { name: 'Submit' }).dblclick();
 
-  const clickActions = log.action('click');
-  expect(clickActions).toEqual([
+  await expect.poll(() => log.action('click')).toEqual([
     expect.objectContaining({
       action: expect.objectContaining({
         name: 'click',
@@ -86,11 +83,10 @@ test('should double click', async ({ context, browserName, platform, channel })
         // Safari does not focus after a click: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/button#clicking_and_focus
         ariaSnapshot: (browserName === 'webkit' && (platform === 'darwin' || (platform === 'win32' && channel !== 'webkit-wsl'))) ? '- button "Submit" [ref=e2]' : '- button "Submit" [active] [ref=e2]',
       }),
-      startTime: expect.any(Number),
     })
   ]);
 
-  expect(normalizeCode(clickActions[0].code)).toEqual(`await page.getByRole('button', { name: 'Submit' }).dblclick();`);
+  expect(normalizeCode(log.action('click')[0].code)).toEqual(`await page.getByRole('button', { name: 'Submit' }).dblclick();`);
 });
 
 test('should right click', async ({ context, browserName, platform, channel }) => {
@@ -99,8 +95,7 @@ test('should right click', async ({ context, browserName, platform, channel }) =
   await page.setContent(`<button oncontextmenu="console.log('contextmenu')">Submit</button>`);
   await page.getByRole('button', { name: 'Submit' }).click({ button: 'right' });
 
-  const clickActions = log.action('click');
-  expect(clickActions).toEqual([
+  await expect.poll(() => log.action('click')).toEqual([
     expect.objectContaining({
       action: expect.objectContaining({
         name: 'click',
@@ -110,11 +105,10 @@ test('should right click', async ({ context, browserName, platform, channel }) =
         // Safari does not focus after a click: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/button#clicking_and_focus
         ariaSnapshot: (browserName === 'webkit' && (platform === 'darwin' || (platform === 'win32' && channel !== 'webkit-wsl'))) ? '- button "Submit" [ref=e2]' : '- button "Submit" [active] [ref=e2]',
       }),
-      startTime: expect.any(Number),
     })
   ]);
 
-  expect(normalizeCode(clickActions[0].code)).toEqual(`await page.getByRole('button', { name: 'Submit' }).click({ button: 'right' });`);
+  expect(normalizeCode(log.action('click')[0].code)).toEqual(`await page.getByRole('button', { name: 'Submit' }).click({ button: 'right' });`);
 });
 
 test('should type', async ({ context }) => {
@@ -124,20 +118,18 @@ test('should type', async ({ context }) => {
 
   await page.getByRole('textbox').pressSequentially('Hello');
 
-  const fillActions = log.action('fill');
-  expect(fillActions).toEqual([
+  await expect.poll(() => log.action('fill')).toEqual([
     expect.objectContaining({
       action: expect.objectContaining({
         name: 'fill',
         selector: 'internal:role=textbox',
         ref: 'e2',
         ariaSnapshot: '- textbox [active] [ref=e2]: Hello',
       }),
-      startTime: expect.any(Number),
     })
   ]);
 
-  expect(normalizeCode(fillActions[0].code)).toEqual(`await page.getByRole('textbox').fill('Hello');`);
+  expect(normalizeCode(log.action('fill')[0].code)).toEqual(`await page.getByRole('textbox').fill('Hello');`);
 });
 
 test('should disable recorder', async ({ context }) => {
@@ -146,9 +138,11 @@ test('should disable recorder', async ({ context }) => {
   await page.setContent(`<button onclick="console.log('click')">Submit</button>`);
   await page.getByRole('button', { name: 'Submit' }).click();
   await page.getByRole('button', { name: 'Submit' }).click();
-  expect(log.action('click')).toHaveLength(2);
+  await expect.poll(() => log.action('click').length).toBe(2);
   await (context as any)._disableRecorder();
   await page.getByRole('button', { name: 'Submit' }).click();
+  // Make sure no extra action is recorded.
+  await page.waitForTimeout(2000);
   expect(log.action('click')).toHaveLength(2);
 });