fix(click): wait for element to be displayed before scrolling into view (#1182)

Otherwise, we may get an error during scrollIntoViewIfNeeded protocol call.

docs/api.md

@@ -824,7 +824,7 @@ To disable authentication, pass `null`.
 #### page.check(selector, [options])
 - `selector` <[string]> A selector to search for checkbox or radio button to check. If there are multiple elements satisfying the selector, the first will be checked.
 - `options` <[Object]>
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully checked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -843,7 +843,7 @@ Shortcut for [page.mainFrame().check(selector[, options])](#framecheckselector-o
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the click, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully clicked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -899,7 +899,7 @@ Browser-specific Coverage implementation, only available for Chromium atm. See [
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the double click, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully double clicked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -1155,7 +1155,7 @@ Shortcut for [page.mainFrame().goto(url, options)](#framegotourl-options)
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the hover, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully hovered. Promise gets rejected if there's no element matching `selector`.
 
@@ -1425,7 +1425,7 @@ Shortcut for [page.mainFrame().title()](#frametitle).
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the triple click, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully triple clicked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -1461,7 +1461,7 @@ Shortcut for [page.mainFrame().type(selector, text[, options])](#frametypeselect
 #### page.uncheck(selector, [options])
 - `selector` <[string]> A selector to search for uncheckbox to check. If there are multiple elements satisfying the selector, the first will be checked.
 - `options` <[Object]>
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully unchecked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -1837,7 +1837,7 @@ Adds a `<link rel="stylesheet">` tag into the page with the desired url or a `<s
 #### frame.check(selector, [options])
 - `selector` <[string]> A selector to search for checkbox to check. If there are multiple elements satisfying the selector, the first will be checked.
 - `options` <[Object]>
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully checked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -1857,7 +1857,7 @@ If there's no element matching `selector`, the method throws an error.
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the click, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully clicked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -1887,7 +1887,7 @@ Gets the full HTML contents of the frame, including the doctype.
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the double click, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully double clicked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -2022,7 +2022,7 @@ console.log(frame === contentFrame);  // -> true
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the hover, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully hovered. Promise gets rejected if there's no element matching `selector`.
 
@@ -2097,7 +2097,7 @@ frame.select('select#colors', { value: 'blue' }, { index: 2 }, 'red');
     - x <[number]>
     - y <[number]>
   - `modifiers` <[Array]<"Alt"|"Control"|"Meta"|"Shift">> Modifier keys to press. Ensures that only these modifiers are pressed during the triple click, and then restores current modifiers back. If not specified, currently pressed modifiers are used.
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully triple clicked. The Promise will be rejected if there is no element matching `selector`.
 
@@ -2129,7 +2129,7 @@ await frame.type('#mytextarea', 'World', {delay: 100}); // Types slower, like a
 #### frame.uncheck(selector, [options])
 - `selector` <[string]> A selector to search for uncheckbox to check. If there are multiple elements satisfying the selector, the first will be checked.
 - `options` <[Object]>
-  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom, stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
+  - `waitFor` <[boolean]> Whether to wait for the element to be present in the dom and displayed (for example, no `display:none`), stop moving (for example, wait until css transition finishes) and potentially receive pointer events at the click point (for example, wait until element becomes non-obscured by other elements). Defaults to `true`.
   - `timeout` <[number]> Maximum time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout. The default value can be changed by using the [browserContext.setDefaultTimeout(timeout)](#browsercontextsetdefaulttimeouttimeout) or [page.setDefaultTimeout(timeout)](#pagesetdefaulttimeouttimeout) methods.
 - returns: <[Promise]> Promise which resolves when the element matching `selector` is successfully unchecked. The Promise will be rejected if there is no element matching `selector`.
 

src/dom.ts

@@ -242,7 +242,7 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     if (!helper.isBoolean(waitFor))
       throw new Error('waitFor option should be a boolean, got "' + (typeof waitFor) + '"');
     if (waitFor)
-      await this._waitForStablePosition(options);
+      await this._waitForDisplayedAtStablePosition(options);
     const offset = options ? options.offset : undefined;
     await this._scrollRectIntoViewIfNeeded(offset ? { x: offset.x, y: offset.y, width: 0, height: 0 } : undefined);
     const point = offset ? await this._offsetPoint(offset) : await this._clickablePoint();
@@ -389,11 +389,11 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     return result;
   }
 
-  async _waitForStablePosition(options: types.TimeoutOptions = {}): Promise<void> {
+  async _waitForDisplayedAtStablePosition(options: types.TimeoutOptions = {}): Promise<void> {
     const stablePromise = this._evaluateInUtility((injected, node, timeout) => {
-      return injected.waitForStablePosition(node, timeout);
+      return injected.waitForDisplayedAtStablePosition(node, timeout);
     }, options.timeout || 0);
-    await helper.waitWithTimeout(stablePromise, 'element to stop moving', options.timeout || 0);
+    await helper.waitWithTimeout(stablePromise, 'element to be displayed and not moving', options.timeout || 0);
   }
 
   async _waitForHitTargetAt(point: types.Point, options: types.TimeoutOptions = {}): Promise<void> {

src/injected/injected.ts

@@ -360,7 +360,7 @@ class Injected {
     throw new Error('Not a checkbox');
   }
 
-  waitForStablePosition(node: Node, timeout: number) {
+  waitForDisplayedAtStablePosition(node: Node, timeout: number) {
     if (!node.isConnected)
       throw new Error('Element is not attached to the DOM');
     const element = node.nodeType === Node.ELEMENT_NODE ? (node as Element) : node.parentElement;
@@ -377,9 +377,9 @@ class Injected {
         return false;
       const clientRect = element.getBoundingClientRect();
       const rect = { x: clientRect.top, y: clientRect.left, width: clientRect.width, height: clientRect.height };
-      const isStable = lastRect && rect.x === lastRect.x && rect.y === lastRect.y && rect.width === lastRect.width && rect.height === lastRect.height;
+      const isDisplayedAndStable = lastRect && rect.x === lastRect.x && rect.y === lastRect.y && rect.width === lastRect.width && rect.height === lastRect.height && rect.width > 0 && rect.height > 0;
       lastRect = rect;
-      return isStable;
+      return isDisplayedAndStable;
     });
   }
 

test/click.spec.js

@@ -163,14 +163,37 @@ module.exports.describe = function({testRunner, expect, playwright, FFOX, CHROMI
       await page.goto(server.PREFIX + '/input/button.html');
       await page.$eval('button', b => b.style.display = 'none');
       const clicked = page.click('button', { timeout: 0 }).then(() => done = true);
-      for (let i = 0; i < 5; i++)
-        await page.evaluate('1'); // Do a round trip.
+      for (let i = 0; i < 10; i++) {
+        // Do enough double rafs to check for possible races.
+        await page.evaluate(() => new Promise(f => requestAnimationFrame(() => requestAnimationFrame(f))));
+      }
       expect(done).toBe(false);
       await page.$eval('button', b => b.style.display = 'block');
       await clicked;
       expect(done).toBe(true);
       expect(await page.evaluate(() => result)).toBe('Clicked');
     });
+    it('should timeout waiting for visible', async({page, server}) => {
+      await page.goto(server.PREFIX + '/input/button.html');
+      await page.$eval('button', b => b.style.display = 'none');
+      const error = await page.click('button', { timeout: 100 }).catch(e => e);
+      expect(error.message).toContain('timeout 100ms exceeded');
+    });
+    it('should waitFor visible when parent is hidden', async({page, server}) => {
+      let done = false;
+      await page.goto(server.PREFIX + '/input/button.html');
+      await page.$eval('button', b => b.parentElement.style.display = 'none');
+      const clicked = page.click('button', { timeout: 0 }).then(() => done = true);
+      for (let i = 0; i < 10; i++) {
+        // Do enough double rafs to check for possible races.
+        await page.evaluate(() => new Promise(f => requestAnimationFrame(() => requestAnimationFrame(f))));
+      }
+      expect(done).toBe(false);
+      await page.$eval('button', b => b.parentElement.style.display = 'block');
+      await clicked;
+      expect(done).toBe(true);
+      expect(await page.evaluate(() => result)).toBe('Clicked');
+    });
 
     it('should click wrapped links', async({page, server}) => {
       await page.goto(server.PREFIX + '/wrappedlink.html');

test/elementhandle.spec.js

@@ -264,20 +264,20 @@ module.exports.describe = function({testRunner, expect, FFOX, CHROMIUM, WEBKIT})
       await page.goto(server.PREFIX + '/input/button.html');
       const button = await page.$('button');
       await page.evaluate(button => button.style.display = 'none', button);
-      const error = await button.click().catch(err => err);
+      const error = await button.click({ waitFor: false }).catch(err => err);
       expect(error.message).toBe('Node is either not visible or not an HTMLElement');
     });
     it('should throw for recursively hidden nodes', async({page, server}) => {
       await page.goto(server.PREFIX + '/input/button.html');
       const button = await page.$('button');
       await page.evaluate(button => button.parentElement.style.display = 'none', button);
-      const error = await button.click().catch(err => err);
+      const error = await button.click({ waitFor: false }).catch(err => err);
       expect(error.message).toBe('Node is either not visible or not an HTMLElement');
     });
     it('should throw for <br> elements', async({page, server}) => {
       await page.setContent('hello<br>goodbye');
       const br = await page.$('br');
-      const error = await br.click().catch(err => err);
+      const error = await br.click({ waitFor: false }).catch(err => err);
       expect(error.message).toBe('Node is either not visible or not an HTMLElement');
     });
   });