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

Add Opportunity and Experiment for images that are too large for display #2550

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

scottjehl
Copy link
Contributor

Note: not ready until we decide on an image API service, which will be set via settings.
Also: This PR builds upon #2548

This new opportunity detects images with natural widths that are more than twice their display width in the page layout. While it does not go further to determine whether the image is sized well or not across other viewport sizes, it does give an idea of how the images are targeted to the particular environment that was tested.

In addition to detecting images that are more than twice as large as displayed, this PR offers a WebPageTest Pro experiment that will resize these images to the width they will be displayed. Such an experiment can be useful in determining the benefits of implementing responsive image markup and serving multiple image sizes.

In the PR, the particular image service URL set via settings. We'll need to investigate the ideal API and whether or not we use an internal one or a third party service.

…nt to swap them with images that are sized for display, as a prediction of how implementing responsive images would impact results. #1986
Copy link
Contributor

@stoyan stoyan left a comment

Choose a reason for hiding this comment

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

Looks great, few nits inline.

@@ -0,0 +1,34 @@
var imgsTooLarge = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say switch to let/const for new code

@@ -0,0 +1,34 @@
var imgsTooLarge = [];
var allImgs = document.querySelectorAll("img");
var widthTolerance = 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth checking height too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I don't think so, but open to ideas? I was thinking one dimension is enough to know if an image is displayed smaller than its intrinsic size.

// if display width is less than 50% of the full image natural size
if (
isVisible &&
boundingClientRect.width < allImgs[i].naturalWidth * widthTolerance
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to catch svgs too, right? Where naturalWidth is way higher but that's probably ok.

Probably not worth special-casing SVG since the goal is to experiment

//display none isn't enough (only catches if that image is display non, not parent)
//so we check offsetParent, but also position fixed (in everyone but ff, position fix makes offsetParent null)
let isVisible =
window.getComputedStyle(allImgs[i]).display != "none" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer !== and ===

@@ -0,0 +1,34 @@
var imgsTooLarge = [];
var allImgs = document.querySelectorAll("img");
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be cool to inspect background images too, in a follow up diff maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, with bg size I guess? harder to parse but I'm interested

www/experiments/imgs-wasteful.inc Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

2 participants