From a58dfda1d69ea4d87abd548cabd8463059a94a5d Mon Sep 17 00:00:00 2001 From: abozhilov Date: Tue, 5 Jul 2022 21:43:48 +0000 Subject: [PATCH] fix(page): fix page.#scrollIntoViewIfNeeded method This patch fixes page.#scrollIntoViewIfNeededso that it works with devtools protocol. Now it blocks the main thread and waits until the scrolling action finishes. Issues: #8627, #1805 BREAKING CHANGE: page.#scrollIntoViewIfNeeded throws erros which come from the internal implementation. - `Protocol error (DOM.scrollIntoViewIfNeeded): Node is detached from document` - `Protocol error (DOM.scrollIntoViewIfNeeded): Node does not have a layout object` Also now it does work with TextNode properly and does not throw an error. --- src/common/ElementHandle.ts | 48 +------------- test/assets/input/button-not-in-viewport.html | 17 +++++ test/src/elementhandle.spec.ts | 66 ++++++++++--------- 3 files changed, 56 insertions(+), 75 deletions(-) create mode 100644 test/assets/input/button-not-in-viewport.html diff --git a/src/common/ElementHandle.ts b/src/common/ElementHandle.ts index bc4f000459e8e..d812a69049e92 100644 --- a/src/common/ElementHandle.ts +++ b/src/common/ElementHandle.ts @@ -247,51 +247,9 @@ export class ElementHandle< } async #scrollIntoViewIfNeeded(): Promise { - const error = await this.evaluate( - async (element, pageJavascriptEnabled): Promise => { - if (!element.isConnected) { - return 'Node is detached from document'; - } - if (element.nodeType !== Node.ELEMENT_NODE) { - return 'Node is not of type HTMLElement'; - } - // force-scroll if page's javascript is disabled. - if (!pageJavascriptEnabled) { - element.scrollIntoView({ - block: 'center', - inline: 'center', - // @ts-expect-error Chrome still supports behavior: instant but - // it's not in the spec so TS shouts We don't want to make this - // breaking change in Puppeteer yet so we'll ignore the line. - behavior: 'instant', - }); - return false; - } - const visibleRatio = await new Promise(resolve => { - const observer = new IntersectionObserver(entries => { - resolve(entries[0]!.intersectionRatio); - observer.disconnect(); - }); - observer.observe(element); - }); - if (visibleRatio !== 1.0) { - element.scrollIntoView({ - block: 'center', - inline: 'center', - // @ts-expect-error Chrome still supports behavior: instant but - // it's not in the spec so TS shouts We don't want to make this - // breaking change in Puppeteer yet so we'll ignore the line. - behavior: 'instant', - }); - } - return false; - }, - this.#page.isJavaScriptEnabled() - ); - - if (error) { - throw new Error(error); - } + await this._client.send('DOM.scrollIntoViewIfNeeded', { + objectId: this._remoteObject.objectId, + }); } async #getOOPIFOffsets( diff --git a/test/assets/input/button-not-in-viewport.html b/test/assets/input/button-not-in-viewport.html new file mode 100644 index 0000000000000..1a92ec4ecab12 --- /dev/null +++ b/test/assets/input/button-not-in-viewport.html @@ -0,0 +1,17 @@ + + + + Button test + + + +
+ + + + \ No newline at end of file diff --git a/test/src/elementhandle.spec.ts b/test/src/elementhandle.spec.ts index ec59d1d8d7ccc..be9e3073e7fc7 100644 --- a/test/src/elementhandle.spec.ts +++ b/test/src/elementhandle.spec.ts @@ -79,10 +79,10 @@ describe('ElementHandle specs', function () { const {page} = getTestState(); await page.setContent(` - - - - `); + + + + `); const element = (await page.$('#therect'))!; const pptrBoundingBox = await element.boundingBox(); const webBoundingBox = await page.evaluate(e => { @@ -207,11 +207,14 @@ describe('ElementHandle specs', function () { const buttonTextNode = await page.evaluateHandle(() => { return document.querySelector('button')!.firstChild as HTMLElement; }); - let error!: Error; - await buttonTextNode.click().catch(error_ => { - return (error = error_); - }); - expect(error.message).toBe('Node is not of type HTMLElement'); + await buttonTextNode.click(); + + expect( + await page.evaluate(() => { + // @ts-expect-error result is expected to be in the page's scope. + return result; + }) + ).toBe('Clicked'); }); it('should throw for detached nodes', async () => { const {page, server} = getTestState(); @@ -225,7 +228,9 @@ describe('ElementHandle specs', function () { await button.click().catch(error_ => { return (error = error_); }); - expect(error.message).toBe('Node is detached from document'); + expect(error.message).toBe( + 'Protocol error (DOM.scrollIntoViewIfNeeded): Node is detached from document' + ); }); it('should throw for hidden nodes', async () => { const {page, server} = getTestState(); @@ -239,7 +244,7 @@ describe('ElementHandle specs', function () { return error_; }); expect(error.message).toBe( - 'Node is either not clickable or not an HTMLElement' + 'Protocol error (DOM.scrollIntoViewIfNeeded): Node does not have a layout object' ); }); it('should throw for recursively hidden nodes', async () => { @@ -254,20 +259,21 @@ describe('ElementHandle specs', function () { return error_; }); expect(error.message).toBe( - 'Node is either not clickable or not an HTMLElement' + 'Protocol error (DOM.scrollIntoViewIfNeeded): Node does not have a layout object' ); }); - it('should throw for
elements', async () => { - const {page} = getTestState(); + it('should work for button which is not in the viewport', async () => { + const {page, server} = getTestState(); - await page.setContent('hello
goodbye'); - const br = (await page.$('br'))!; - const error = await br.click().catch(error_ => { - return error_; - }); - expect(error.message).toBe( - 'Node is either not clickable or not an HTMLElement' - ); + await page.goto(server.PREFIX + '/input/button-not-in-viewport.html'); + await page.click('button'); + + expect( + await page.evaluate(() => { + // @ts-expect-error result is expected to be in the page's scope. + return result; + }) + ).toBe('Clicked'); }); }); @@ -302,14 +308,14 @@ describe('ElementHandle specs', function () { // Set the page content after the waitFor has been started. await page.setContent( `
- el1 -
- el2 -
-
-
- el3 -
` + el1 +
+ el2 +
+ +
+ el3 +
` ); const el2 = (await page.waitForSelector('#el1'))!;