playwright icon indicating copy to clipboard operation
playwright copied to clipboard

[BUG] Click misses target due to animation

Open leotg130 opened this issue 2 years ago • 3 comments

Context:

  • Playwright Version: 1.22.1
  • Operating System: Linux
  • Node.js version: 16.13.2
  • Browser: Chromium

Code Snippet

import { expect, test } from '@playwright/test';

const html = `
<html>
<head>
  <style>
    .expanded .list {
      max-height: 100%;
    }
    .list {
      transition: max-height 0.25s cubic-bezier(0.82, 0, 0.1, 1);
      max-height: 0;
      overflow: auto;
    }
  </style>
  <script>
    function headerHandler() {
      const parent = document.getElementById('parent');
      if (parent.classList.contains('expanded')) {
        parent.classList.remove('expanded')
      } else {
        parent.classList.add('expanded')
      }
    }

    function optionHandler(i) {
      document.getElementById('result').innerHTML = 'O' + i;
    }
  </script>
</head>
<body>
<div id="parent">
  <div>
    <span onclick="headerHandler()">Header</span>
  </div>
  <ul class="list">
    <li onclick="optionHandler(1)">Option1</li>
    <li onclick="optionHandler(2)">Option2</li>
    <li onclick="optionHandler(3)">Option3</li>
    <li onclick="optionHandler(4)">Option4</li>
    <li onclick="optionHandler(5)">Option5</li>
    <li onclick="optionHandler(6)">Option6</li>
    <li onclick="optionHandler(7)">Option7</li>
  </ul>
</div>
<div id="result"></div>
</body>
</html>
`;

test('repro', async ({page}) => {
  await page.setContent(html);

  const testClick = async (i: number) => {
    await page.click('text=Header');
    await page.click(`text=Option${i}`);
    await page.click('text=Header');
    await expect(page.locator('#result')).toHaveText(`O${i}`);
    await page.waitForTimeout(260); // Some time for the menu to close, essentially resetting state.
  }

  for (let i = 0; i < 100; i++) {
    await testClick(6);
    await testClick(7);
  }
});

Describe the bug

Seems like PW is not waiting for animation to finish before trying to click an element, causing it to click wrong element. Due to the 100 iterations within the test it fails quite reliably, so this doesn't happen very often in the above code, though with our real code it happens a bit more, maybe around 10% of the time.

The trace sometimes shows the red dot (where PW is going to click) quite a bit off from the intended element, while the intended element is highlighted in blue.

Video on the other hand shows that PW is clicking before animation has finished.

leotg130 avatar Jul 29 '22 13:07 leotg130

@leotg130 would this API help in your case? https://github.com/microsoft/playwright/issues/15660

unlikelyzero avatar Aug 01 '22 15:08 unlikelyzero

Yes, the waitForAnimationEnd() mentioned in https://github.com/microsoft/playwright/issues/15660#issuecomment-1185339343 does work. I'll probably use that at least for now, as it seems to be 100% stable.

  • Will that become part of the Playwright API?
  • Why does the stable only wait for two consecutive animation frames, instead of animation to finish? https://playwright.dev/docs/actionability#stable

PS: I noticed that the trace viewer doesn't account the waiting of this evaluate promise for finished animations correctly. I increased the animation to 1s to make it more visible. In the top bar you can easily see huge chunks of time as simply background white, and in the left hand list of actions it's ~50-100 ms.

image

leotg130 avatar Aug 04 '22 10:08 leotg130

@leotg130 you should bring up your findings on that RFE and give it a 👍 to raise the priority

unlikelyzero avatar Aug 04 '22 14:08 unlikelyzero

Yes, the waitForAnimationEnd() mentioned in #15660 (comment) does work. I'll probably use that at least for now, as it seems to be 100% stable.

* Will that become part of the Playwright API?

* Why does the `stable` only wait for two consecutive animation frames, instead of animation to finish? https://playwright.dev/docs/actionability#stable

PS: I noticed that the trace viewer doesn't account the waiting of this evaluate promise for finished animations correctly. I increased the animation to 1s to make it more visible. In the top bar you can easily see huge chunks of time as simply background white, and in the left hand list of actions it's ~50-100 ms.

image

Doesn't work for elements animated with react-spring

skysantoroa avatar Aug 31 '22 10:08 skysantoroa

Cypress has this option: interacting-with-elements#animations. Maybe in some cases it makes sense to adjust the stable option? See playwright source for injectedScript. Here we have the time hardcoded to 15.


I can confirm this problem. We have a side panel in our backoffice with entries hidden behind a accordion animation, which animates the items down. Playwright sometimes clicks above the links. Headless, this problem appears to happen often, without headless it works sometimes. A quickfix with waitForTimeout(300) solved this, but as known, this should not be used except in creating tests.

I also tried to fix this with getAnimations({ subtree: true })/ observers, but as many users have the same problem I think this solving should be baked inside playwright itself.


Headless attempts

First attempt (should click "Auswertungen"):
image

Second attempt: Success

Third attempt:
image (You see that the click point is slightly more down than the previous.)

Without headless

First attempt: Success Second attempt: Success Third attempt: Success ... 5 attempt: Failed image

Edit: Log

waiting for selector "role=link[name="Auswertungen "i]"
selector resolved to visible <a href="#">…</a>
attempting click action
waiting for element to be visible, enabled and stable
element is visible, enabled and stable
scrolling into view if needed
done scrolling
performing click action
click action done
waiting for scheduled navigations to finish
navigations have finished

MarkusJLechner avatar Oct 25 '22 07:10 MarkusJLechner

For the time beeing i took the code from the injected functions and make the stable count accessible. Now the tests run through smoothly. Also if inspecting the trace, the click action is exactly after the animation

