fix!: fix bounding box visibility conditions (#8954)

puppeteer · Sep 15, 2022 · ac9929d · ac9929d
1 parent 64763e9
commit ac9929d
Show file tree

Hide file tree

Showing 4 changed files with 203 additions and 107 deletions.
diff --git a/src/injected/util.ts b/src/injected/util.ts
@@ -19,6 +19,8 @@ export const createFunction = (
   return fn;
 };
 
+const HIDDEN_VISIBILITY_VALUES = ['hidden', 'collapse'];
+
 /**
  * @internal
  */
@@ -38,11 +40,20 @@ export const checkVisibility = (
 
   const style = window.getComputedStyle(element);
   const isVisible =
-    style && style.visibility !== 'hidden' && isBoundingBoxVisible(element);
+    style &&
+    !HIDDEN_VISIBILITY_VALUES.includes(style.visibility) &&
+    isBoundingBoxVisible(element);
   return visible === isVisible ? node : false;
 };
 
 function isBoundingBoxVisible(element: Element): boolean {
   const rect = element.getBoundingClientRect();
-  return !!(rect.top || rect.bottom || rect.width || rect.height);
+  return (
+    rect.width > 0 &&
+    rect.height > 0 &&
+    rect.right > 0 &&
+    rect.bottom > 0 &&
+    rect.left < self.innerWidth &&
+    rect.top < self.innerHeight
+  );
 }
diff --git a/test/src/ariaqueryhandler.spec.ts b/test/src/ariaqueryhandler.spec.ts
@@ -446,7 +446,7 @@ describe('AriaQueryHandler', () => {
 
       let divHidden = false;
       await page.setContent(
-        `<div role='button' style='display: block;'></div>`
+        `<div role='button' style='display: block;'>text</div>`
       );
       const waitForSelector = page
         .waitForSelector('aria/[role="button"]', {hidden: true})
@@ -468,7 +468,9 @@ describe('AriaQueryHandler', () => {
       const {page} = getTestState();
 
       let divHidden = false;
-      await page.setContent(`<div role='main' style='display: block;'></div>`);
+      await page.setContent(
+        `<div role='main' style='display: block;'>text</div>`
+      );
       const waitForSelector = page
         .waitForSelector('aria/[role="main"]', {hidden: true})
         .then(() => {
@@ -488,7 +490,7 @@ describe('AriaQueryHandler', () => {
     it('hidden should wait for removal', async () => {
       const {page} = getTestState();
 
-      await page.setContent(`<div role='main'></div>`);
+      await page.setContent(`<div role='main'>text</div>`);
       let divRemoved = false;
       const waitForSelector = page
         .waitForSelector('aria/[role="main"]', {hidden: true})
@@ -516,13 +518,13 @@ describe('AriaQueryHandler', () => {
     it('should respect timeout', async () => {
       const {page, puppeteer} = getTestState();
 
-      let error!: Error;
-      await page
-        .waitForSelector('aria/[role="button"]', {timeout: 10})
-        .catch(error_ => {
-          return (error = error_);
+      const error = await page
+        .waitForSelector('aria/[role="button"]', {
+          timeout: 10,
+        })
+        .catch(error => {
+          return error;
         });
-      expect(error).toBeTruthy();
       expect(error.message).toContain(
         'Waiting for selector `[role="button"]` failed: Waiting failed: 10ms exceeded'
       );
@@ -532,17 +534,15 @@ describe('AriaQueryHandler', () => {
     it('should have an error message specifically for awaiting an element to be hidden', async () => {
       const {page} = getTestState();
 
-      await page.setContent(`<div role='main'></div>`);
-      let error!: Error;
-      await page
-        .waitForSelector('aria/[role="main"]', {hidden: true, timeout: 10})
-        .catch(error_ => {
-          return (error = error_);
-        });
-      expect(error).toBeTruthy();
-      expect(error.message).toContain(
-        'Waiting for selector `[role="main"]` failed: Waiting failed: 10ms exceeded'
-      );
+      await page.setContent(`<div role='main'>text</div>`);
+      const promise = page.waitForSelector('aria/[role="main"]', {
+        hidden: true,
+        timeout: 10,
+      });
+      await expect(promise).rejects.toMatchObject({
+        message:
+          'Waiting for selector `[role="main"]` failed: Waiting failed: 10ms exceeded',
+      });
     });
 
     it('should respond to node attribute mutation', async () => {

diff --git a/test/src/mocha-utils.ts b/test/src/mocha-utils.ts
@@ -295,3 +295,14 @@ export const shortWaitForArrayToHaveAtLeastNElements = async (
     });
   }
 };
+
+export const createTimeout = <T>(
+  n: number,
+  value?: T
+): Promise<T | undefined> => {
+  return new Promise(resolve => {
+    setTimeout(() => {
+      return resolve(value);
+    }, n);
+  });
+};