feat(locator handler): address api review feedback (#30554)

- Remove `handler` argument from `removeLocatorHandler`. - Rename `allowStayingVisible` into `noWaitAfter`. - Improve logging related to locator handler. - Remove experimental badges.
2024-09-11 20:37:54 +03:00 · 2024-04-25 14:00:02 -07:00 · 2024-04-25 14:00:02 -07:00 · 9bd2aea130
commit 9bd2aea130
parent dc0665210f
9 changed files with 72 additions and 76 deletions
--- a/docs/src/api/class-page.md
+++ b/docs/src/api/class-page.md
@ -3144,10 +3144,6 @@ return value resolves to `[]`.
 ## async method: Page.addLocatorHandler
 * since: v1.42

-:::warning[Experimental]
-This method is experimental and its behavior may change in the upcoming releases.
-:::
-
 When testing a web page, sometimes unexpected overlays like a "Sign up" dialog appear and block actions you want to automate, e.g. clicking a button. These overlays don't always show up in the same way or at the same time, making them tricky to handle in automated tests.

 This method lets you set up a special function, called a handler, that activates when it detects that overlay is visible. The handler's job is to remove the overlay, allowing your test to continue as if the overlay wasn't there.
@ -3155,7 +3151,7 @@ This method lets you set up a special function, called a handler, that activates
 Things to keep in mind:
 * When an overlay is shown predictably, we recommend explicitly waiting for it in your test and dismissing it as a part of your normal test flow, instead of using [`method: Page.addLocatorHandler`].
 * Playwright checks for the overlay every time before executing or retrying an action that requires an [actionability check](../actionability.md), or before performing an auto-waiting assertion check. When overlay is visible, Playwright calls the handler first, and then proceeds with the action/assertion. Note that the handler is only called when you perform an action/assertion - if the overlay becomes visible but you don't perform any actions, the handler will not be triggered.
-* After executing the handler, Playwright will ensure that overlay that triggered the handler is not visible anymore. You can opt-out of this behavior with [`option: allowStayingVisible`].
+* After executing the handler, Playwright will ensure that overlay that triggered the handler is not visible anymore. You can opt-out of this behavior with [`option: noWaitAfter`].
 * The execution time of the handler counts towards the timeout of the action/assertion that executed the handler. If your handler takes too long, it might cause timeouts.
 * You can register multiple handlers. However, only a single handler will be running at a time. Make sure the actions within a handler don't depend on another handler.

@ -3285,13 +3281,13 @@ await page.GotoAsync("https://example.com");
 await page.GetByRole("button", new() { Name = "Start here" }).ClickAsync();
 ```

-An example with a custom callback on every actionability check. It uses a `<body>` locator that is always visible, so the handler is called before every actionability check. It is important to specify [`option: allowStayingVisible`], because the handler does not hide the `<body>` element.
+An example with a custom callback on every actionability check. It uses a `<body>` locator that is always visible, so the handler is called before every actionability check. It is important to specify [`option: noWaitAfter`], because the handler does not hide the `<body>` element.

 ```js
 // Setup the handler.
 await page.addLocatorHandler(page.locator('body'), async () => {
  await page.evaluate(() => window.removeObstructionsForTestIfNeeded());
-}, { allowStayingVisible: true });
+}, { noWaitAfter: true });

 // Write the test as usual.
 await page.goto('https://example.com');
@ -3302,7 +3298,7 @@ await page.getByRole('button', { name: 'Start here' }).click();
 // Setup the handler.
 page.addLocatorHandler(page.locator("body")), () => {
  page.evaluate("window.removeObstructionsForTestIfNeeded()");
-}, new Page.AddLocatorHandlerOptions.setAllowStayingVisible(true));
+}, new Page.AddLocatorHandlerOptions.setNoWaitAfter(true));

 // Write the test as usual.
 page.goto("https://example.com");
