Skip to content

Prevent submitting URL Metric if viewport size changed #1712

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

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Prevent submitting URL Metric if viewport size changed #1712

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5bd367
Prevent submitting URL Metric if viewport size changed
westonruter Nov 23, 2024
e222c85
Use resize event to detect viewport change
westonruter Dec 2, 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
25 changes: 21 additions & 4 deletions plugins/optimization-detective/detect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,12 @@ function recursiveFreeze( obj ) {
Object.freeze( obj );
}

// This needs to be captured early in case the user later resizes the window.
const viewport = {
width: win.innerWidth,
height: win.innerHeight,
};

/**
* URL Metric being assembled for submission.
*
Expand Down Expand Up @@ -443,10 +449,7 @@ export default async function detect( {

urlMetric = {
url: currentUrl,
viewport: {
width: win.innerWidth,
height: win.innerHeight,
},
viewport,
elements: [],
};

Expand Down Expand Up @@ -507,6 +510,20 @@ export default async function detect( {
);
} );

// Only proceed with submitting the URL Metric if viewport stayed the same size. Changing the viewport size (e.g. due
// to resizing a window or changing the orientation of a device) will result in unexpected metrics being collected.
if (
window.innerWidth !== urlMetric.viewport.width ||
window.innerHeight !== urlMetric.viewport.height
) {
if ( isDebug ) {
log(
'Aborting URL Metric collection due to viewport size change.'
);
}
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

From @felixarntz in #1697 (comment):

Couldn't this be moved much further up in the logic, given we'll know whether the viewport has changed far earlier in this function? This could avoid doing quite a bit of "useless" processing as far as I can tell.

Copy link
Member

Choose a reason for hiding this comment

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

Awaiting an answer to this question, otherwise LGTM :)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it cannot be moved up. Note the immediately preceding code:

performance/plugins/optimization-detective/detect.js

Lines 497 to 511 in a5bd367

// Wait for the page to be hidden.
await new Promise( ( resolve ) => {
win.addEventListener( 'pagehide', resolve, { once: true } );
win.addEventListener( 'pageswap', resolve, { once: true } );
doc.addEventListener(
'visibilitychange',
() => {
if ( document.visibilityState === 'hidden' ) {
// TODO: This will fire even when switching tabs.
resolve();
}
},
{ once: true }
);
} );

Recall that the URL Metric is now sent when someone leaves the page, not as soon as the page is fully loaded. So data is collected up until the moment someone leaves the page, and if at this moment we see that the page is not the same dimensions, then we abort sending the URL Metric.

That said, I realize that a resize event listener would be more robust, as it could be that someone resizes the window once the page has loaded, and then before leaving they resize it back to the original size. That wouldn't be detected as it is here. I've implemented resize event in e222c85.

Copy link
Member Author

Choose a reason for hiding this comment

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

@felixarntz Does this make sense?

Copy link
Member

Choose a reason for hiding this comment

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

@westonruter Makes sense. I wonder though, would there be other tasks here that could be deferred to when leaving the page so that they don't run unnecessarily? For example some of the LCP element logic that runs before then as far as I can see?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, although I think if we wait until this point to call onLCP() I'd worry about increasing the amount of time needing to be spent at this pagehide phase and risk the browser killing the tasks before it gets a chance to send the beacon. But I think that is something that we can investigate further in a follow-up.


if ( extensions.size > 0 ) {
for ( const [
extensionModuleUrl,
Expand Down
Loading