// await waitStable(page.getByRole('link', { name: 'Auswertungen' })).then((l) => l.click())

export async function waitStable(locator: Locator, stableFrames: number = 20) {
  await locator.evaluate(async (element: SVGElement | HTMLElement, stableFrames: number) => {
    async function progressIsStable(
      element: SVGElement | HTMLElement,
      lastRect: { x: number; y: number; width: number; height: number } | null = null,
      samePositionCounter: number = 0,
      iteration: number = 0
    ): Promise<void> {
      // kill switch
      if (iteration > 500) {
        return
      }

      await new Promise((resolve) => setTimeout(resolve, 15))

      const clientRect = element.getBoundingClientRect()
      const rect = { x: clientRect.left, y: clientRect.top, width: clientRect.width, height: clientRect.height }
      const samePosition =
        lastRect &&
        rect.x === lastRect.x &&
        rect.y === lastRect.y &&
        rect.width === lastRect.width &&
        rect.height === lastRect.height

      if (samePosition) {
        ++samePositionCounter
      } else {
        samePositionCounter = 0
      }

      const isStable = samePositionCounter >= stableFrames

      lastRect = rect

      if (!isStable) {
        return progressIsStable(element, lastRect, samePositionCounter, ++iteration)
      }
    }

    return progressIsStable(element)
  }, stableFrames)

  return locator
}

MarkusJLechner avatar Oct 26 '22 09:10 MarkusJLechner

For the time beeing i took the code from the injected functions and make the stable count accessible. Now the tests run through smoothly. Also if inspecting the trace, the click action is exactly after the animation

// await waitStable(page.getByRole('link', { name: 'Auswertungen' })).then((l) => l.click())

export async function waitStable(locator: Locator, stableFrames: number = 20) {
  await locator.evaluate(async (element: SVGElement | HTMLElement, stableFrames: number) => {
    async function progressIsStable(
      element: SVGElement | HTMLElement,
      lastRect: { x: number; y: number; width: number; height: number } | null = null,
      samePositionCounter: number = 0,
      iteration: number = 0
    ): Promise<void> {
      // kill switch
      if (iteration > 500) {
        return
      }

      await new Promise((resolve) => setTimeout(resolve, 15))

      const clientRect = element.getBoundingClientRect()
      const rect = { x: clientRect.left, y: clientRect.top, width: clientRect.width, height: clientRect.height }
      const samePosition =
        lastRect &&
        rect.x === lastRect.x &&
        rect.y === lastRect.y &&
        rect.width === lastRect.width &&
        rect.height === lastRect.height

      if (samePosition) {
        ++samePositionCounter
      } else {
        samePositionCounter = 0
      }

      const isStable = samePositionCounter >= stableFrames

      lastRect = rect

      if (!isStable) {
        return progressIsStable(element, lastRect, samePositionCounter, ++iteration)
      }
    }

    return progressIsStable(element)
  }, stableFrames)

  return locator
}

What is stableFrames?

skysantoroa avatar Oct 26 '22 10:10 skysantoroa

I updated the function to include the waitForAnimations snippet + renamed stableFrames to samePositionCount to make it more clear what it is. @skysantoroa Basically playwright looks for two iterations to have the same position, then it is called stable. With the samePositionCount you can increase the stable iterations to a higher number. Between one iteration there is a sleep for 15ms, i was reading something about a win webkit bug, for safety i leave it there

export async function waitStable(locator: Locator, waitForAnimations: boolean = false, samePositionCount: number = 20) {
  await locator.evaluate(
    async (element: SVGElement | HTMLElement, { waitForAnimations, samePositionCount }) => {
      // https://github.com/microsoft/playwright/blob/98215b4d74030dac7a98a37c67650fd9d0b82509/packages/playwright-core/src/server/injected/injectedScript.ts#L527-L586
      async function progressIsStable(
        element: SVGElement | HTMLElement,
        lastRect: { x: number; y: number; width: number; height: number } | null = null,
        samePositionCounter: number = 0,
        iteration: number = 0
      ): Promise<void> {
        // kill switch
        if (iteration > 500) {
          return
        }

        if (waitForAnimations) {
          await Promise.all(element.getAnimations().map((animation) => animation.finished))
        }

        await new Promise((resolve) => setTimeout(resolve, 15))

        const clientRect = element.getBoundingClientRect()
        const rect = { x: clientRect.left, y: clientRect.top, width: clientRect.width, height: clientRect.height }
        const samePosition =
          lastRect &&
          rect.x === lastRect.x &&
          rect.y === lastRect.y &&
          rect.width === lastRect.width &&
          rect.height === lastRect.height

        if (samePosition) {
          ++samePositionCounter
        } else {
          samePositionCounter = 0
        }

        const isStable = samePositionCounter >= samePositionCount

        lastRect = rect

        if (!isStable) {
          return progressIsStable(element, lastRect, samePositionCounter, ++iteration)
        }
      }

      return progressIsStable(element)
    },
    { waitForAnimations, samePositionCount }
  )

  return locator
}

MarkusJLechner avatar Oct 26 '22 10:10 MarkusJLechner

Why was this issue closed?

Thank you for your contribution to our project. This issue has been closed due to its limited upvotes and recent activity, and insufficient feedback for us to effectively act upon. Our priority is to focus on bugs that reflect higher user engagement and have actionable feedback, to ensure our bug database stays manageable.

Should you feel this closure was in error, please create a new issue and reference this one. We're open to revisiting it given increased support or additional clarity. Your understanding and cooperation are greatly appreciated.

pavelfeldman avatar Jun 30 '23 20:06 pavelfeldman