From 61ff52704c3980ab42e8ebb8ea301708420f2066 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 5 Nov 2021 17:31:28 -0700 Subject: [PATCH] feat(input): perform hit target check during input (#9546) This replaces previous `checkHitTarget` heuristic that took place before the action with a new `setupHitTargetInterceptor` that works during the action: - Before the action we set up capturing listeners on the window. - During the action we ensure that event target is the element we expect to interact with. - After the action we clear the listeners. This should catch the "layout shift" issues where things move between action point calculation and the actual action. Possible issues: - **Risk:** `{ trial: true }` might dispatch move events like `mousemove` or `pointerout`, because we do actually move the mouse but prevent all other events. - **Timing**: The timing of "hit target check" has moved, so this may affect different web pages in different ways, for example expose more races. In this case, we should retry the click as before. - **No risk**: There is still a possibility of mis-targeting with iframes shifting around, because we only intercept in the target frame. This behavior does not change. There is an opt-out environment variable PLAYWRIGHT_NO_LAYOUT_SHIFT_CHECK that reverts to previous behavior. --- packages/playwright-core/src/server/dom.ts | 70 +++++++++++- .../src/server/injected/injectedScript.ts | 83 +++++++++++++- tests/hit-target.spec.ts | 108 ++++++++++++++++++ tests/page/page-click-timeout-3.spec.ts | 17 +++ tests/page/page-click-timeout-4.spec.ts | 16 +++ tests/tap.spec.ts | 3 +- 6 files changed, 287 insertions(+), 10 deletions(-) create mode 100644 tests/hit-target.spec.ts diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index 0f44f4da52..0f5370fdb7 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -19,7 +19,7 @@ import * as injectedScriptSource from '../generated/injectedScriptSource'; import * as channels from '../protocol/channels'; import { isSessionClosedError } from './common/protocolError'; import * as frames from './frames'; -import type { InjectedScript, InjectedScriptPoll, LogEntry } from './injected/injectedScript'; +import type { InjectedScript, InjectedScriptPoll, LogEntry, HitTargetInterceptionResult } from './injected/injectedScript'; import { CallMetadata } from './instrumentation'; import * as js from './javascript'; import { Page } from './page'; @@ -367,8 +367,6 @@ export class ElementHandle extends js.JSHandle { continue; } if (typeof result === 'object' && 'hitTargetDescription' in result) { - if (options.force) - throw new Error(`Element does not receive pointer events, ${result.hitTargetDescription} intercepts them`); progress.log(` ${result.hitTargetDescription} intercepts pointer events`); continue; } @@ -407,8 +405,16 @@ export class ElementHandle extends js.JSHandle { if (typeof maybePoint === 'string') return maybePoint; const point = roundPoint(maybePoint); + progress.metadata.point = point; - if (!force) { + if (process.env.PLAYWRIGHT_NO_LAYOUT_SHIFT_CHECK) + return this._finishPointerAction(progress, actionName, point, options, action); + else + return this._finishPointerActionDetectLayoutShift(progress, actionName, point, options, action); + } + + private async _finishPointerAction(progress: Progress, actionName: string, point: types.Point, options: types.PointerActionOptions & types.PointerActionWaitOptions & types.NavigatingActionWaitOptions, action: (point: types.Point) => Promise) { + if (!options.force) { if ((options as any).__testHookBeforeHitTarget) await (options as any).__testHookBeforeHitTarget(); progress.log(` checking that element receives pointer events at (${point.x},${point.y})`); @@ -418,7 +424,6 @@ export class ElementHandle extends js.JSHandle { progress.log(` element does receive pointer events`); } - progress.metadata.point = point; if (options.trial) { progress.log(` trial ${actionName} has finished`); return 'done'; @@ -446,6 +451,61 @@ export class ElementHandle extends js.JSHandle { return 'done'; } + private async _finishPointerActionDetectLayoutShift(progress: Progress, actionName: string, point: types.Point, options: types.PointerActionOptions & types.PointerActionWaitOptions & types.NavigatingActionWaitOptions, action: (point: types.Point) => Promise) { + await progress.beforeInputAction(this); + + let hitTargetInterceptionHandle: js.JSHandle | undefined; + if (!options.force) { + if ((options as any).__testHookBeforeHitTarget) + await (options as any).__testHookBeforeHitTarget(); + + const actionType = (actionName === 'hover' || actionName === 'tap') ? actionName : 'mouse'; + const handle = await this.evaluateHandleInUtility(([injected, node, { actionType, trial }]) => injected.setupHitTargetInterceptor(node, actionType, trial), { actionType, trial: !!options.trial } as const); + if (handle === 'error:notconnected') + return handle; + if (!handle._objectId) + return handle.rawValue() as 'error:notconnected'; + hitTargetInterceptionHandle = handle as any; + progress.cleanupWhenAborted(() => { + // Do not await here, just in case the renderer is stuck (e.g. on alert) + // and we won't be able to cleanup. + hitTargetInterceptionHandle!.evaluate(h => h.stop()).catch(e => {}); + }); + } + + const actionResult = await this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { + if ((options as any).__testHookBeforePointerAction) + await (options as any).__testHookBeforePointerAction(); + progress.throwIfAborted(); // Avoid action that has side-effects. + let restoreModifiers: types.KeyboardModifier[] | undefined; + if (options && options.modifiers) + restoreModifiers = await this._page.keyboard._ensureModifiers(options.modifiers); + progress.log(` performing ${actionName} action`); + await action(point); + if (restoreModifiers) + await this._page.keyboard._ensureModifiers(restoreModifiers); + if (hitTargetInterceptionHandle) { + const stopHitTargetInterception = hitTargetInterceptionHandle.evaluate(h => h.stop()).catch(e => 'done' as const); + if (!options.noWaitAfter) { + // When noWaitAfter is passed, we do not want to accidentally stall on + // non-committed navigation blocking the evaluate. + const hitTargetResult = await stopHitTargetInterception; + if (hitTargetResult !== 'done') + return hitTargetResult; + } + } + progress.log(` ${options.trial ? 'trial ' : ''}${actionName} action done`); + progress.log(' waiting for scheduled navigations to finish'); + if ((options as any).__testHookAfterPointerAction) + await (options as any).__testHookAfterPointerAction(); + return 'done'; + }, 'input'); + if (actionResult !== 'done') + return actionResult; + progress.log(' navigations have finished'); + return 'done'; + } + async hover(metadata: CallMetadata, options: types.PointerActionOptions & types.PointerActionWaitOptions): Promise { const controller = new ProgressController(metadata, this); return controller.run(async progress => { diff --git a/packages/playwright-core/src/server/injected/injectedScript.ts b/packages/playwright-core/src/server/injected/injectedScript.ts index 9206e9c92d..4192e63555 100644 --- a/packages/playwright-core/src/server/injected/injectedScript.ts +++ b/packages/playwright-core/src/server/injected/injectedScript.ts @@ -63,12 +63,17 @@ export type ElementMatch = { capture: Element | undefined; }; +export type HitTargetInterceptionResult = { + stop: () => 'done' | { hitTargetDescription: string }; +}; + export class InjectedScript { private _engines: Map; _evaluator: SelectorEvaluatorImpl; private _stableRafCount: number; private _browserName: string; onGlobalListenersRemoved = new Set<() => void>(); + private _hitTargetInterceptor: undefined | ((event: MouseEvent | PointerEvent | TouchEvent) => void); constructor(stableRafCount: number, browserName: string, customEngines: { name: string, engine: SelectorEngine}[]) { this._evaluator = new SelectorEvaluatorImpl(new Map()); @@ -99,6 +104,7 @@ export class InjectedScript { this._browserName = browserName; this._setupGlobalListenersRemovalDetection(); + this._setupHitTargetInterceptors(); } eval(expression: string): any { @@ -654,19 +660,24 @@ export class InjectedScript { if (!element || !element.isConnected) return 'error:notconnected'; element = element.closest('button, [role=button]') || element; - let hitElement = this.deepElementFromPoint(document, point.x, point.y); + const hitElement = this.deepElementFromPoint(document, point.x, point.y); + return this._expectHitTargetParent(hitElement, element); + } + + private _expectHitTargetParent(hitElement: Element | undefined, targetElement: Element) { const hitParents: Element[] = []; - while (hitElement && hitElement !== element) { + while (hitElement && hitElement !== targetElement) { hitParents.push(hitElement); hitElement = parentElementOrShadowHost(hitElement); } - if (hitElement === element) + if (hitElement === targetElement) return 'done'; - const hitTargetDescription = this.previewNode(hitParents[0]); + const hitTargetDescription = this.previewNode(hitParents[0] || document.documentElement); // Root is the topmost element in the hitTarget's chain that is not in the // element's chain. For example, it might be a dialog element that overlays // the target. let rootHitTargetDescription: string | undefined; + let element: Element | undefined = targetElement; while (element) { const index = hitParents.indexOf(element); if (index !== -1) { @@ -681,6 +692,55 @@ export class InjectedScript { return { hitTargetDescription }; } + setupHitTargetInterceptor(node: Node, action: 'hover' | 'tap' | 'mouse', blockAllEvents: boolean): HitTargetInterceptionResult | 'error:notconnected' { + const maybeElement: Element | null | undefined = node.nodeType === Node.ELEMENT_NODE ? (node as Element) : node.parentElement; + if (!maybeElement || !maybeElement.isConnected) + return 'error:notconnected'; + const element = maybeElement.closest('button, [role=button]') || maybeElement; + + const events = { + 'hover': kHoverHitTargetInterceptorEvents, + 'tap': kTapHitTargetInterceptorEvents, + 'mouse': kMouseHitTargetInterceptorEvents, + }[action]; + let result: 'done' | { hitTargetDescription: string } | undefined; + + const listener = (event: PointerEvent | MouseEvent | TouchEvent) => { + // Ignore events that we do not expect to intercept. + if (!events.has(event.type)) + return; + + // Playwright only issues trusted events, so allow any custom events originating from + // the page or content scripts. + if (!event.isTrusted) + return; + + // Determine the event point. Note that Firefox does not always have window.TouchEvent. + const point = (!!window.TouchEvent && (event instanceof window.TouchEvent)) ? event.touches[0] : (event as MouseEvent | PointerEvent); + if (!!point && (result === undefined || result === 'done')) { + // Determine whether we hit the target element, unless we have already failed. + const hitElement = this.deepElementFromPoint(document, point.clientX, point.clientY); + result = this._expectHitTargetParent(hitElement, element); + } + if (blockAllEvents || result !== 'done') { + event.preventDefault(); + event.stopPropagation(); + event.stopImmediatePropagation(); + } + }; + + const stop = () => { + if (this._hitTargetInterceptor === listener) + this._hitTargetInterceptor = undefined; + return result!; + }; + + // Note: this removes previous listener, just in case there are two concurrent clicks + // or something went wrong and we did not cleanup. + this._hitTargetInterceptor = listener; + return { stop }; + } + dispatchEvent(node: Node, type: string, eventInit: Object) { let event; eventInit = { bubbles: true, cancelable: true, composed: true, ...eventInit }; @@ -798,6 +858,16 @@ export class InjectedScript { }).observe(document, { childList: true }); } + private _setupHitTargetInterceptors() { + const listener = (event: PointerEvent | MouseEvent | TouchEvent) => this._hitTargetInterceptor?.(event); + const addHitTargetInterceptorListeners = () => { + for (const event of kAllHitTargetInterceptorEvents) + window.addEventListener(event as any, listener, { capture: true, passive: false }); + }; + addHitTargetInterceptorListeners(); + this.onGlobalListenersRemoved.add(addHitTargetInterceptorListeners); + } + expectSingleElement(progress: InjectedScriptProgress, element: Element, options: FrameExpectParams): { matches: boolean, received?: any } { const injected = progress.injectedScript; const expression = options.expression; @@ -972,6 +1042,11 @@ const eventType = new Map { + await page.goto(server.PREFIX + '/input/button.html'); + await page.evaluate(() => { + const blocker = document.createElement('div'); + blocker.style.position = 'absolute'; + blocker.style.width = '400px'; + blocker.style.height = '400px'; + blocker.style.left = '0'; + blocker.style.top = '0'; + document.body.appendChild(blocker); + + const allEvents = []; + (window as any).allEvents = allEvents; + for (const name of ['mousedown', 'mouseup', 'click', 'dblclick', 'auxclick', 'contextmenu', 'pointerdown', 'pointerup']) { + window.addEventListener(name, e => allEvents.push(e.type)); + blocker.addEventListener(name, e => allEvents.push(e.type)); + } + }); + + const error = await page.click('button', { timeout: 1000 }).catch(e => e); + expect(error.message).toContain('page.click: Timeout 1000ms exceeded.'); + + // Give it some time, just in case. + await page.waitForTimeout(1000); + const allEvents = await page.evaluate(() => (window as any).allEvents); + expect(allEvents).toEqual([]); +}); + +it('should block click when mousedown succeeds but mouseup fails', async ({ page, server }) => { + await page.goto(server.PREFIX + '/input/button.html'); + await page.$eval('button', button => { + button.addEventListener('mousedown', () => { + button.style.marginLeft = '100px'; + }); + + const allEvents = []; + (window as any).allEvents = allEvents; + for (const name of ['mousedown', 'mouseup', 'click', 'dblclick', 'auxclick', 'contextmenu', 'pointerdown', 'pointerup']) + button.addEventListener(name, e => allEvents.push(e.type)); + }); + + await page.click('button'); + expect(await page.evaluate('result')).toBe('Clicked'); + const allEvents = await page.evaluate(() => (window as any).allEvents); + expect(allEvents).toEqual([ + // First attempt failed. + 'pointerdown', 'mousedown', + // Second attempt succeeded. + 'pointerdown', 'mousedown', 'pointerup', 'mouseup', 'click', + ]); +}); + +it('should not block programmatic events', async ({ page, server }) => { + await page.goto(server.PREFIX + '/input/button.html'); + await page.$eval('button', button => { + button.addEventListener('mousedown', () => { + button.style.marginLeft = '100px'; + button.dispatchEvent(new MouseEvent('click')); + }); + + const allEvents = []; + (window as any).allEvents = allEvents; + button.addEventListener('click', e => { + if (!e.isTrusted) + allEvents.push(e.type); + }); + }); + + await page.click('button'); + expect(await page.evaluate('result')).toBe('Clicked'); + const allEvents = await page.evaluate(() => (window as any).allEvents); + // We should get one programmatic click on each attempt. + expect(allEvents).toEqual([ + 'click', 'click', + ]); +}); + +it('should click the button again after document.write', async ({ page, server }) => { + await page.goto(server.PREFIX + '/input/button.html'); + await page.click('button'); + expect(await page.evaluate('result')).toBe('Clicked'); + + await page.evaluate(() => { + document.open(); + document.write(''); + document.close(); + }); + await page.click('button'); + expect(await page.evaluate('result2')).toBe(true); +}); diff --git a/tests/page/page-click-timeout-3.spec.ts b/tests/page/page-click-timeout-3.spec.ts index 6c0747f223..e07f69edfe 100644 --- a/tests/page/page-click-timeout-3.spec.ts +++ b/tests/page/page-click-timeout-3.spec.ts @@ -57,6 +57,23 @@ it('should timeout waiting for hit target', async ({ page, server }) => { expect(error.message).toContain('waiting 500ms'); }); +it('should still click when force but hit target is obscured', async ({ page, server }) => { + await page.goto(server.PREFIX + '/input/button.html'); + const button = await page.$('button'); + await page.evaluate(() => { + document.body.style.position = 'relative'; + const blocker = document.createElement('div'); + blocker.id = 'blocker'; + blocker.style.position = 'absolute'; + blocker.style.width = '400px'; + blocker.style.height = '200px'; + blocker.style.left = '0'; + blocker.style.top = '0'; + document.body.appendChild(blocker); + }); + await button.click({ force: true }); +}); + it('should report wrong hit target subtree', async ({ page, server }) => { await page.goto(server.PREFIX + '/input/button.html'); const button = await page.$('button'); diff --git a/tests/page/page-click-timeout-4.spec.ts b/tests/page/page-click-timeout-4.spec.ts index 83b5ef3746..dcd490ab45 100644 --- a/tests/page/page-click-timeout-4.spec.ts +++ b/tests/page/page-click-timeout-4.spec.ts @@ -29,3 +29,19 @@ it('should timeout waiting for stable position', async ({ page, server }) => { expect(error.message).toContain('waiting for element to be visible, enabled and stable'); expect(error.message).toContain('element is not stable - waiting'); }); + +it('should click for the second time after first timeout', async ({ page, server, mode }) => { + it.skip(mode !== 'default'); + + await page.goto(server.PREFIX + '/input/button.html'); + const __testHookBeforePointerAction = () => new Promise(f => setTimeout(f, 1500)); + const error = await page.click('button', { timeout: 1000, __testHookBeforePointerAction } as any).catch(e => e); + expect(error.message).toContain('page.click: Timeout 1000ms exceeded.'); + + expect(await page.evaluate('result')).toBe('Was not clicked'); + await page.waitForTimeout(2000); + expect(await page.evaluate('result')).toBe('Was not clicked'); + + await page.click('button'); + expect(await page.evaluate('result')).toBe('Clicked'); +}); diff --git a/tests/tap.spec.ts b/tests/tap.spec.ts index a530d73890..ad080b059a 100644 --- a/tests/tap.spec.ts +++ b/tests/tap.spec.ts @@ -48,7 +48,8 @@ it('trial run should not tap', async ({ page }) => { await page.tap('#a'); const eventsHandle = await trackEvents(await page.$('#b')); await page.tap('#b', { trial: true }); - expect(await eventsHandle.jsonValue()).toEqual([]); + const expected = process.env.PLAYWRIGHT_NO_LAYOUT_SHIFT_CHECK ? [] : ['pointerover', 'pointerenter', 'pointerout', 'pointerleave']; + expect(await eventsHandle.jsonValue()).toEqual(expected); }); it('should not send mouse events touchstart is canceled', async ({ page }) => {