fix(visibility): unify visibilty checks (#1998)

This applies a common definition of visibility to clicks and waitfors:
- non-empty bounding box - implies non-empty content and no display:none;
- no visibility:hidden.

Author: dgozman

docs/api.md

@@ -947,7 +947,7 @@ Shortcut for [page.mainFrame().addStyleTag(options)](#frameaddstyletagoptions).
 - `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]>
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -971,7 +971,7 @@ Shortcut for [page.mainFrame().check(selector[, options])](#framecheckselector-o
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -1024,7 +1024,7 @@ Browser-specific Coverage implementation, only available for Chromium atm. See [
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -1333,7 +1333,7 @@ Shortcut for [page.mainFrame().goto(url[, options])](#framegotourl-options)
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -1658,7 +1658,7 @@ Shortcut for [page.mainFrame().type(selector, text[, options])](#frametypeselect
 - `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]>
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -1819,6 +1819,8 @@ return finalResponse.ok();
 
 Wait for the `selector` to satisfy `waitFor` option (either appear/disappear from dom, or become visible/hidden). If at the moment of calling the method `selector` already satisfies the condition, the method will return immediately. If the selector doesn't satisfy the condition for the `timeout` milliseconds, the function will throw.
 
+Element is considered `visible` when it has non-empty bounding box (for example, it has some content and no `display:none`) and no `visibility:hidden`. Element is considired `hidden` when it is not `visible` as defined above.
+
 This method works across navigations:
 ```js
 const { chromium } = require('playwright');  // Or 'firefox' or 'webkit'.
@@ -2002,7 +2004,7 @@ Adds a `<link rel="stylesheet">` tag into the page with the desired url or a `<s
 - `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]>
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2027,7 +2029,7 @@ If there's no element matching `selector`, the method throws an error.
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2053,7 +2055,7 @@ Gets the full HTML contents of the frame, including the doctype.
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2226,7 +2228,7 @@ console.log(frame === contentFrame);  // -> true
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2349,7 +2351,7 @@ await frame.type('#mytextarea', 'World', {delay: 100}); // Types slower, like a
 - `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]>
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2593,7 +2595,7 @@ This method returns the bounding box of the element (relative to the main frame)
 #### elementHandle.check([options])
 - `options` <[Object]>
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2613,7 +2615,7 @@ If element is not already checked, it scrolls it into view if needed, and then u
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2636,7 +2638,7 @@ If the element is detached from DOM, the method throws an error.
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2709,7 +2711,7 @@ Returns element attribute value.
     - 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.
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.
@@ -2848,7 +2850,7 @@ await elementHandle.press('Enter');
 #### elementHandle.uncheck([options])
 - `options` <[Object]>
   - `force` <[boolean]> Whether to bypass the actionability checks. By default actions wait until the element is:
-    - displayed (for example, no `display:none`),
+    - displayed (for example, not empty, no `display:none`, no `visibility:hidden`),
     - is not moving (for example, waits until css transition finishes),
     - receives pointer events at the action point (for example, waits until element becomes non-obscured by other elements).
     Even if the action is forced, it will wait for the element matching selector to be in DOM. Defaults to `false`.

docs/core-concepts.md

