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.

Sign up for GitHub

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

Jump to bottom

Wait for lazy loaded elements when using the fullPage option #40

Merged
merged 15 commits into from Aug 10, 2020
Merged

Wait for lazy loaded elements when using the fullPage option #40

Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d81b43a
Wait for lazy loaded elements when using the ullPage option
Nov 13, 2019
d4000ad
Had to use eslint-disable. Had no clue how to fix these issues, altho…
Nov 13, 2019
d61dc4c
Added await proper function for images
Feb 24, 2020
5dec125
Timeout of 30 was too short, pushed to 60
Feb 24, 2020
8662efa
Update index.js
sgtrusty Mar 13, 2020
a940db1
Variable name fix
sgtrusty Mar 13, 2020
c165d3d
Added navigationPromise and using adequate variable names instead.
Mar 13, 2020
4b80210
Updates (#1)
sgtrusty Apr 12, 2020
16eec30
Added testing script
Apr 12, 2020
990ad56
Add provisional changes
sgtrusty Aug 2, 2020
e0ebd01
Update test.js
sgtrusty Aug 2, 2020
5f1aa86
Update test.js
sgtrusty Aug 2, 2020
ebb5c1c
Update test.js
sindresorhus Aug 10, 2020
7ae2147
Update test.js
sindresorhus Aug 10, 2020
46ee934
Merge branch 'master' into master
sindresorhus Aug 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions index.js
View file
Open in desktop
Expand Up @@ -128,6 +128,8 @@ const parseCookie = (url, cookie) => {
return ret;
};

const imagesHaveLoaded = () => [...document.images].map(element => element.complete);

const captureWebsite = async (input, options) => {
options = {
inputType: 'url',
Expand Down Expand Up @@ -321,6 +323,39 @@ const captureWebsite = async (input, options) => {
await options.beforeScreenshot(page, browser);
}

if (screenshotOptions.fullPage) {
// Get the height of the rendered page
const bodyHandle = await page.$('body');
const bodyBoundingHeight = await bodyHandle.boundingBox();
await bodyHandle.dispose();

// Scroll one viewport at a time, pausing to let content load
const viewportHeight = viewportOptions.height;
let viewportIncrement = 0;
while (viewportIncrement + viewportHeight < bodyBoundingHeight) {
const navigationPromise = page.waitForNavigation();
Copy link

@ghost ghost Apr 8, 2020 •
edited by ghost

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this line wait for networkidle instead of the default loaded? I assume this promise will be awaited to let the images load, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably agree with you, but care to clarify?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably agree with you, but care to clarify?

Waiting for network idle allows lazy elements to load (elements loaded after the "load" event of the document). I suggest doing as follows:

  1. Wait "load" event of the document.
  2. Scroll the page to its maximum Y (this will load lazy elements like lazy loaded images).
  3. Wait for network idle.

Copy link

@ghost ghost Apr 12, 2020 •
edited by ghost

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also worth to keep in mind that some (old) websites can actually never fulfill the requirements for the network idle navigation event - for example frequent chat requests.

In the end, the propsed flow above should work just fine (most cases).

/* eslint-disable no-await-in-loop */
await page.evaluate(_viewportHeight => {
/* eslint-disable no-undef */
window.scrollBy(0, _viewportHeight);
/* eslint-enable no-undef */
}, viewportHeight);
await navigationPromise;
/* eslint-enable no-await-in-loop */
viewportIncrement += viewportHeight;
}

// Scroll back to top
await page.evaluate(_ => {
/* eslint-disable no-undef */
window.scrollTo(0, 0);
/* eslint-enable no-undef */
});

// Some extra delay to let images load
await page.waitForFunction(imagesHaveLoaded, {timeout: 60});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the user would expect it to take up to one minute. If we keep this, it needs to be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also be added as an argument with default value set to timeout 60. Might want to comment on this or implement some other better functionality? Maybe it's also worth looking into alternate functions, such as the waitForNavigation Vasile mentioned earlier.

This

await wait(100);

is not good.

You should wait actually for images and other snippets to load, not giving them 100ms...
The waitForNavigation method waiting for a network idle should be used instead.

In case it might be useful for documentation purposes; I retrieved part of the solution from https://stackoverflow.com/questions/51651830/how-to-wait-for-all-images-to-load-from-page-evaluate-function-in-puppeteer-when

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would look into something better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using page.waitForNavigation as per pointed by #40 (review)

}

const buffer = await page.screenshot(screenshotOptions);

await page.close();
Expand Down