@ -3313,7 +3309,7 @@ page.getByRole("button", Page.GetByRoleOptions().setName("Start here")).click();
 # Setup the handler.
 def handler():
  page.evaluate("window.removeObstructionsForTestIfNeeded()")
-page.add_locator_handler(page.locator("body"), handler, allow_staying_visible=True)
+page.add_locator_handler(page.locator("body"), handler, no_wait_after=True)

 # Write the test as usual.
 page.goto("https://example.com")
@ -3324,7 +3320,7 @@ page.get_by_role("button", name="Start here").click()
 # Setup the handler.
 def handler():
  await page.evaluate("window.removeObstructionsForTestIfNeeded()")
-await page.add_locator_handler(page.locator("body"), handler, allow_staying_visible=True)
+await page.add_locator_handler(page.locator("body"), handler, no_wait_after=True)

 # Write the test as usual.
 await page.goto("https://example.com")
@ -3335,7 +3331,7 @@ await page.get_by_role("button", name="Start here").click()
 // Setup the handler.
 await page.AddLocatorHandlerAsync(page.Locator("body"), async () => {
  await page.EvaluateAsync("window.removeObstructionsForTestIfNeeded()");
-}, new() { AllowStayingVisible = true });
+}, new() { NoWaitAfter = true });

 // Write the test as usual.
 await page.GotoAsync("https://example.com");
@ -3407,9 +3403,9 @@ Function that should be run once [`param: locator`] appears. This function shoul

 Specifies the maximum number of times this handler should be called. Unlimited by default.

-### option: Page.addLocatorHandler.allowStayingVisible
+### option: Page.addLocatorHandler.noWaitAfter
 * since: v1.44
- `allowStayingVisible` <[boolean]>
+- `noWaitAfter` <[boolean]>

 By default, after calling the handler Playwright will wait until the overlay becomes hidden, and only then Playwright will continue with the action/assertion that triggered the handler. This option allows to opt-out of this behavior, so that overlay can stay visible after the handler has run.

@ -3417,11 +3413,7 @@ By default, after calling the handler Playwright will wait until the overlay bec
 ## async method: Page.removeLocatorHandler
 * since: v1.44

-:::warning[Experimental]
-This method is experimental and its behavior may change in the upcoming releases.
-:::
-
-Removes locator handler added by [`method: Page.addLocatorHandler`].
+Removes all locator handlers added by [`method: Page.addLocatorHandler`] for a specific locator.

 ### param: Page.removeLocatorHandler.locator
 * since: v1.44
@ -3429,20 +3421,6 @@ Removes locator handler added by [`method: Page.addLocatorHandler`].

 Locator passed to [`method: Page.addLocatorHandler`].

-### param: Page.removeLocatorHandler.handler
-* langs: js, python
-* since: v1.44
- `handler` <[function]\([Locator]\): [Promise<any>]>
-
-Handler passed to [`method: Page.addLocatorHandler`].
-
-### param: Page.addLocatorHandler.handler
-* langs: csharp, java
-* since: v1.44
- `handler` <[function]\([Locator]\)>
-
-Handler passed to [`method: Page.addLocatorHandler`].
-

 ## async method: Page.reload
 * since: v1.8
--- a/packages/playwright-core/src/client/page.ts
+++ b/packages/playwright-core/src/client/page.ts
@ -362,12 +362,12 @@ export class Page extends ChannelOwner<channels.PageChannel> implements api.Page
    return Response.fromNullable((await this._channel.reload({ ...options, waitUntil })).response);
  }

-  async addLocatorHandler(locator: Locator, handler: (locator: Locator) => any, options: { times?: number, allowStayingVisible?: boolean } = {}): Promise<void> {
+  async addLocatorHandler(locator: Locator, handler: (locator: Locator) => any, options: { times?: number, noWaitAfter?: boolean } = {}): Promise<void> {
    if (locator._frame !== this._mainFrame)
      throw new Error(`Locator must belong to the main frame of this page`);
    if (options.times === 0)
      return;
-    const { uid } = await this._channel.registerLocatorHandler({ selector: locator._selector, allowStayingVisible: options.allowStayingVisible });
+    const { uid } = await this._channel.registerLocatorHandler({ selector: locator._selector, noWaitAfter: options.noWaitAfter });
    this._locatorHandlers.set(uid, { locator, handler, times: options.times });
  }

@ -386,11 +386,11 @@ export class Page extends ChannelOwner<channels.PageChannel> implements api.Page
    }
  }