@@ -183,10 +183,11 @@ const sectionText = await page.$eval('*css=section >> text=Selectors', e => e.te
 
 Actions like `click` and `fill` auto-wait for the element to be visible and actionable. For example, click will:
 - wait for element with given selector to be in DOM
-- wait for it to become displayed, i.e. not `display:none`,
+- wait for it to become displayed, i.e. not empty, no `display:none`, no `visibility:hidden`
 - wait for it to stop moving, for example, until css transition finishes
 - scroll the element into view
 - wait for it to receive pointer events at the action point, for example, waits until element becomes non-obscured by other elements
+- retry if the element is detached during any of the above checks
 
 
 ```js

docs/input.md

@@ -115,10 +115,11 @@ await page.click('button#submit');
 Performs a simple human click. Under the hood, this and other pointer-related methods:
 
 - wait for element with given selector to be in DOM
-- wait for it to become displayed, i.e. not `display:none`,
+- wait for it to become displayed, i.e. not empty, no `display:none`, no `visibility:hidden`
 - wait for it to stop moving, for example, until css transition finishes
 - scroll the element into view
 - wait for it to receive pointer events at the action point, for example, waits until element becomes non-obscured by other elements
+- retry if the element is detached during any of the above checks
 
 #### Variations
 

src/injected/injected.ts

@@ -25,13 +25,14 @@ export type InjectedResult<T = undefined> =
 
 export class Injected {
   isVisible(element: Element): boolean {
+    // Note: this logic should be similar to waitForDisplayedAtStablePosition() to avoid surprises.
     if (!element.ownerDocument || !element.ownerDocument.defaultView)
       return true;
     const style = element.ownerDocument.defaultView.getComputedStyle(element);
     if (!style || style.visibility === 'hidden')
       return false;
     const rect = element.getBoundingClientRect();
-    return !!(rect.top || rect.bottom || rect.width || rect.height);
+    return rect.width > 0 && rect.height > 0;
   }
 
   private _pollMutation<T>(predicate: Predicate<T>, timeout: number): Promise<T | undefined> {
@@ -311,9 +312,12 @@ export class Injected {
         return false;
       if (!node.isConnected)
         return 'notconnected';
+      // Note: this logic should be similar to isVisible() to avoid surprises.
       const clientRect = element.getBoundingClientRect();
       const rect = { x: clientRect.top, y: clientRect.left, width: clientRect.width, height: clientRect.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;
+      let isDisplayedAndStable = lastRect && rect.x === lastRect.x && rect.y === lastRect.y && rect.width === lastRect.width && rect.height === lastRect.height && rect.width > 0 && rect.height > 0;
+      const style = element.ownerDocument && element.ownerDocument.defaultView ? element.ownerDocument.defaultView.getComputedStyle(element) : undefined;
+      isDisplayedAndStable = isDisplayedAndStable && (!!style && style.visibility !== 'hidden');
       lastRect = rect;
       return !!isDisplayedAndStable;
     });

test/click.spec.js

@@ -146,7 +146,7 @@ describe('Page.click', function() {
     expect(error.message).toBe('Node is either not visible or not an HTMLElement');
     expect(await page.evaluate(() => result)).toBe('Was not clicked');
   });
-  it('should waitFor visible', async({page, server}) => {
+  it('should waitFor display:none to be gone', async({page, server}) => {
     let done = false;
     await page.goto(server.PREFIX + '/input/button.html');
     await page.$eval('button', b => b.style.display = 'none');
@@ -155,18 +155,41 @@ describe('Page.click', function() {
       // Do enough double rafs to check for possible races.
       await page.evaluate(() => new Promise(f => requestAnimationFrame(() => requestAnimationFrame(f))));
     }
+    expect(await page.evaluate(() => result)).toBe('Was not clicked');
     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}) => {
+  it('should waitFor visibility:hidden to be gone', async({page, server}) => {
+    let done = false;
+    await page.goto(server.PREFIX + '/input/button.html');
+    await page.$eval('button', b => b.style.visibility = 'hidden');
+    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(await page.evaluate(() => result)).toBe('Was not clicked');
+    expect(done).toBe(false);
+    await page.$eval('button', b => b.style.visibility = 'visible');
+    await clicked;
+    expect(done).toBe(true);
+    expect(await page.evaluate(() => result)).toBe('Clicked');
+  });
+  it('should timeout waiting for display:none to be gone', 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 exceeded');
   });
+  it('should timeout waiting for visbility:hidden to be gone', async({page, server}) => {
+    await page.goto(server.PREFIX + '/input/button.html');
+    await page.$eval('button', b => b.style.visibility = 'hidden');
+    const error = await page.click('button', { timeout: 100 }).catch(e => e);
+    expect(error.message).toContain('timeout exceeded');
+  });
   it('should waitFor visible when parent is hidden', async({page, server}) => {
     let done = false;
     await page.goto(server.PREFIX + '/input/button.html');