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

Wait until idle before starting detection #978

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Wait until idle before starting detection #978

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a0d862
Wait until idle before starting detection
westonruter Feb 9, 2024
7d3a3ea
Move isStorageLocked check after idle since sessionStorage is synchro…
westonruter Feb 9, 2024
dcdce43
Update modules/images/image-loading-optimization/detection/detect.js
adamsilverstein Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 48 additions & 38 deletions modules/images/image-loading-optimization/detection/detect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,16 +155,6 @@ export default async function detect( {
} ) {
const currentTime = getCurrentTime();

// As an alternative to this, the ilo_print_detection_script() function can short-circuit if the
// ilo_is_url_metric_storage_locked() function returns true. However, the downside with that is page caching could
// result in metrics being missed being gathered when a user navigates around a site and primes the page cache.
if ( isStorageLocked( currentTime, storageLockTTL ) ) {
if ( isDebug ) {
warn( 'Aborted detection due to storage being locked.' );
}
return;
}

// Abort running detection logic if it was served in a cached page.
if ( currentTime - serveTime > detectionTimeWindow ) {
if ( isDebug ) {
Expand All @@ -175,20 +165,55 @@ export default async function detect( {
return;
}

// Prevent detection when page is not scrolled to the initial viewport.
// TODO: Does this cause layout/reflow? https://gist.github.com/paulirish/5d52fb081b3570c81e3a
if ( doc.documentElement.scrollTop > 0 ) {
// Abort if the current viewport is not among those which need URL metrics.
if ( ! isViewportNeeded( win.innerWidth, neededMinimumViewportWidths ) ) {
if ( isDebug ) {
warn(
'Aborted detection since initial scroll position of page is not at the top.'
);
log( 'No need for URL metrics from the current viewport.' );
}
return;
}

if ( ! isViewportNeeded( win.innerWidth, neededMinimumViewportWidths ) ) {
// Ensure the DOM is loaded (although it surely already is since we're executing in a module).
await new Promise( ( resolve ) => {
if ( doc.readyState !== 'loading' ) {
resolve();
} else {
doc.addEventListener( 'DOMContentLoaded', resolve, { once: true } );
}
} );

// Wait until the resources on the page have fully loaded.
await new Promise( ( resolve ) => {
if ( doc.readyState === 'complete' ) {
resolve();
} else {
win.addEventListener( 'load', resolve, { once: true } );
}
} );

// Wait yet further until idle.
if ( typeof requestIdleCallback === 'function' ) {
await new Promise( ( resolve ) => {
requestIdleCallback( resolve );
} );
}

// As an alternative to this, the ilo_print_detection_script() function can short-circuit if the
// ilo_is_url_metric_storage_locked() function returns true. However, the downside with that is page caching could
// result in metrics missed from being gathered when a user navigates around a site and primes the page cache.
if ( isStorageLocked( currentTime, storageLockTTL ) ) {
if ( isDebug ) {
log( 'No need for URL metrics from the current viewport.' );
warn( 'Aborted detection due to storage being locked.' );
}
return;
}

// Prevent detection when page is not scrolled to the initial viewport.
Copy link
Member

Choose a reason for hiding this comment

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

this makes me wonder if on the server side we should potentially avoid using the captured data when the link is apparently to a non scrolled to top position - ie. when it contains an anchor hash. Do you do that already or what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

That effectively is what this is doing, yes.

As for server-side detection of this, the URL fragment is not sent along in the HTTP request for the page, so the server won't be able to see it. (Which is why annoyingly URL fragments can be lost when having to authenticate.) It would have to be checked client-side. And often a URL fragment may not refer to an element on the page so that would require checking the DOM to see if it exists. But we only care in the end if the user is not in the first viewport, so that's what checking the scroll position does, I think in a more reliable way.

if ( doc.documentElement.scrollTop > 0 ) {
if ( isDebug ) {
warn(
'Aborted detection since initial scroll position of page is not at the top.'
);
}
return;
}
Expand All @@ -197,7 +222,6 @@ export default async function detect( {
log( 'Proceeding with detection' );
}

// TODO: This query no longer needs to be done as early as possible since the server is adding the breadcrumbs.
const breadcrumbedElements =
doc.body.querySelectorAll( '[data-ilo-xpath]' );

Expand All @@ -212,15 +236,6 @@ export default async function detect( {
)
);

// Ensure the DOM is loaded (although it surely already is since we're executing in a module).
await new Promise( ( resolve ) => {
if ( doc.readyState !== 'loading' ) {
resolve();
} else {
doc.addEventListener( 'DOMContentLoaded', resolve, { once: true } );
}
} );

/** @type {IntersectionObserverEntry[]} */
const elementIntersections = [];

Expand Down Expand Up @@ -286,15 +301,6 @@ export default async function detect( {
);
} );

// Wait until the resources on the page have fully loaded.
await new Promise( ( resolve ) => {
if ( doc.readyState === 'complete' ) {
resolve();
} else {
win.addEventListener( 'load', resolve, { once: true } );
}
} );

// Stop observing.
disconnectIntersectionObserver();
if ( isDebug ) {
Expand Down Expand Up @@ -348,7 +354,11 @@ export default async function detect( {
log( 'URL metrics:', urlMetrics );
}

// TODO: Wait until idle? Yield to main?
// Yield to main before sending data to server to further break up task.
await new Promise( ( resolve ) => {
setTimeout( resolve, 0 );
} );

try {
const response = await fetch( restApiEndpoint, {
method: 'POST',
Expand Down
Loading