-  async removeLocatorHandler(locator: Locator, handler: (locator: Locator) => any): Promise<void> {
+  async removeLocatorHandler(locator: Locator): Promise<void> {
    for (const [uid, data] of this._locatorHandlers) {
-      if (data.locator._equals(locator) && data.handler === handler) {
+      if (data.locator._equals(locator)) {
        this._locatorHandlers.delete(uid);
-        await this._channel.unregisterLocatorHandlerNoReply({ uid }).catch(() => {});
+        await this._channel.unregisterLocatorHandler({ uid }).catch(() => {});
      }
    }
  }
--- a/packages/playwright-core/src/protocol/validator.ts
+++ b/packages/playwright-core/src/protocol/validator.ts
@ -1046,7 +1046,7 @@ scheme.PageGoForwardResult = tObject({
 });
 scheme.PageRegisterLocatorHandlerParams = tObject({
  selector: tString,
-  allowStayingVisible: tOptional(tBoolean),
+  noWaitAfter: tOptional(tBoolean),
 });
 scheme.PageRegisterLocatorHandlerResult = tObject({
  uid: tNumber,
@ -1056,10 +1056,10 @@ scheme.PageResolveLocatorHandlerNoReplyParams = tObject({
  remove: tOptional(tBoolean),
 });
 scheme.PageResolveLocatorHandlerNoReplyResult = tOptional(tObject({}));
-scheme.PageUnregisterLocatorHandlerNoReplyParams = tObject({
+scheme.PageUnregisterLocatorHandlerParams = tObject({
  uid: tNumber,
 });
-scheme.PageUnregisterLocatorHandlerNoReplyResult = tOptional(tObject({}));
+scheme.PageUnregisterLocatorHandlerResult = tOptional(tObject({}));
 scheme.PageReloadParams = tObject({
  timeout: tOptional(tNumber),
  waitUntil: tOptional(tType('LifecycleEvent')),
--- a/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts
+++ b/packages/playwright-core/src/server/dispatchers/pageDispatcher.ts
@ -138,7 +138,7 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
  }

  async registerLocatorHandler(params: channels.PageRegisterLocatorHandlerParams, metadata: CallMetadata): Promise<channels.PageRegisterLocatorHandlerResult> {
-    const uid = this._page.registerLocatorHandler(params.selector, params.allowStayingVisible);
+    const uid = this._page.registerLocatorHandler(params.selector, params.noWaitAfter);
    return { uid };
  }

@ -146,7 +146,7 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
    this._page.resolveLocatorHandler(params.uid, params.remove);
  }

-  async unregisterLocatorHandlerNoReply(params: channels.PageUnregisterLocatorHandlerNoReplyParams, metadata: CallMetadata): Promise<void> {
+  async unregisterLocatorHandler(params: channels.PageUnregisterLocatorHandlerParams, metadata: CallMetadata): Promise<void> {
    this._page.unregisterLocatorHandler(params.uid);
  }

--- a/packages/playwright-core/src/server/page.ts
+++ b/packages/playwright-core/src/server/page.ts
@ -168,7 +168,7 @@ export class Page extends SdkObject {
  _video: Artifact | null = null;
  _opener: Page | undefined;
  private _isServerSideOnly = false;
-  private _locatorHandlers = new Map<number, { selector: string, allowStayingVisible?: boolean, resolved?: ManualPromise<void> }>();
+  private _locatorHandlers = new Map<number, { selector: string, noWaitAfter?: boolean, resolved?: ManualPromise<void> }>();
  private _lastLocatorHandlerUid = 0;
  private _locatorHandlerRunningCounter = 0;

@ -432,9 +432,9 @@ export class Page extends SdkObject {
    }), this._timeoutSettings.navigationTimeout(options));
  }

-  registerLocatorHandler(selector: string, allowStayingVisible: boolean | undefined) {
+  registerLocatorHandler(selector: string, noWaitAfter: boolean | undefined) {
    const uid = ++this._lastLocatorHandlerUid;
-    this._locatorHandlers.set(uid, { selector, allowStayingVisible });
+    this._locatorHandlers.set(uid, { selector, noWaitAfter });
    return uid;
  }

@ -468,8 +468,12 @@ export class Page extends SdkObject {
        progress.log(`  found ${asLocator(this.attribution.playwright.options.sdkLanguage, handler.selector)}, intercepting action to run the handler`);
        const promise = handler.resolved.then(async () => {
          progress.throwIfAborted();
-          if (!handler.allowStayingVisible)
+          if (!handler.noWaitAfter) {
+            progress.log(`  locator handler has finished, waiting for ${asLocator(this.attribution.playwright.options.sdkLanguage, handler.selector)} to be hidden`);
            await this.mainFrame().waitForSelectorInternal(progress, handler.selector, { state: 'hidden' });
+          } else {
+            progress.log(`  locator handler has finished`);
+          }
        });
        await this.openScope.race(promise).finally(() => --this._locatorHandlerRunningCounter);
        // Avoid side-effects after long-running operation.
--- a/packages/playwright-core/types/types.d.ts
+++ b/packages/playwright-core/types/types.d.ts
@ -1790,8 +1790,6 @@ export interface Page {
  prependListener(event: 'worker', listener: (worker: Worker) => any): this;

  /**
-   * **NOTE** This method is experimental and its behavior may change in the upcoming releases.
-   *
   * When testing a web page, sometimes unexpected overlays like a "Sign up" dialog appear and block actions you want to
   * automate, e.g. clicking a button. These overlays don't always show up in the same way or at the same time, making
   * them tricky to handle in automated tests.
@ -1809,7 +1807,7 @@ export interface Page {
   *   handler is only called when you perform an action/assertion - if the overlay becomes visible but you don't
   *   perform any actions, the handler will not be triggered.
   * - After executing the handler, Playwright will ensure that overlay that triggered the handler is not visible
-   *   anymore. You can opt-out of this behavior with `allowStayingVisible`.
+   *   anymore. You can opt-out of this behavior with `noWaitAfter`.
   * - The execution time of the handler counts towards the timeout of the action/assertion that executed the handler.
   *   If your handler takes too long, it might cause timeouts.
   * - You can register multiple handlers. However, only a single handler will be running at a time. Make sure the
@ -1859,14 +1857,14 @@ export interface Page {
   * ```
   *
   * An example with a custom callback on every actionability check. It uses a `<body>` locator that is always visible,
-   * so the handler is called before every actionability check. It is important to specify `allowStayingVisible`,
-   * because the handler does not hide the `<body>` element.
+   * so the handler is called before every actionability check. It is important to specify `noWaitAfter`, because the
+   * handler does not hide the `<body>` element.
   *
   * ```js
   * // Setup the handler.
   * await page.addLocatorHandler(page.locator('body'), async () => {
   *   await page.evaluate(() => window.removeObstructionsForTestIfNeeded());
-   * }, { allowStayingVisible: true });
+   * }, { noWaitAfter: true });
   *
   * // Write the test as usual.
   * await page.goto('https://example.com');
@ -1893,7 +1891,7 @@ export interface Page {
     * Playwright will continue with the action/assertion that triggered the handler. This option allows to opt-out of
     * this behavior, so that overlay can stay visible after the handler has run.
     */
-    allowStayingVisible?: boolean;
+    noWaitAfter?: boolean;

    /**
     * Specifies the maximum number of times this handler should be called. Unlimited by default.
@ -3680,16 +3678,13 @@ export interface Page {
  }): Promise<null|Response>;

  /**
-   * **NOTE** This method is experimental and its behavior may change in the upcoming releases.
-   *
-   * Removes locator handler added by
-   * [page.addLocatorHandler(locator, handler[, options])](https://playwright.dev/docs/api/class-page#page-add-locator-handler).
+   * Removes all locator handlers added by
+   * [page.addLocatorHandler(locator, handler[, options])](https://playwright.dev/docs/api/class-page#page-add-locator-handler)
+   * for a specific locator.
   * @param locator Locator passed to
   * [page.addLocatorHandler(locator, handler[, options])](https://playwright.dev/docs/api/class-page#page-add-locator-handler).
-   * @param handler Handler passed to
-   * [page.addLocatorHandler(locator, handler[, options])](https://playwright.dev/docs/api/class-page#page-add-locator-handler).
   */
-  removeLocatorHandler(locator: Locator, handler: ((locator: Locator) => Promise<any>)): Promise<void>;
+  removeLocatorHandler(locator: Locator): Promise<void>;

  /**
   * Routing provides the capability to modify network requests that are made by a page.
--- a/packages/protocol/src/channels.ts
+++ b/packages/protocol/src/channels.ts
@ -1790,7 +1790,7 @@ export interface PageChannel extends PageEventTarget, EventTargetChannel {
  goForward(params: PageGoForwardParams, metadata?: CallMetadata): Promise<PageGoForwardResult>;
  registerLocatorHandler(params: PageRegisterLocatorHandlerParams, metadata?: CallMetadata): Promise<PageRegisterLocatorHandlerResult>;
  resolveLocatorHandlerNoReply(params: PageResolveLocatorHandlerNoReplyParams, metadata?: CallMetadata): Promise<PageResolveLocatorHandlerNoReplyResult>;
-  unregisterLocatorHandlerNoReply(params: PageUnregisterLocatorHandlerNoReplyParams, metadata?: CallMetadata): Promise<PageUnregisterLocatorHandlerNoReplyResult>;
+  unregisterLocatorHandler(params: PageUnregisterLocatorHandlerParams, metadata?: CallMetadata): Promise<PageUnregisterLocatorHandlerResult>;
  reload(params: PageReloadParams, metadata?: CallMetadata): Promise<PageReloadResult>;
  expectScreenshot(params: PageExpectScreenshotParams, metadata?: CallMetadata): Promise<PageExpectScreenshotResult>;
  screenshot(params: PageScreenshotParams, metadata?: CallMetadata): Promise<PageScreenshotResult>;
@ -1927,10 +1927,10 @@ export type PageGoForwardResult = {
 };
 export type PageRegisterLocatorHandlerParams = {
  selector: string,
-  allowStayingVisible?: boolean,
+  noWaitAfter?: boolean,
 };
 export type PageRegisterLocatorHandlerOptions = {
-  allowStayingVisible?: boolean,
+  noWaitAfter?: boolean,
 };
 export type PageRegisterLocatorHandlerResult = {
  uid: number,
@ -1943,13 +1943,13 @@ export type PageResolveLocatorHandlerNoReplyOptions = {
  remove?: boolean,
 };
 export type PageResolveLocatorHandlerNoReplyResult = void;
-export type PageUnregisterLocatorHandlerNoReplyParams = {
+export type PageUnregisterLocatorHandlerParams = {
  uid: number,
 };
-export type PageUnregisterLocatorHandlerNoReplyOptions = {
+export type PageUnregisterLocatorHandlerOptions = {

 };
-export type PageUnregisterLocatorHandlerNoReplyResult = void;
+export type PageUnregisterLocatorHandlerResult = void;
 export type PageReloadParams = {
  timeout?: number,
  waitUntil?: LifecycleEvent,
--- a/packages/protocol/src/protocol.yml
+++ b/packages/protocol/src/protocol.yml
@ -1350,7 +1350,7 @@ Page:
    registerLocatorHandler:
      parameters:
        selector: string
-        allowStayingVisible: boolean?
+        noWaitAfter: boolean?
      returns:
        uid: number

@ -1359,7 +1359,7 @@ Page:
        uid: number
        remove: boolean?

-    unregisterLocatorHandlerNoReply:
+    unregisterLocatorHandler:
      parameters:
        uid: number

--- a/tests/page/page-add-locator-handler.spec.ts
+++ b/tests/page/page-add-locator-handler.spec.ts
@ -66,7 +66,7 @@ test('should work with a custom check', async ({ page, server }) => {
  await page.addLocatorHandler(page.locator('body'), async () => {
    if (await page.getByText('This interstitial covers the button').isVisible())
      await page.locator('#close').click();
-  }, { allowStayingVisible: true });
+  }, { noWaitAfter: true });

  for (const args of [
    ['mouseover', 2],
@ -243,7 +243,7 @@ test('should work with times: option', async ({ page, server }) => {
  let called = 0;
  await page.addLocatorHandler(page.locator('body'), async () => {
    ++called;
-  }, { allowStayingVisible: true, times: 2 });
+  }, { noWaitAfter: true, times: 2 });

  await page.locator('#aside').hover();
  await page.evaluate(() => {
@ -278,7 +278,27 @@ test('should wait for hidden by default', async ({ page, server }) => {
  expect(called).toBe(1);
 });

-test('should work with allowStayingVisible', async ({ page, server }) => {
+test('should wait for hidden by default 2', async ({ page, server }) => {
+  await page.goto(server.PREFIX + '/input/handle-locator.html');
+
+  let called = 0;
+  await page.addLocatorHandler(page.getByRole('button', { name: 'close' }), async button => {
+    called++;
+  });
+
+  await page.locator('#aside').hover();
+  await page.evaluate(() => {
+    (window as any).clicked = 0;
+    (window as any).setupAnnoyingInterstitial('hide', 1);
+  });
+  const error = await page.locator('#target').click({ timeout: 3000 }).catch(e => e);
+  expect(await page.evaluate('window.clicked')).toBe(0);
+  await expect(page.locator('#interstitial')).toBeVisible();
+  expect(called).toBe(1);
+  expect(error.message).toContain(`locator handler has finished, waiting for getByRole('button', { name: 'close' }) to be hidden`);
+});
+
+test('should work with noWaitAfter', async ({ page, server }) => {
  await page.goto(server.PREFIX + '/input/handle-locator.html');

  let called = 0;
@ -288,7 +308,7 @@ test('should work with allowStayingVisible', async ({ page, server }) => {
      await button.click();
    else
      await page.locator('#interstitial').waitFor({ state: 'hidden' });
-  }, { allowStayingVisible: true });
+  }, { noWaitAfter: true });

  await page.locator('#aside').hover();
  await page.evaluate(() => {
@ -305,11 +325,10 @@ test('should removeLocatorHandler', async ({ page, server }) => {
  await page.goto(server.PREFIX + '/input/handle-locator.html');

  let called = 0;
-  const handler = async locator => {
+  await page.addLocatorHandler(page.getByRole('button', { name: 'close' }), async locator => {
    ++called;
    await locator.click();
-  };
-  await page.addLocatorHandler(page.getByRole('button', { name: 'close' }), handler);
+  });

  await page.evaluate(() => {
    (window as any).clicked = 0;
@ -324,7 +343,7 @@ test('should removeLocatorHandler', async ({ page, server }) => {
    (window as any).clicked = 0;
    (window as any).setupAnnoyingInterstitial('hide', 1);
  });
-  await page.removeLocatorHandler(page.getByRole('button', { name: 'close' }), handler);
+  await page.removeLocatorHandler(page.getByRole('button', { name: 'close' }));

  const error = await page.locator('#target').click({ timeout: 3000 }).catch(e => e);
  expect(called).toBe(1);