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

[REGRESSION]: context.pages() now works differently #2878

Closed
GoldStrikeArch opened this issue Jul 8, 2020 · 6 comments
Closed

[REGRESSION]: context.pages() now works differently #2878

GoldStrikeArch opened this issue Jul 8, 2020 · 6 comments

Comments

@GoldStrikeArch
Copy link

Context:

  • GOOD Playwright Version: [what Playwright version worked nicely?]
  • BAD Playwright Version: [what Playwright version doesn't work any more?]
  • Operating System: [e.g. Windows, Linux or Mac]
  • Extra: [any specific details about your environment]

-GOOD Playwright 1.0.1

-BAD Playwright 1.2.0

-Operating System: Windows 10

Code Snippet

Help us help you! Put down a short code snippet that illustrates your bug and
that we can run and debug locally. For example:

const {chromium, webkit, firefox} = require('playwright');

(async () => {
  const browser = await chromium.launch();
  const context = await browser.newContext();
  const page = await context.newPage();
 
await page.click('[name="Add new child window"]')

//This code worked back in 1.0.1
const allPages = await context.pages();
const childWindowPage = allPages[allPages.length - 1];

///////////////////////////////////////////////////////////////////

// Now you need to do it this way (1.2.0 version)

await page.waitForTimeout(5000);

const allPages = await context.pages();
const childWindowPage = allPages[allPages.length - 1];
})();

Describe the bug

For some reason in current (1.2.0) version you must wait till page is loaded, before you can grab it and assign it to a variable via context.pages() method... This wasnt the case in 1.0.1 version.

@dgozman
Copy link
Contributor

dgozman commented Jul 8, 2020

The old snippet from 1.0.1 happened to work accidentally, because sometimes page.click() was resolving after the new page has been created. In 1.2, this happens less frequently - that's why you see that snippet does not work anymore.

Overall, Playwright does not guarantee that by the time page.click() is resolved, all the pages opened by this click are available. Moreover, this is almost impossible to guarantee, because it is hard to determine whether the new page was indeed a result of a click or not, and whether the click will open a page after a timeout.

Instead, we recommend the following way to capture popups from clicks:

const [childWindowPage] = await Promise.all([
  page.waitForEvent('popup'),
  page.click('[name="Add new child window"]')
]);

// Wait for the child page to load before accessing the content.
await childWindowPage.waitForLoadState();

// Ready to use.
const title = await childWindowPage.title();

You can find even more samples in our documentation.

@shirshak55
Copy link

@dgozman

I think even this code has race condition

await Promise.all([
  page.waitForEvent('popup'),
  page.click('[name="Add new child window"]')
]);

Chances are low but that doesn't mean race condition cannot happen. Spurious event do occur time to time.

@dgozman
Copy link
Contributor

dgozman commented Jul 10, 2020

Chances are low but that doesn't mean race condition cannot happen. Spurious event do occur time to time.

Well, I assume that you expect a popup as a result of the click, so there must be at least one popup. If there could be multiple, I'd suggest to use page.on('popup') and capture all of them. Is that what you are worried about, or did I misunderstand?

@GoldStrikeArch
Copy link
Author

GoldStrikeArch commented Jul 12, 2020

@dgozman
What I am saying is, that context.pages() doesn't includes the pages that are not loaded yet (the behaviour that I am seeing in 1.2.0). If new browser tab is created, why not include it? Even if it doesn't have load state of 'DOMcontentloaded' or 'load'. Thanks for alternative solution,
this:

await Promise.all([
  page.waitForEvent('popup'),// or page.waitForEvent('page')
  page.click('[name="Add new child window"]')
]);

works well, but I want to have an easy access to all pages of current context even if it those pages are loading.

@dgozman
Copy link
Contributor

dgozman commented Jul 15, 2020

@MihailPertsev The actual change in 1.2.0 is that page.click() returns earlier instead of taking long enough time for the new page to appear in context.pages().

It would indeed be handy if context.pages() would give you pages immediately after click, but we cannot guarantee that because:

  • click does not guarantee to create the new page instantly, for example it might do so after a timeout;
  • it is hard to determine whether the new page was indeed a result of a click or not, for example it might as well be a result of some fetch() finishing;
  • the new page is not initialized instantly in the browser and Playwright.
    Therefore, we cannot be sure that there is a page by the time page.click() resolves. The alternative would be to wait for undetermined amount of time in page.click(), but we cannot afford that.

Hopefully, this explanation helps a bit.

@GoldStrikeArch
Copy link
Author

@dgozman

the new page is not initialized instantly in the browser and Playwright.

Now I get it. Thanks!

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

No branches or pull requests

4 participants