feat: make scrollIntoView work with zero-sized elements (#13702)

We skip waiting for "visible" state that enforces non-zero size.
Other invisible conditions like "display:none" fail during the
actual "scrolling" step and will retry.

docs/src/actionability.md

@@ -24,7 +24,7 @@ Here is the complete list of actionability checks performed for each action:
 | tap | Yes | Yes | Yes | Yes | Yes | - |
 | uncheck | Yes | Yes | Yes | Yes | Yes | - |
 | hover | Yes | Yes | Yes | Yes | - | - |
-| scrollIntoViewIfNeeded | Yes | Yes | Yes | - | - | - |
+| scrollIntoViewIfNeeded | Yes | - | Yes | - | - | - |
 | screenshot | Yes | Yes | Yes | - | - | - |
 | fill | Yes | Yes | - | - | Yes | Yes |
 | selectText | Yes | Yes | - | - | - | - |

packages/playwright-core/src/server/dom.ts

@@ -20,7 +20,7 @@ import type * as channels from '../protocol/channels';
 import { isSessionClosedError } from './protocolError';
 import type { ScreenshotOptions } from './screenshotter';
 import type * as frames from './frames';
-import type { InjectedScript, InjectedScriptPoll, LogEntry, HitTargetInterceptionResult } from './injected/injectedScript';
+import type { InjectedScript, InjectedScriptPoll, LogEntry, HitTargetInterceptionResult, ElementState } from './injected/injectedScript';
 import type { CallMetadata } from './instrumentation';
 import * as js from './javascript';
 import type { Page } from './page';
@@ -260,14 +260,22 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     return await this._page._delegate.scrollRectIntoViewIfNeeded(this, rect);
   }
 
-  async _waitAndScrollIntoViewIfNeeded(progress: Progress): Promise<void> {
+  async _waitAndScrollIntoViewIfNeeded(progress: Progress, waitForVisible: boolean): Promise<void> {
+    const timeouts = [0, 50, 100, 250];
     while (progress.isRunning()) {
-      assertDone(throwRetargetableDOMError(await this._waitForDisplayedAtStablePosition(progress, false /* force */, false /* waitForEnabled */)));
+      assertDone(throwRetargetableDOMError(await this._waitForElementStates(progress, waitForVisible ? ['visible', 'stable'] : ['stable'], false /* force */)));
-
       progress.throwIfAborted();  // Avoid action that has side-effects.
       const result = throwRetargetableDOMError(await this._scrollRectIntoViewIfNeeded());
-      if (result === 'error:notvisible')
+      if (result === 'error:notvisible') {
+        if (!waitForVisible) {
+          // Wait for a timeout to avoid retrying too often when not waiting for visible.
+          // If we wait for visible, this should be covered by _waitForElementStates instead.
+          const timeout = timeouts.shift() ?? 500;
+          progress.log(`  element is not displayed, retrying in ${timeout}ms`);
+          await new Promise(f => setTimeout(f, timeout));
+        }
         continue;
+      }
       assertDone(result);
       return;
     }
@@ -276,7 +284,7 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
   async scrollIntoViewIfNeeded(metadata: CallMetadata, options: types.TimeoutOptions = {}) {
     const controller = new ProgressController(metadata, this);
     return controller.run(
-        progress => this._waitAndScrollIntoViewIfNeeded(progress),
+        progress => this._waitAndScrollIntoViewIfNeeded(progress, false /* waitForVisible */),
         this._page._timeoutSettings.timeout(options));
   }
 
@@ -395,7 +403,7 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     const { force = false, position } = options;
     if ((options as any).__testHookBeforeStable)
       await (options as any).__testHookBeforeStable();
-    const result = await this._waitForDisplayedAtStablePosition(progress, force, waitForEnabled);
+    const result = await this._waitForElementStates(progress, waitForEnabled ? ['visible', 'enabled', 'stable'] : ['visible', 'stable'], force);
     if (result !== 'done')
       return result;
     if ((options as any).__testHookAfterStable)
@@ -845,21 +853,15 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     return this;
   }
 
-  async _waitForDisplayedAtStablePosition(progress: Progress, force: boolean, waitForEnabled: boolean): Promise<'error:notconnected' | 'done'> {
+  async _waitForElementStates(progress: Progress, states: ElementState[], force: boolean): Promise<'error:notconnected' | 'done'> {
-    if (waitForEnabled)
+    const title = joinWithAnd(states);
-      progress.log(`  waiting for element to be visible, enabled and stable`);
+    progress.log(`  waiting for element to be ${title}`);
-    else
+    const result = await this.evaluatePoll(progress, ([injected, node, { states, force }]) => {
-      progress.log(`  waiting for element to be visible and stable`);
+      return injected.waitForElementStatesAndPerformAction(node, states, force, () => 'done' as const);
-    const result = await this.evaluatePoll(progress, ([injected, node, { waitForEnabled, force }]) => {
+    }, { states, force });
-      return injected.waitForElementStatesAndPerformAction(node,
-          waitForEnabled ? ['visible', 'stable', 'enabled'] : ['visible', 'stable'], force, () => 'done' as const);
-    }, { waitForEnabled, force });
     if (result === 'error:notconnected')
       return result;
-    if (waitForEnabled)
+    progress.log(`  element is ${title}`);
-      progress.log('  element is visible, enabled and stable');
-    else
-      progress.log('  element is visible and stable');
     return result;
   }
 
@@ -1023,4 +1025,10 @@ export function waitForSelectorTask(selector: SelectorInfo, state: 'attached' |
   }, { parsed: selector.parsed, strict: selector.strict, state, omitReturnValue, root });
 }
 
