Implement visible option for Page.waitFor method

This patch adds a 'visible' option to the Page.waitFor method, making
it possible to wait for the element to become actually visible.

References #89, #91.

docs/api.md

@@ -59,7 +59,7 @@
     + [page.url()](#pageurl)
     + [page.userAgent()](#pageuseragent)
     + [page.viewport()](#pageviewport)
-    + [page.waitFor(selector)](#pagewaitforselector)
+    + [page.waitFor(selector[, options])](#pagewaitforselector-options)
     + [page.waitForNavigation(options)](#pagewaitfornavigationoptions)
   * [class: Keyboard](#class-keyboard)
     + [keyboard.down(key[, options])](#keyboarddownkey-options)
@@ -83,7 +83,7 @@
     + [frame.name()](#framename)
     + [frame.parentFrame()](#frameparentframe)
     + [frame.url()](#frameurl)
-    + [frame.waitFor(selector)](#framewaitforselector)
+    + [frame.waitFor(selector[, options])](#framewaitforselector-options)
   * [class: Request](#class-request)
     + [request.headers](#requestheaders)
     + [request.method](#requestmethod)
@@ -593,30 +593,18 @@ This is a shortcut for [page.mainFrame().url()](#frameurl)
 - returns: <[Object]>  An object with the save fields as described in [page.setViewport](#pagesetviewportviewport)
 
 
-#### page.waitFor(selector)
+#### page.waitFor(selector[, options])
 - `selector` <[string]> A query selector to wait for on the page.
+- `options` <[Object]> Optional waiting parameters
+  - `visible` <[boolean]> wait for element to be present in DOM and to be visible, i.e. to not have `display: none` or `visibility: hidden` CSS properties.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` appears in the page.
 
-The `page.waitFor` successfully survives page navigations:
+Shortcut for [page.mainFrame().waitFor(selector)](#framewaitforselector).
-```js
-const {Browser} = new require('puppeteer');
-const browser = new Browser();
-
-browser.newPage().then(async page => {
-  let currentURL;
-  page.waitFor('img').then(() => console.log('First URL with image: ' + currentURL));
-  for (currentURL of ['https://example.com', 'https://google.com', 'https://bbc.com'])
-    await page.navigate(currentURL);
-  browser.close();
-});
-```
 
 #### page.waitForNavigation(options)
 - `options` <[Object]> Navigation parameters, same as in [page.navigate](#pagenavigateurl-options).
 - returns: <[Promise]<[Response]>> Promise which resolves to the main resource response. In case of multiple redirects, the navigation will resolve with the response of the last redirect.
 
-Shortcut for [page.mainFrame().waitFor(selector)](#framewaitforselector).
-
 ### class: Keyboard
 
 Keyboard provides an api for managing a virtual keyboard. The high level api is [`keyboard.type`](#keyboardtypetext), which takes raw characters and generates proper keydown, keypress/input, and keyup events on your page.
@@ -813,14 +801,30 @@ Returns frame's name as specified in the tag.
 
 Returns frame's url.
 
-#### frame.waitFor(selector)
+#### frame.waitFor(selector[, options])
 - `selector` <[string]> CSS selector of awaited element,
+- `options` <[Object]> Optional waiting parameters
+  - `visible` <[boolean]> wait for element to be present in DOM and to be visible, i.e. to not have `display: none` or `visibility: hidden` CSS properties.
 - returns: <[Promise]> Promise which resolves when element specified by selector string is added to DOM.
 
 Wait for the `selector` to appear in page. If at the moment of calling
 the method the `selector` already exists, the method will return
 immediately.
 
+This method works across navigations:
+```js
+const {Browser} = new require('puppeteer');
+const browser = new Browser();
+
+browser.newPage().then(async page => {
+  let currentURL;
+  page.waitFor('img').then(() => console.log('First URL with image: ' + currentURL));
+  for (currentURL of ['https://example.com', 'https://google.com', 'https://bbc.com'])
+    await page.navigate(currentURL);
+  browser.close();
+});
+```
+
 
 ### class: Request
 

lib/FrameManager.js

@@ -173,40 +173,19 @@ class FrameManager extends EventEmitter {
   }
 
   /**
-   * @param {string} selector
    * @param {!Frame} frame
+   * @param {string} selector
+   * @param {boolean} waitForVisible
    * @return {!Promise<undefined>}
    */
-  async _waitForSelector(selector, frame) {
+  async _waitForSelector(frame, selector, waitForVisible) {
-
-    function code(selector) {
-      if (document.querySelector(selector))
-        return Promise.resolve();
-
-      let callback;
-      const result = new Promise(fulfill => callback = fulfill);
-
-      const mo = new MutationObserver((mutations, observer) => {
-        if (document.querySelector(selector)) {
-          observer.disconnect();
-          callback();
-          return;
-        }
-      });
-      mo.observe(document, {
-        childList: true,
-        subtree: true
-      });
-      return result;
-    }
-
     let contextId = undefined;
     if (!frame.isMainFrame()) {
       contextId = this._frameIdToExecutionContextId.get(frame._id);
       console.assert(contextId, 'Frame does not have default context to evaluate in!');
     }
     let { exceptionDetails } = await this._client.send('Runtime.evaluate', {
-      expression: helper.evaluationString(code, selector),
+      expression: helper.evaluationString(inPageWatchdog, selector, waitForVisible),
       contextId,
       awaitPromise: true,
       returnByValue: false,
@@ -215,6 +194,65 @@ class FrameManager extends EventEmitter {
       let message = await helper.getExceptionMessage(this._client, exceptionDetails);
       throw new Error('Evaluation failed: ' + message);
     }
+
+    /**
+     * @param {string} selector
+     * @param {boolean} waitForVisible
+     * @return {!Promise}
+     */
+    function inPageWatchdog(selector, visible) {
+      return visible ? waitForVisible(selector) : waitInDOM(selector);
+
+      /**
+       * @param {string} selector
+       * @return {!Promise<!Element>}
+       */
+      function waitInDOM(selector) {
+        let node = document.querySelector(selector);
+        if (node)
+          return Promise.resolve(node);
+
+        let fulfill;
+        const result = new Promise(x => fulfill = x);
+        const observer = new MutationObserver(mutations => {
+          const node = document.querySelector(selector);
+          if (node) {
+            observer.disconnect();
+            fulfill(node);
+          }
+        });
+        observer.observe(document, {
+          childList: true,
+          subtree: true
+        });
+        return result;
+      }
+
+      /**
+       * @param {string} selector
+       * @return {!Promise<!Element>}
+       */
+      async function waitForVisible(selector) {
+        let fulfill;
+        const result = new Promise(x => fulfill = x);
+        onRaf();
+        return result;
+
+        async function onRaf() {
+          const node = await waitInDOM(selector);
+          if (!node) {
+            fulfill(null);
+            return;
+          }
+          const style = window.getComputedStyle(node);
+          if (style.display === 'none' || style.visibility === 'hidden') {
+            requestAnimationFrame(onRaf);
+            return;
+          }
+          fulfill(node);
+        }
+      }
+    }
   }
 }
 
@@ -304,10 +342,11 @@ class Frame {
 
   /**
    * @param {string} selector
+   * @param {!Object=} options
    * @return {!Promise}
    */
-  async waitFor(selector) {
+  async waitFor(selector, options = {}) {
-    const awaitedElement = new AwaitedElement(() => this._frameManager._waitForSelector(selector, this));
+    const awaitedElement = new AwaitedElement(() => this._frameManager._waitForSelector(this, selector, !!options.visible));
 
     this._awaitedElements.add(awaitedElement);
     let cleanup = () => this._awaitedElements.delete(awaitedElement);

lib/Page.js

@@ -598,10 +598,11 @@ class Page extends EventEmitter {
 
   /**
    * @param {string} selector
+   * @param {!Object=} options
    * @return {!Promise<undefined>}
    */
-  waitFor(selector) {
+  waitFor(selector, options) {
-    return this.mainFrame().waitFor(selector);
+    return this.mainFrame().waitFor(selector, options);
   }
 
   /**

test/test.js

@@ -264,6 +264,16 @@ describe('Puppeteer', function() {
       await waitFor;
       expect(boxFound).toBe(true);
     }));
+    it('should wait for visible', SX(async function() {
+      let divFound = false;
+      let waitFor = page.waitFor('div', {visible: true}).then(() => divFound = true);
+      await page.setContent(`<div style='display: none;visibility: hidden'></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 waitFor).toBe(true);
+    }));
   });
 
   describe('Page.Events.Console', function() {

utils/doclint/JSBuilder.js

@@ -66,8 +66,16 @@ class JSOutline {
       walker.walk(node.value.body);
     }
     const args = [];
-    for (let param of node.value.params)
+    for (let param of node.value.params) {
-      args.push(new Documentation.Argument(this._extractText(param)));
+      if (param.type === 'AssignmentPattern')
+        args.push(new Documentation.Argument(param.left.name));
+      else if (param.type === 'RestElement')
+        args.push(new Documentation.Argument('...' + param.argument.name));
+      else if (param.type === 'Identifier')
+        args.push(new Documentation.Argument(param.name));
+      else
+        this.errors.push('JS Parsing issue: cannot support parameter of type ' + param.type + ' in method ' + methodName);
+    }
     let method = Documentation.Member.createMethod(methodName, args, hasReturn, node.value.async);
     this._currentClassMembers.push(method);
     return ESTreeWalker.SkipSubtree;

utils/doclint/test/04-bad-arguments/doc.md

@@ -3,5 +3,5 @@
 - `arg1` <[string]>
 - `arg2` <[string]>
 
-#### foo.test(fileNames)
+#### foo.test(...files)
-- `filePaths` <[Array]>
+- `...filePaths` <[string]>

utils/doclint/test/04-bad-arguments/foo.js

@@ -1,7 +1,7 @@
 class Foo {
-  constructor(arg1, arg3) {
+  constructor(arg1, arg3 = {}) {
   }
 
-  test(filePaths) {
+  test(...filePaths) {
   }
 }

utils/doclint/test/golden/04-bad-arguments.txt

@@ -1,4 +1,4 @@
 [MarkDown] Method Foo.constructor() fails to describe its parameters:
 - Argument not found: arg3
 - Non-existing argument found: arg2
-[MarkDown] Heading arguments for "foo.test(fileNames)" do not match described ones, i.e. "fileNames" != "filePaths"
+[MarkDown] Heading arguments for "foo.test(...files)" do not match described ones, i.e. "...files" != "...filePaths"