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.
puppeteer · Jul 5, 2022 · a58dfda · a58dfda
1 parent b5a345b
commit a58dfda
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 75 deletions.
diff --git a/src/common/ElementHandle.ts b/src/common/ElementHandle.ts
@@ -247,51 +247,9 @@ export class ElementHandle<
   }
 
   async #scrollIntoViewIfNeeded(): Promise<void> {
-    const error = await this.evaluate(
-      async (element, pageJavascriptEnabled): Promise<string | false> => {
-        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
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <title>Button test</title>
+  </head>
+  <body>
+    <script src="mouse-helper.js"></script>
+    <div style="width: 5000px; height: 5000px;"></div>
+    <button onclick="clicked();">Click target</button>
+    <script>
+      window.result = 'Was not clicked';
+      function clicked() {
+        result = 'Clicked';
+      }
+    </script>
+  </body>
+</html>
diff --git 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(`
-        <svg xmlns="http://www.w3.org/2000/svg" width="500" height="500">
-          <rect id="theRect" x="30" y="50" width="200" height="300"></rect>
-        </svg>
-      `);
+         <svg xmlns="http://www.w3.org/2000/svg" width="500" height="500">
+           <rect id="theRect" x="30" y="50" width="200" height="300"></rect>
+         </svg>
+       `);
       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 <br> 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<br>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(
         `<div id=el1>
-          el1
-          <div id=el2>
-            el2
-          </div>
-        </div>
-        <div id=el3>
-          el3
-        </div>`
+           el1
+           <div id=el2>
+             el2
+           </div>
+         </div>
+         <div id=el3>
+           el3
+         </div>`
       );
 
       const el2 = (await page.waitForSelector('#el1'))!;