fix(dom): make selectText and scrollIntoViewIfNeeded wait for visible (#2628)

All other methods wait for the element to be visible, so we should make
them behave similarly.

docs/actionability.md

@@ -10,9 +10,8 @@ Some actions like `page.click()` support `{force: true}` option that disable non
 | ------ | ------- |
 | `check()`<br>`click()`<br>`dblclick()`<br>`hover()`<br>`uncheck()` | [Visible]<br>[Stable]<br>[Enabled]<br>[Receiving Events]<br>[Attached]† |
 | `fill()` | [Visible]<br>[Enabled]<br>[Editable]<br>[Attached]† |
-| `focus()`<br>`press()`<br>`setInputFiles()`<br>`selectOption()`<br>`type()` | [Attached]† |
-| `selectText()` | [Visible] |
-| `dispatchEvent()`<br>`scrollIntoViewIfNeeded()` | -- |
+| `dispatchEvent()`<br>`focus()`<br>`press()`<br>`setInputFiles()`<br>`selectOption()`<br>`type()` | [Attached]† |
+| `selectText()`<br>`scrollIntoViewIfNeeded()` | [Visible] |
 | `getAttribute()`<br>`innerText()`<br>`innerHTML()`<br>`textContent()` | [Attached]† |
 
 † [Attached] check is only performed during selector-based actions.
@@ -41,7 +40,7 @@ Element is considered receiving pointer events when it is the hit target of the
 
 Element is considered attached when it is [connected](https://developer.mozilla.org/en-US/docs/Web/API/Node/isConnected) to a Document or a ShadowRoot.
 
-Attached check is performed during a selector-based action, like `page.click(selector, options)` as opposite to `elementHandle.click(options)`.
+Attached check is performed during a selector-based action, like `page.click(selector, options)` as opposite to `elementHandle.click(options)`. First, Playwright waits for an element matching `selector` to be attached to the DOM, and then checks that element is still attached before performing the action.
 
 For example, consider a scenario where Playwright will click `Sign Up` button regardless of when the `page.click()` call was made:
 - page is checking that user name is unique and `Sign Up` button is disabled;

docs/api.md

@@ -2627,9 +2627,9 @@ ElementHandle instances can be used as an argument in [`page.$eval()`](#pageeval
 - [elementHandle.ownerFrame()](#elementhandleownerframe)
 - [elementHandle.press(key[, options])](#elementhandlepresskey-options)
 - [elementHandle.screenshot([options])](#elementhandlescreenshotoptions)
-- [elementHandle.scrollIntoViewIfNeeded()](#elementhandlescrollintoviewifneeded)
+- [elementHandle.scrollIntoViewIfNeeded([options])](#elementhandlescrollintoviewifneededoptions)
 - [elementHandle.selectOption(values[, options])](#elementhandleselectoptionvalues-options)
-- [elementHandle.selectText()](#elementhandleselecttext)
+- [elementHandle.selectText([options])](#elementhandleselecttextoptions)
 - [elementHandle.setInputFiles(files[, options])](#elementhandlesetinputfilesfiles-options)
 - [elementHandle.textContent()](#elementhandletextcontent)
 - [elementHandle.toString()](#elementhandletostring)
@@ -2860,15 +2860,15 @@ Shortcuts such as `key: "Control+o"` or `key: "Control+Shift+T"` are supported a
 
 This method scrolls element into view if needed before taking a screenshot. If the element is detached from DOM, the method throws an error.
 
-#### elementHandle.scrollIntoViewIfNeeded()
-- returns: <[Promise]> Resolves after the element has been scrolled into view.
+#### elementHandle.scrollIntoViewIfNeeded([options])
+- `options` <[Object]>
+  - `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]>
 
-This method tries to scroll element into view, unless it is completely visible as defined by [IntersectionObserver](https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API)'s ```ratio```.
+This method waits for [actionability](./actionability.md) checks, then tries to scroll element into view, unless it is completely visible as defined by [IntersectionObserver](https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API)'s ```ratio```.
 
 Throws when ```elementHandle``` does not point to an element [connected](https://developer.mozilla.org/en-US/docs/Web/API/Node/isConnected) to a Document or a ShadowRoot.
 
-> **NOTE** If javascript is disabled, element is scrolled into view even when already completely visible.
-
 #### elementHandle.selectOption(values[, options])
 - `values` <null|[string]|[ElementHandle]|[Array]<[string]>|[Object]|[Array]<[ElementHandle]>|[Array]<[Object]>> Options to select. If the `<select>` has the `multiple` attribute, all matching options are selected, otherwise only the first option matching one of the passed options is selected. String values are equivalent to `{value:'string'}`. Option is considered matching if all specified properties match.
   - `value` <[string]> Matches by `option.value`.
@@ -2896,10 +2896,12 @@ handle.selectOption('red', 'green', 'blue');
 handle.selectOption({ value: 'blue' }, { index: 2 }, 'red');
 ```
 
-#### elementHandle.selectText()
-- returns: <[Promise]> Promise which resolves when the element is successfully selected.
+#### elementHandle.selectText([options])
+- `options` <[Object]>
+  - `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]>
 
-This method focuses the element and selects all its text content.
+This method waits for [actionability](./actionability.md) checks, then focuses the element and selects all its text content.
 
 #### elementHandle.setInputFiles(files[, options])
 - `files` <[string]|[Array]<[string]>|[Object]|[Array]<[Object]>>

src/dom.ts

@@ -212,11 +212,29 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     return await this._page._delegate.scrollRectIntoViewIfNeeded(this, rect);
   }
 
-  async scrollIntoViewIfNeeded() {
-    const result = await this._scrollRectIntoViewIfNeeded();
-    if (result === 'notvisible')
-      throw new Error('Element is not visible');
-    throwIfNotConnected(result);
+  async scrollIntoViewIfNeeded(options: types.TimeoutOptions = {}) {
+    return this._runAbortableTask(async progress => {
+      while (progress.isRunning()) {
+        const waited = await this._waitForVisible(progress);
+        throwIfNotConnected(waited);
+
+        progress.throwIfAborted();  // Avoid action that has side-effects.
+        const result = await this._scrollRectIntoViewIfNeeded();
+        throwIfNotConnected(result);
+        if (result === 'notvisible')
+          continue;
+        assert(result === 'done');
+        return;
+      }
+    }, this._page._timeoutSettings.timeout(options), 'scrollIntoViewIfNeeded');
+  }
+
+  private async _waitForVisible(progress: Progress): Promise<'notconnected' | 'done'> {
+    const poll = await this._evaluateHandleInUtility(([injected, node]) => {
+      return injected.waitForNodeVisible(node);
+    }, {});
+    const pollHandler = new InjectedScriptPollHandler(progress, poll);
+    return throwIfError(await pollHandler.finish());
   }
 
   private async _clickablePoint(): Promise<types.Point | 'notvisible' | 'notinviewport'> {
@@ -455,12 +473,16 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     }, 'input');
   }
 
-  async selectText(): Promise<void> {
+  async selectText(options: types.TimeoutOptions = {}): Promise<void> {
     return this._runAbortableTask(async progress => {
       progress.throwIfAborted();  // Avoid action that has side-effects.
-      const selected = throwIfError(await this._evaluateInUtility(([injected, node]) => injected.selectText(node), {}));
-      throwIfNotConnected(selected);
-    }, 0, 'selectText');
+      const poll = await this._evaluateHandleInUtility(([injected, node]) => {
+        return injected.waitForVisibleAndSelectText(node);
+      }, {});
+      const pollHandler = new InjectedScriptPollHandler(progress, poll);
+      const result = throwIfError(await pollHandler.finish());
+      throwIfNotConnected(result);
+    }, this._page._timeoutSettings.timeout(options), 'selectText');
   }
 
   async setInputFiles(files: string | types.FilePayload | string[] | types.FilePayload[], options: types.NavigatingActionWaitOptions = {}) {

src/injected/injectedScript.ts

@@ -235,7 +235,7 @@ export default class InjectedScript {
     return this.poll('raf', progress => {
       if (node.nodeType !== Node.ELEMENT_NODE)
         return { error: 'Node is not of type HTMLElement' };
-      const element = node as HTMLElement;
+      const element = node as Element;
       if (!element.isConnected)
         return { value: 'notconnected' };
       if (!this.isVisible(element)) {
@@ -282,48 +282,74 @@ export default class InjectedScript {
           progress.logRepeating('    element is readonly - waiting...');
           return false;
         }
-      } else if (!element.isContentEditable) {
+      } else if (!(element as HTMLElement).isContentEditable) {
         return { error: 'Element is not an <input>, <textarea> or [contenteditable] element.' };
       }
-      const result = this.selectText(node);
-      if (result.error)
-        return { error: result.error };
-      if (result.value === 'notconnected')
-        return { value: 'notconnected' };
+      const result = this._selectText(element);
+      if (result === 'notvisible') {
+        progress.logRepeating('    element is not visible - waiting...');
+        return false;
+      }
       return { value: 'needsinput' };  // Still need to input the value.
     });
   }
 
-  selectText(node: Node): types.InjectedScriptResult<'notconnected' | 'done'> {
-    if (node.nodeType !== Node.ELEMENT_NODE)
-      return { error: 'Node is not of type HTMLElement' };
-    if (!node.isConnected)
-      return { value: 'notconnected' };
-    const element = node as HTMLElement;
-    if (!this.isVisible(element))
-      return { error: 'Element is not visible' };
+  waitForVisibleAndSelectText(node: Node): types.InjectedScriptPoll<types.InjectedScriptResult<'notconnected' | 'done'>> {
+    return this.poll('raf', progress => {
+      if (node.nodeType !== Node.ELEMENT_NODE)
+        return { error: 'Node is not of type HTMLElement' };
+      if (!node.isConnected)
+        return { value: 'notconnected' };
+      const element = node as Element;
+      if (!this.isVisible(element)) {
+        progress.logRepeating('    element is not visible - waiting...');
+        return false;
+      }
+      const result = this._selectText(element);
+      if (result === 'notvisible') {
+        progress.logRepeating('    element is not visible - waiting...');
+        return false;
+      }
+      return { value: result };
+    });
+  }
+
+  private _selectText(element: Element): 'notvisible' | 'done' {
     if (element.nodeName.toLowerCase() === 'input') {
       const input = element as HTMLInputElement;
       input.select();
       input.focus();
-      return { value: 'done' };
+      return 'done';
     }
     if (element.nodeName.toLowerCase() === 'textarea') {
       const textarea = element as HTMLTextAreaElement;
       textarea.selectionStart = 0;
       textarea.selectionEnd = textarea.value.length;
       textarea.focus();
-      return { value: 'done' };
+      return 'done';
     }
     const range = element.ownerDocument.createRange();
     range.selectNodeContents(element);
     const selection = element.ownerDocument.defaultView!.getSelection();
     if (!selection)
-      return { error: 'Element belongs to invisible iframe.' };
+      return 'notvisible';
     selection.removeAllRanges();
     selection.addRange(range);
-    element.focus();
-    return { value: 'done' };
+    (element as HTMLElement | SVGElement).focus();
+    return 'done';
+  }
+
+  waitForNodeVisible(node: Node): types.InjectedScriptPoll<types.InjectedScriptResult<'notconnected' | 'done'>> {
+    return this.poll('raf', progress => {
+      const element = node.nodeType === Node.ELEMENT_NODE ? node as Element : node.parentElement;
+      if (!node.isConnected || !element)
+        return { value: 'notconnected' };
+      if (!this.isVisible(element)) {
+        progress.logRepeating('    element is not visible - waiting...');
+        return false;
+      }
+      return { value: 'done' };
+    });
   }
 
   focusNode(node: Node): types.InjectedScriptResult<'notconnected' | 'done'> {

src/screenshotter.ts

@@ -95,7 +95,10 @@ export class Screenshotter {
     return this._queue.postTask(async () => {
       const { viewportSize, originalViewportSize } = await this._originalViewportSize();
 
-      await handle.scrollIntoViewIfNeeded();
+      // TODO: make screenshot wait visible, migrate to progress.
+      const scrolled = await handle._scrollRectIntoViewIfNeeded();
+      if (scrolled === 'notconnected')
+        throw new Error('Element is not attached to the DOM');
       let boundingBox = await handle.boundingBox();
       assert(boundingBox, 'Node is either not visible or not an HTMLElement');
       assert(boundingBox.width !== 0, 'Node has 0 width.');
@@ -110,7 +113,9 @@ export class Screenshotter {
         });
         await this._page.setViewportSize(overridenViewportSize);
 
-        await handle.scrollIntoViewIfNeeded();
+        const scrolled = await handle._scrollRectIntoViewIfNeeded();
+        if (scrolled === 'notconnected')
+          throw new Error('Element is not attached to the DOM');
         boundingBox = await handle.boundingBox();
         assert(boundingBox, 'Node is either not visible or not an HTMLElement');
         assert(boundingBox.width !== 0, 'Node has 0 width.');

test/elementhandle.spec.js

@@ -330,33 +330,43 @@ describe('ElementHandle.scrollIntoViewIfNeeded', function() {
     const error = await div.scrollIntoViewIfNeeded().catch(e => e);
     expect(error.message).toContain('Element is not attached to the DOM');
   });
-  it('should throw for display:none element', async({page, server}) => {
+
+  async function testWaiting(page, after) {
+    const div = await page.$('div');
+    let done = false;
+    const promise = div.scrollIntoViewIfNeeded().then(() => done = true);
+    await page.evaluate(() => new Promise(f => setTimeout(f, 1000)));
+    expect(done).toBe(false);
+    await div.evaluate(after);
+    await promise;
+    expect(done).toBe(true);
+  }
+  it('should wait for display:none to become visible', async({page, server}) => {
+    await page.setContent('<div style="display:none">Hello</div>');
+    await testWaiting(page, div => div.style.display = 'block');
+  });
+  it('should wait for display:contents to become visible', async({page, server}) => {
+    await page.setContent('<div style="display:contents">Hello</div>');
+    await testWaiting(page, div => div.style.display = 'block');
+  });
+  it('should wait for visibility:hidden to become visible', async({page, server}) => {
+    await page.setContent('<div style="visibility:hidden">Hello</div>');
+    await testWaiting(page, div => div.style.visibility = 'visible');
+  });
+  it('should wait for zero-sized element to become visible', async({page, server}) => {
+    await page.setContent('<div style="height:0">Hello</div>');
+    await testWaiting(page, div => div.style.height = '100px');
+  });
+  it('should wait for nested display:none to become visible', async({page, server}) => {
+    await page.setContent('<span style="display:none"><div>Hello</div></span>');
+    await testWaiting(page, div => div.parentElement.style.display = 'block');
+  });
+
+  it('should timeout waiting for visible', async({page, server}) => {
     await page.setContent('<div style="display:none">Hello</div>');
     const div = await page.$('div');
-    const error = await div.scrollIntoViewIfNeeded().catch(e => e);
-    expect(error.message).toContain('Element is not visible');
-  });
-  it('should throw for nested display:none element', async({page, server}) => {
-    await page.setContent('<span style="display:none"><div>Hello</div></span>');
-    const div = await page.$('div');
-    const error = await div.scrollIntoViewIfNeeded().catch(e => e);
-    expect(error.message).toContain('Element is not visible');
-  });
-  it('should throw for display:contents element', async({page, server}) => {
-    await page.setContent('<div style="display:contents">Hello</div>');
-    const div = await page.$('div');
-    const error = await div.scrollIntoViewIfNeeded().catch(e => e);
-    expect(error.message).toContain('Element is not visible');
-  });
-  it('should scroll a zero-sized element', async({page, server}) => {
-    await page.setContent('<br>');
-    const br = await page.$('br');
-    await br.scrollIntoViewIfNeeded();
-  });
-  it('should scroll a visibility:hidden element', async({page, server}) => {
-    await page.setContent('<div style="visibility:hidden">Hello</div>');
-    const div = await page.$('div');
-    await div.scrollIntoViewIfNeeded();
+    const error = await div.scrollIntoViewIfNeeded({ timeout: 3000 }).catch(e => e);
+    expect(error.message).toContain('element is not visible');
   });
 });
 
@@ -407,6 +417,25 @@ describe('ElementHandle.selectText', function() {
     await div.selectText();
     expect(await page.evaluate(() => window.getSelection().toString())).toBe('Plain div');
   });
+  it('should timeout waiting for invisible element', async({page, server}) => {
+    await page.goto(server.PREFIX + '/input/textarea.html');
+    const textarea = await page.$('textarea');
+    await textarea.evaluate(e => e.style.display = 'none');
+    const error = await textarea.selectText({ timeout: 3000 }).catch(e => e);
+    expect(error.message).toContain('element is not visible');
+  });
+  it('should wait for visible', async({page, server}) => {
+    await page.goto(server.PREFIX + '/input/textarea.html');
+    const textarea = await page.$('textarea');
+    await textarea.evaluate(textarea => textarea.value = 'some value');
+    await textarea.evaluate(e => e.style.display = 'none');
+    let done = false;
+    const promise = textarea.selectText({ timeout: 3000 }).then(() => done = true);
+    await page.evaluate(() => new Promise(f => setTimeout(f, 1000)));
+    expect(done).toBe(false);
+    await textarea.evaluate(e => e.style.display = 'block');
+    await promise;
+  });
 });