fix(Page.waitForSelector): "visible" option should check parent visib…

…ility (#1354) This patch starts checking for boundingClientRect to make sure that the element is visible.
puppeteer · Nov 10, 2017 · f8d19e7 · f8d19e7
1 parent 44d1e83
commit f8d19e7
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 2 deletions.
diff --git a/lib/FrameManager.js b/lib/FrameManager.js
@@ -502,8 +502,16 @@ class Frame {
       if (!waitForVisible && !waitForHidden)
         return true;
       const style = window.getComputedStyle(node);
-      const isVisible = style && style.display !== 'none' && style.visibility !== 'hidden';
+      const isVisible = style && style.visibility !== 'hidden' && hasVisibleBoundingBox();
       return (waitForVisible === isVisible || waitForHidden === !isVisible);
+
+      /**
+       * @return {boolean}
+       */
+      function hasVisibleBoundingBox() {
+        const rect = node.getBoundingClientRect();
+        return !!(rect.top || rect.bottom || rect.width || rect.height);
+      }
     }
   }
 

diff --git a/test/test.js b/test/test.js
@@ -752,18 +752,31 @@ describe('Page', function() {
     it('should wait for visible', SX(async function() {
       let divFound = false;
       const waitForSelector = page.waitForSelector('div', {visible: true}).then(() => divFound = true);
-      await page.setContent(`<div style='display: none; visibility: hidden;'></div>`);
+      await page.setContent(`<div style='display: none; visibility: hidden;'>1</div>`);
       expect(divFound).toBe(false);
       await page.evaluate(() => document.querySelector('div').style.removeProperty('display'));
       expect(divFound).toBe(false);
       await page.evaluate(() => document.querySelector('div').style.removeProperty('visibility'));
       expect(await waitForSelector).toBe(true);
       expect(divFound).toBe(true);
     }));
+    it('should wait for visible recursively', SX(async function() {
+      let divVisible = false;
+      const waitForSelector = page.waitForSelector('div#inner', {visible: true}).then(() => divVisible = true);
+      await page.setContent(`<div style='display: none; visibility: hidden;'><div id="inner">hi</div></div>`);
+      expect(divVisible).toBe(false);
+      await page.evaluate(() => document.querySelector('div').style.removeProperty('display'));
+      expect(divVisible).toBe(false);
+      await page.evaluate(() => document.querySelector('div').style.removeProperty('visibility'));
+      expect(await waitForSelector).toBe(true);
+      expect(divVisible).toBe(true);
+    }));
     it('hidden should wait for visibility: hidden', SX(async function() {
       let divHidden = false;
       await page.setContent(`<div style='display: block;'></div>`);
       const waitForSelector = page.waitForSelector('div', {hidden: true}).then(() => divHidden = true);
+      await page.waitForSelector('div'); // do a round trip
+      expect(divHidden).toBe(false);
       await page.evaluate(() => document.querySelector('div').style.setProperty('visibility', 'hidden'));
       expect(await waitForSelector).toBe(true);
       expect(divHidden).toBe(true);
@@ -772,6 +785,8 @@ describe('Page', function() {
       let divHidden = false;
       await page.setContent(`<div style='display: block;'></div>`);
       const waitForSelector = page.waitForSelector('div', {hidden: true}).then(() => divHidden = true);
+      await page.waitForSelector('div'); // do a round trip
+      expect(divHidden).toBe(false);
       await page.evaluate(() => document.querySelector('div').style.setProperty('display', 'none'));
       expect(await waitForSelector).toBe(true);
       expect(divHidden).toBe(true);
@@ -780,6 +795,7 @@ describe('Page', function() {
       await page.setContent(`<div></div>`);
       let divRemoved = false;
       const waitForSelector = page.waitForSelector('div', {hidden: true}).then(() => divRemoved = true);
+      await page.waitForSelector('div'); // do a round trip
       expect(divRemoved).toBe(false);
       await page.evaluate(() => document.querySelector('div').remove());
       expect(await waitForSelector).toBe(true);