Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

waitForSelector with visible:true not returning the first visible element, causes timeout. #4356

Open
razorman8669 opened this issue Apr 27, 2019 · 16 comments

Comments

@razorman8669
Copy link

razorman8669 commented Apr 27, 2019

Steps to reproduce

Tell us about your environment:

What steps will reproduce the problem?
Open Website OR use this file:

<html>
  <head>
    <meta charset="UTF-8" />
  </head>

  <body>
    <input style="display: none" type="number" class="my-input-class" />
    <input type="number" class="my-input-class" />
  </body>
</html>

Please include code that reproduces the issue.

const browser = await puppeteer.launch();

const page = await browser.newPage();
await page.goto('https://j4q389wzv3.codesandbox.io/');

try {
  const visibleInput = await page.waitForSelector('.my-input-class', {visible: true, timeout: 1000 })
  console.log('Found visible Element')
} catch (e) {
  console.log('Could NOT find a visible element ', e.message)
}

const inputs = await page.$$('.my-input-class')
console.log(`Found ${inputs.length} inputs`)

await browser.close()

What is the expected result?
There are 2 elements matching the CSS Selector on the page. the first one is hidden, the second one is visible. The page.waitForSelector with {visible: true} should have found and returned the visible element on the page.

What happens instead?
It Times Out since there was another HIDDEN element matching the CSS selector higher up in the DOM structure, causing it to wait until timeout even though a visible element exists on the page.

@razorman8669
Copy link
Author

razorman8669 commented Apr 28, 2019

A working solution (though, not ideal) is as follows. Ideally the waitForSelector with { visible:true } would behave this way... or the documentation clearly states that it doesn't do this.

/** Internal method to determine if an elementHandle is visible on the page. */
const _isVisible = async(page, elementHandle) => await page.evaluate((el) => {
  if (!el || el.offsetParent === null) {
    return false;
  }

  const style = window.getComputedStyle(el);
  return style && style.display !== 'none' && style.visibility !== 'hidden' && style.opacity !== '0';
}, elementHandle);

/**
 * Checks if an element with selector exists in DOM and is visible.
 * @param {*} page
 * @param {*} selector CSS Selector.
 * @param {*} timeout amount of time to wait for existence and visible.
 */
const waitForVisible = async(page, selector, timeout=25) => {
  const startTime = new Date();
  try {
    await page.waitForSelector(selector, { timeout: timeout });
    // Keep looking for the first visible element matching selector until timeout
    while (true) {
      const els = await page.$$(selector);
      for(const el of els) {
        if (await _isVisible(page, el)) {
          console.log(`PASS Check visible : ${selector}`);
          return el;
        }
      }
      if (new Date() - startTime > timeout) {
        throw new Error(`Timeout after ${timeout}ms`);
      }
      page.waitFor(50);
    }
  } catch (e) {
    console.log(`FAIL Check visible : ${selector}`);
    return false;
  }
};

@JoelEinbinder
Copy link
Collaborator

Sounds reasonable. PRs welcome.

razorman8669 added a commit to razorman8669/puppeteer that referenced this issue Apr 30, 2019
This patch fixes an issue where waitForSelector with visible:true would cause a timeout should there be multiple elements matching the selector, but the ones higher up on the DOM are hidden.  It now returns the first visible element it finds.

fixes puppeteer#4356
razorman8669 added a commit to razorman8669/puppeteer that referenced this issue Apr 30, 2019
This patch fixes an issue where waitForSelector with visible:true would cause a timeout should there be multiple elements matching the selector, but the ones higher up on the DOM are hidden. It now returns the first visible element it finds.

fixes puppeteer#4356
@razorman8669
Copy link
Author

@JoelEinbinder PR is up, though you guys don't have any info or docs on testing/building for the experimental/firefox stuff. what's the protocol?

@aslushnikov
Copy link
Contributor

The page.waitForSelector with {visible: true} should have found and returned the visible element on the page.

@razorman8669 Ok I see what you mean here. Indeed, the behavior is somewhat confusing.

However, I disagree with the proposed approach. CSS selector is the only thing that we should use to identify the element. Adding the suggested logic adds magic to how we find elements - making it equally hard to debug.

The best solution here is to update the documentation - so that we do a better job explaining what's going on.

@prneelak
Copy link

prneelak commented Sep 5, 2019

So , since razorman's fix did not go through. What is the alternative if I hit this case ? I do wish to skip the first selector ( inside display none) and assert on the second selector like the bug explains. Any alternatives or suggestions ?

@yashLadha
Copy link
Contributor

Hey, @aslushnikov is this task up for grab? I would like to kickstart my contribution and looking for a good first issue. Also, I completely agree with you as technologies like selenium also moving towards finding element through CSS selector components only and giving consistent behaviour across technologies should be an ideal thing to do.

@mercertom
Copy link

I'm having this same issue. For most instances of the element in question, parent elements are hidden, and I need the visible elements matching the selector. How are we supposed to find the 1 (or more) visible element among many hidden elements with the same selector?

@mitbhuptani
Copy link

Hi Guys, I would like to help and contribute to the repo. Is this bug still up for grabs ?. I am actually new to open source contribution, so is there any way I can kickstart my contributions. Would like your help here.

@99littlebugs
Copy link

Similar situation that I think is being caused by the same bug. I would like to use a , in my selector on a page where only 1 of 2 elements will be visible.
Selector is #item1, #item2. If #item1 becomes visible, this works as expected, if #item2 does, I get a timeout.

@hsks
Copy link

hsks commented May 31, 2021

Any updates on this? What should be the recommended approach in this case?

@AdrianNeatu
Copy link

AdrianNeatu commented Feb 10, 2022

You can do: await page.waitForSelector('.my-input-class:visible')

@stale
Copy link

stale bot commented Jun 23, 2022

We're marking this issue as unconfirmed because it has not had recent activity and we weren't able to confirm it yet. It will be closed if no further activity occurs within the next 30 days.

@stale stale bot added the unconfirmed label Jun 23, 2022
@mercertom
Copy link

It's been confirmed repeatedly. Don't know who "we" is in a bot's language.

@stale stale bot removed the unconfirmed label Jun 23, 2022
@stale
Copy link

stale bot commented Aug 30, 2022

We're marking this issue as unconfirmed because it has not had recent activity and we weren't able to confirm it yet. It will be closed if no further activity occurs within the next 30 days.

@stale stale bot added the unconfirmed label Aug 30, 2022
@mercertom
Copy link

It's been confirmed repeatedly. Don't know who "we" is in a bot's language.

@stale stale bot removed the unconfirmed label Aug 30, 2022
@OrKoN OrKoN added feature and removed bug labels Sep 5, 2022
@OrKoN OrKoN added the confirmed label Sep 5, 2022
@OrKoN
Copy link
Collaborator

OrKoN commented Sep 5, 2022

I don't think we are not going to support the feature request in the near future. As pointed out in #4356 (comment), the web APIs don't offer a way to query only the visible tree of elements and we should not change the semantic of CSS selectors. The workarounds are 1) use selectors that identify the visible element uniquely or 2) implement retries + waiting heuristics for becoming visible that are specific to your use case.

Perhaps we should change the API to be const el = await page.waitForSelector(); await el.waitToBecomeVisible() to avoid ambiguity in addition to better documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet