diff --git a/packages/injected/src/recorder/recorder.ts b/packages/injected/src/recorder/recorder.ts index 91f146ea1ab61..ca6805ed7bd43 100644 --- a/packages/injected/src/recorder/recorder.ts +++ b/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); diff --git a/packages/isomorphic/codegen/actions.d.ts b/packages/isomorphic/codegen/actions.d.ts index f6dbc6b51a115..73c28873bd72c 100644 --- a/packages/isomorphic/codegen/actions.d.ts +++ b/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; }; diff --git a/packages/isomorphic/codegen/language.ts b/packages/isomorphic/codegen/language.ts index eb3df508efd3d..ee71f98ceb3cf 100644 --- a/packages/isomorphic/codegen/language.ts +++ b/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, diff --git a/packages/playwright-core/src/server/recorder.ts b/packages/playwright-core/src/server/recorder.ts index 5ef7ffc08f1e3..63044779ba239 100644 --- a/packages/playwright-core/src/server/recorder.ts +++ b/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 implements Instrume name: 'closePage', signals: [], }, - startTime: monotonicTime() }); this._filePrimaryURLChanged(); }); @@ -523,7 +521,6 @@ export class Recorder extends EventEmitter implements Instrume url: page.mainFrame().url(), signals: [], }, - startTime: monotonicTime() }); } this._filePrimaryURLChanged(); @@ -551,7 +548,6 @@ export class Recorder extends EventEmitter implements Instrume const actionInContext: actions.ActionInContext = { pageGuid: frame._page.guid, action, - startTime: monotonicTime(), }; return actionInContext; } @@ -562,12 +558,8 @@ export class Recorder extends EventEmitter 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) { diff --git a/packages/playwright-core/src/server/recorder/recorderSignalProcessor.ts b/packages/playwright-core/src/server/recorder/recorderSignalProcessor.ts index fb510fe657f26..542538a9a1b12 100644 --- a/packages/playwright-core/src/server/recorder/recorderSignalProcessor.ts +++ b/packages/playwright-core/src/server/recorder/recorderSignalProcessor.ts @@ -29,6 +29,7 @@ export interface ProcessorDelegate { export class RecorderSignalProcessor { private _delegate: ProcessorDelegate; private _lastAction: actions.ActionInContext | null = null; + private _lastActionTimestamp = 0; constructor(actionSink: ProcessorDelegate) { this._delegate = actionSink; @@ -36,11 +37,11 @@ export class RecorderSignalProcessor { 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, }); } } diff --git a/packages/playwright-core/src/server/recorder/recorderUtils.ts b/packages/playwright-core/src/server/recorder/recorderUtils.ts index fd66b37da8c9e..531e2939504a4 100644 --- a/packages/playwright-core/src/server/recorder/recorderUtils.ts +++ b/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; } diff --git a/tests/library/debug-controller.spec.ts b/tests/library/debug-controller.spec.ts index 0bdb25cc24658..aa572a994d1f6 100644 --- a/tests/library/debug-controller.spec.ts +++ b/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. diff --git a/tests/library/inspector/recorder-api.spec.ts b/tests/library/inspector/recorder-api.spec.ts index 568c09df57663..d71998b52c666 100644 --- a/tests/library/inspector/recorder-api.spec.ts +++ b/tests/library/inspector/recorder-api.spec.ts @@ -52,8 +52,7 @@ test('should click', async ({ context, browserName, platform, channel }) => { await page.setContent(``); 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(``); 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(``); 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,8 +118,7 @@ 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', @@ -133,11 +126,10 @@ test('should type', async ({ context }) => { 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(``); 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); });