+function joinWithAnd(strings: string[]): string {
+  if (strings.length < 1)
+    return strings.join(', ');
+  return strings.slice(0, strings.length - 1).join(', ') + ' and ' + strings[strings.length - 1];
+}
+
 export const kUnableToAdoptErrorMessage = 'Unable to adopt element handle from a different document';

packages/playwright-core/src/server/screenshotter.ts

@@ -119,7 +119,7 @@ export class Screenshotter {
       await this._preparePageForScreenshot(progress, options.caret !== 'initial', options.animations === 'disabled', options.fonts === 'ready');
       progress.throwIfAborted(); // Do not do extra work.
 
-      await handle._waitAndScrollIntoViewIfNeeded(progress);
+      await handle._waitAndScrollIntoViewIfNeeded(progress, true /* waitForVisible */);
 
       progress.throwIfAborted(); // Do not do extra work.
       const boundingBox = await handle.boundingBox();

tests/page/elementhandle-scroll-into-view.spec.ts

@@ -54,6 +54,7 @@ async function testWaiting(page, 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');
@@ -64,14 +65,16 @@ it('should wait for display:contents to become visible', async ({ page, server }
   await testWaiting(page, div => div.style.display = 'block');
 });
 
-it('should wait for visibility:hidden to become visible', async ({ page, server }) => {
+it('should work for visibility:hidden element', async ({ page }) => {
   await page.setContent('<div style="visibility:hidden">Hello</div>');
-  await testWaiting(page, div => div.style.visibility = 'visible');
+  const div = await page.$('div');
+  await div.scrollIntoViewIfNeeded();
 });
 
-it('should wait for zero-sized element to become visible', async ({ page, server }) => {
+it('should work for zero-sized element', async ({ page }) => {
   await page.setContent('<div style="height:0">Hello</div>');
-  await testWaiting(page, div => div.style.height = '100px');
+  const div = await page.$('div');
+  await div.scrollIntoViewIfNeeded();
 });
 
 it('should wait for nested display:none to become visible', async ({ page, server }) => {
@@ -99,5 +102,5 @@ 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({ timeout: 3000 }).catch(e => e);
-  expect(error.message).toContain('element is not visible');
+  expect(error.message).toContain('element is not displayed, retrying in 100ms');
 });

tests/page/locator-misc-2.spec.ts

@@ -42,6 +42,34 @@ it('should scroll into view', async ({ page, server, isAndroid }) => {
   }
 });
 
+it('should scroll zero-sized element into view', async ({ page, isAndroid }) => {
+  it.fixme(isAndroid);
+
+  await page.setContent(`
+    <style>html,body { margin: 0; padding: 0; }</style>
+    <div style="height: 2000px; text-align: center; border: 10px solid blue;">
+      <h1>SCROLL DOWN</h1>
+    </div>
+    <div id=lazyload style="font-size:75px; background-color: green;"></div>
+    <script>
+      const lazyLoadElement = document.querySelector('#lazyload');
+      const observer = new IntersectionObserver((entries) => {
+        if (entries.some(entry => entry.isIntersecting)) {
+          lazyLoadElement.textContent = 'LAZY LOADED CONTENT';
+          lazyLoadElement.style.height = '20px';
+          observer.disconnect();
+        }
+      });
+      observer.observe(lazyLoadElement);
+    </script>
+  `);
+  expect(await page.locator('#lazyload').boundingBox()).toEqual({ x: 0, y: 2020, width: 1280, height: 0 });
+  await page.locator('#lazyload').scrollIntoViewIfNeeded();
+  await page.evaluate(() => new Promise(requestAnimationFrame));
+  expect(await page.locator('#lazyload').textContent()).toBe('LAZY LOADED CONTENT');
+  expect(await page.locator('#lazyload').boundingBox()).toEqual({ x: 0, y: 720, width: 1280, height: 20 });
+});
+
 it('should select textarea', async ({ page, server, browserName }) => {
   await page.goto(server.PREFIX + '/input/textarea.html');
   const textarea = page.locator('textarea');