-
Notifications
You must be signed in to change notification settings - Fork 714
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
base: master
Are you sure you want to change the base?
Conversation
…nt to swap them with images that are sized for display, as a prediction of how implementing responsive images would impact results. #1986
There was a problem hiding this 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 = []; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" && |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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.