From f6210ae99615db25e8e640609a026e1c7f0902db Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 4 May 2020 16:30:19 -0700 Subject: [PATCH] fix(webkit): click moving targets on windows (#2101) --- package-lock.json | 2 +- src/chromium/crPage.ts | 4 ++++ src/dom.ts | 7 ++++--- src/firefox/ffPage.ts | 4 ++++ src/injected/injected.ts | 18 ++++++++++++++++-- src/page.ts | 1 + src/webkit/wkPage.ts | 4 ++++ test/click.spec.js | 4 ++-- 8 files changed, 36 insertions(+), 8 deletions(-) diff --git a/package-lock.json b/package-lock.json index 27fedbc5a0..5bfacf5733 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "playwright-core", - "version": "0.16.0-post", + "version": "0.17.0-post", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index 871dfa5949..84bc024a47 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -262,6 +262,10 @@ export class CRPage implements PageDelegate { await this._forAllFrameSessions(frame => frame._setActivityPaused(paused)); } + rafCountForStablePosition(): number { + return 1; + } + async getContentQuads(handle: dom.ElementHandle): Promise { return this._sessionForHandle(handle)._getContentQuads(handle); } diff --git a/src/dom.ts b/src/dom.ts index c55fff8e40..fa36e8b094 100644 --- a/src/dom.ts +++ b/src/dom.ts @@ -455,9 +455,10 @@ export class ElementHandle extends js.JSHandle { async _waitForDisplayedAtStablePosition(deadline: number): Promise { this._page._log(inputLog, 'waiting for element to be displayed and not moving...'); - const stablePromise = this._evaluateInUtility(({ injected, node }, timeout) => { - return injected.waitForDisplayedAtStablePosition(node, timeout); - }, helper.timeUntilDeadline(deadline)); + const rafCount = this._page._delegate.rafCountForStablePosition(); + const stablePromise = this._evaluateInUtility(({ injected, node }, { rafCount, timeout }) => { + return injected.waitForDisplayedAtStablePosition(node, rafCount, timeout); + }, { rafCount, timeout: helper.timeUntilDeadline(deadline) }); const timeoutMessage = 'element to be displayed and not moving'; const injectedResult = await helper.waitWithDeadline(stablePromise, timeoutMessage, deadline); handleInjectedResult(injectedResult, timeoutMessage); diff --git a/src/firefox/ffPage.ts b/src/firefox/ffPage.ts index 4416d40707..ff67ec9481 100644 --- a/src/firefox/ffPage.ts +++ b/src/firefox/ffPage.ts @@ -431,6 +431,10 @@ export class FFPage implements PageDelegate { async setActivityPaused(paused: boolean): Promise { } + rafCountForStablePosition(): number { + return 1; + } + async getContentQuads(handle: dom.ElementHandle): Promise { const result = await this._session.send('Page.getContentQuads', { frameId: handle._context.frame._id, diff --git a/src/injected/injected.ts b/src/injected/injected.ts index 45ebe8d255..38a2b1d71e 100644 --- a/src/injected/injected.ts +++ b/src/injected/injected.ts @@ -262,7 +262,7 @@ export class Injected { input.dispatchEvent(new Event('change', { 'bubbles': true })); } - async waitForDisplayedAtStablePosition(node: Node, timeout: number): Promise { + async waitForDisplayedAtStablePosition(node: Node, rafCount: number, timeout: number): Promise { if (!node.isConnected) return { status: 'notconnected' }; const element = node.nodeType === Node.ELEMENT_NODE ? (node as Element) : node.parentElement; @@ -271,6 +271,8 @@ export class Injected { let lastRect: types.Rect | undefined; let counter = 0; + let samePositionCounter = 0; + let lastTime = 0; const result = await this.poll('raf', timeout, (): 'notconnected' | boolean => { // First raf happens in the same animation frame as evaluation, so it does not produce // any client rect difference compared to synchronous call. We skip the synchronous call @@ -279,10 +281,22 @@ export class Injected { return false; if (!node.isConnected) return 'notconnected'; + + // Drop frames that are shorter than 16ms - WebKit Win bug. + const time = performance.now(); + if (rafCount > 1 && time - lastTime < 15) + return false; + lastTime = time; + // Note: this logic should be similar to isVisible() to avoid surprises. const clientRect = element.getBoundingClientRect(); const rect = { x: clientRect.top, y: clientRect.left, width: clientRect.width, height: clientRect.height }; - let isDisplayedAndStable = lastRect && rect.x === lastRect.x && rect.y === lastRect.y && rect.width === lastRect.width && rect.height === lastRect.height && rect.width > 0 && rect.height > 0; + const samePosition = lastRect && rect.x === lastRect.x && rect.y === lastRect.y && rect.width === lastRect.width && rect.height === lastRect.height && rect.width > 0 && rect.height > 0; + if (samePosition) + ++samePositionCounter; + else + samePositionCounter = 0; + let isDisplayedAndStable = samePositionCounter >= rafCount; const style = element.ownerDocument && element.ownerDocument.defaultView ? element.ownerDocument.defaultView.getComputedStyle(element) : undefined; isDisplayedAndStable = isDisplayedAndStable && (!!style && style.visibility !== 'hidden'); lastRect = rect; diff --git a/src/page.ts b/src/page.ts index 21ec69c5b8..9d885f96ff 100644 --- a/src/page.ts +++ b/src/page.ts @@ -70,6 +70,7 @@ export interface PageDelegate { getFrameElement(frame: frames.Frame): Promise; scrollRectIntoViewIfNeeded(handle: dom.ElementHandle, rect?: types.Rect): Promise; setActivityPaused(paused: boolean): Promise; + rafCountForStablePosition(): number; getAccessibilityTree(needle?: dom.ElementHandle): Promise<{tree: accessibility.AXNode, needle: accessibility.AXNode | null}>; pdf?: (options?: types.PDFOptions) => Promise; diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index d88f39bf84..5e34230794 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -764,6 +764,10 @@ export class WKPage implements PageDelegate { async setActivityPaused(paused: boolean): Promise { } + rafCountForStablePosition(): number { + return process.platform === 'win32' ? 5 : 1; + } + async getContentQuads(handle: dom.ElementHandle): Promise { const result = await this._session.send('DOM.getContentQuads', { objectId: toRemoteObject(handle).objectId! diff --git a/test/click.spec.js b/test/click.spec.js index 037c814cc1..3a168b2ba3 100644 --- a/test/click.spec.js +++ b/test/click.spec.js @@ -406,7 +406,7 @@ describe('Page.click', function() { await context.close(); }); - it.fail(WEBKIT && WIN)('should wait for stable position', async({page, server}) => { + it('should wait for stable position', async({page, server}) => { await page.goto(server.PREFIX + '/input/button.html'); await page.$eval('button', button => { button.style.transition = 'margin 500ms linear 0s'; @@ -424,7 +424,7 @@ describe('Page.click', function() { expect(await page.evaluate(() => pageX)).toBe(300); expect(await page.evaluate(() => pageY)).toBe(10); }); - it.fail(WEBKIT && WIN)('should timeout waiting for stable position', async({page, server}) => { + it('should timeout waiting for stable position', async({page, server}) => { await page.goto(server.PREFIX + '/input/button.html'); const button = await page.$('button'); await button.evaluate(button => {