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

feat: add OffscreenImages Audit #1807

Merged

brendankenny merged 15 commits into master from unused_image_audit

Mar 23, 2017

Collaborator

patrickhulce commented Mar 3, 2017

Continues the byte efficiency audits by finding unused images (images that are mostly or entirely outside the viewport). Broken into 2 commits, 1 that renames ContentWidth to ViewportDimensions to accommodate the addition of height and the other that adds the audit with tests.

Partially addresses above the fold content issue for PSI #909

patrickhulce added 2 commits

March 2, 2017 16:54


          refator: rename ContentWidth to ViewportDimensions

938feb3


          feat: add UnusedImages audit

fe5470d

patrickhulce force-pushed the unused_image_audit branch from 76f0e89 to fe5470d Compare

March 3, 2017 00:59

ebidel suggested changes

View reviewed changes

lighthouse-core/audits/byte-efficiency/unused-images.js Outdated

+                    category: 'Images',
+                    name: 'unused-images',
+                    description: 'Unused images',
+                    helpText: 'Images that are not above the fold should be lazy loaded on interaction',

Contributor

ebidel Mar 3, 2017

learn more link to https://developers.google.com/web/updates/2016/04/intersectionobserver?

Collaborator Author

patrickhulce Mar 3, 2017

done

lighthouse-core/audits/byte-efficiency/unused-images.js Outdated


		const IGNORE_THRESHOLD_IN_BYTES = 2048;

		class UnusedImages extends Audit {

Contributor

ebidel Mar 3, 2017

would "OffscreenImages" make more sense? Or "AboveTheFoldImages"

Collaborator Author

patrickhulce Mar 3, 2017

yeah OffscreenImages sounds good

lighthouse-core/audits/byte-efficiency/unused-images.js Outdated

+                }
+                /**
+                 * @param {{top: number, right: number, bottom: number, left: number}} imageRect

Contributor

ebidel Mar 3, 2017

nit: {ClientRect}

Collaborator Author

patrickhulce Mar 3, 2017

will do

lighthouse-core/audits/byte-efficiency/unused-images.js Outdated

+                 * @return {number}
+                 */
+                static computeVisiblePixels(imageRect, viewportDimensions) {
+                  const scrollWidth = viewportDimensions.scrollWidth;

Contributor

ebidel Mar 3, 2017

Not clientWidth/Height of the viewport? scrollWidth/Height includes off screen content.

Collaborator Author

patrickhulce Mar 3, 2017

The existing naming of scroll and viewport for inner and outer somewhat confused me so I'm up for renaming to just inner and outer, but I think I do want inner* (which is scroll here), right? Everything else gives me the size of the screen not what content is being displayed.

lighthouse-core/audits/byte-efficiency/unused-images.js Outdated

+                  const url = URL.getDisplayName(image.src, {preserveQuery: true});
+                  const totalPixels = image.clientWidth * image.clientHeight;
+                  const visiblePixels = this.computeVisiblePixels(image.clientRect, viewportDimensions);
+                  const wastedRatio = 1 - visiblePixels / totalPixels;

Contributor

ebidel Mar 3, 2017

One thought about this audit is that it would be really nice to use IntersectionObserver for finding the visible ratio of the image to the viewport. That API was designed for this use case. One problem with that approach is that the observer only fires if the image clientrect intersects with the viewports. So, for offscreen images, you'd never get a callback.

Collaborator Author

patrickhulce Mar 3, 2017

True, I open to one that would do set subtraction but that seemed too specific to this audit to put in image usage and felt weird breaking out into a new one. It also meant the settings for failure (margin and so forth) were located in the gatherer rather than with the audit.

We have a spectrum of this throughout LH though, so I'm curious what other opinions are. I'm generally more in favor of keeping gatherers purely informational/more logic-less and having failure conditions measured in audits.

Member

paulirish commented Mar 3, 2017

A different approach would be to use the trace and see what images we decoded. If we didn't decode an image yet, we probably didn't need to download it.

This would be slightly more resilient in case images move location during load.

Also, did you look at #821 ? How does this approach differ?

Collaborator Author

patrickhulce commented Mar 3, 2017

Also, did you look at #821 ? How does this approach differ?

Similar overall approach, mostly just updated to leverage the byte efficiency framework and image gatherer that's been added since then. Also adjustments to use innerHeight/Width instead.


          style: rename to OffscreenImages

b80470c

patrickhulce changed the title ~~feat: add UnusedImage Audit~~ feat: add OffscreenImages Audit

ebidel reviewed

View reviewed changes

lighthouse-core/gather/gatherers/viewport-dimensions.js Outdated

		scrollWidth: window.innerWidth,
		scrollHeight: window.innerHeight,

Contributor

ebidel Mar 8, 2017 •

edited

Loading

Ah I see these now. Yea, I would switch these to use the same name as the property you're stashing. e.g. innerHeight: window.innerHeight.

I typically use document.documentElement.clientWidth to get the viewport width, but inner* works great. That's what you want.

patrickhulce added 2 commits

March 7, 2017 16:44


          update viewport dimensions naming

7c2b75f


          Merge branch 'master' into unused_image_audit

4c76bb1

ebidel reviewed

View reviewed changes

lighthouse-core/audits/content-width.js Outdated

@@ @@ -40,8 +40,8 @@ class ContentWidth extends Audit { @@
                  * @return {!AuditResult}
                  */
                 static audit(artifacts) {
-                  const scrollWidth = artifacts.ViewportDimensions.scrollWidth;
-                  const viewportWidth = artifacts.ViewportDimensions.viewportWidth;
+                  const scrollWidth = artifacts.ViewportDimensions.innerWidth;

Contributor

ebidel Mar 8, 2017

rename var to innerWidth too?

lighthouse-core/audits/content-width.js Outdated

    
                  return 'The content scroll size is ' + artifact.scrollWidth + 'px, ' +

                      'whereas the viewport size is ' + artifact.viewportWidth + 'px.';

                  return 'The content scroll size is ' + artifact.innerWidth + 'px, ' +

                      'whereas the viewport size is ' + artifact.outerWidth + 'px.';

Contributor

ebidel Mar 8, 2017 •

edited

Loading

Think this sentence needs updating. The first var is the viewport width and the second is
the size of the browser window. Right? Not sure you want outerWidth/Height. Isn't that inclusive of the browser chrome? https://developer.mozilla.org/en-US/docs/Web/API/Window/outerWidth

For "content scroll size", don't you want scrollWidth of the content?

Contributor

ebidel Mar 8, 2017

I'm on a plane so sorry for the short replies.

Collaborator Author

patrickhulce Mar 8, 2017 •

edited

Loading

No worries, and I was just going with the "if it ain't broke don't fix it" approach, but I agree ~~documentElement.scrollWidth !== documentElement.clientWidth makes the intention a lot more clear~~ I don't know what the intention is anymore :). In practice, outerWidth seems to reflect the non-scaled true width of the viewport and innerWidth seems to reflect the width of the content shown

Contributor

ebidel Mar 8, 2017

Sorry. I'm still caught up in lingo and intended behavior. Does http://jsbin.com/murusonore/edit?html,console,output capture what you're after?

document.documentElement.scrollWidth == content scroll size
window.innerWidth == visible viewport size

(In the bin example, both of these metrics are < window.outerWidth because it's running inside the iframe...which is smaller than the viewport.)

Other refs:

innerWidth: Width (in pixels) of the browser window viewport including, if rendered, the vertical scrollbar.

https://developer.mozilla.org/en-US/docs/Web/API/Window/innerWidth

outerWidth gets the width of the outside of the browser window. It represents the width of the whole browser window including sidebar (if expanded), window chrome and window resizing borders/handles.

https://developer.mozilla.org/en-US/docs/Web/API/Window/outerWidth

Collaborator Author

patrickhulce Mar 8, 2017

I'm not after anything other than to make the audit have more accurate names than it had before the rename :) I'm honestly not sure what this audit is getting after since we already have the viewport audit checking for the most obvious condition under which this one will fail.

Proposal: rename scrollWidth variable to viewportWidth, rename viewportWidth to windowWidth and update the description accordingly. Do not change comparison of innerWidth to outerWidth.

Aside, my previous example of scrollWidth != clientWidth is actually non-sensical I mistakenly assumed they meant something different on documentElement than regular elements.

Contributor

ebidel Mar 8, 2017 •

edited

Loading

SGTM on those renames.

Collaborator Author

patrickhulce Mar 8, 2017

Ok talked to Paul and found the motivation of this audit and it's just actually measuring something totally different. I'll clean that up in a future PR :) Will rename for now.


          update variable names

5145ce6

ebidel approved these changes

View reviewed changes

Contributor

ebidel left a comment

LGTM. Who knows what the audit does, but at least the output text is matching what the code is doing :)

Collaborator Author

patrickhulce commented Mar 8, 2017

Who knows what the audit does, but at least the output text is matching what the code is doing

lol, I filed #1836 and linked it in your PSI meta issue

brendankenny requested changes

View reviewed changes

Member

brendankenny left a comment

left two nits

More conceptual issues:

does it make sense to punish for images that are partly on screen? Slicing them would not be a maintainable solution for developers, so I'm not sure how actionable the advice for those images will be
"Images that are not above the fold should be lazy loaded on interaction" - should they? What's the downside vs e.g. loading below the fold images after TTI?

lighthouse-core/gather/gatherers/viewport-dimensions.js Outdated

                   .then(returnedValue => {
-                    if (!Number.isFinite(returnedValue.scrollWidth) ||
-                        !Number.isFinite(returnedValue.viewportWidth) ||
+                    if (!Number.isFinite(returnedValue.innerWidth) ||

Member

brendankenny Mar 9, 2017

time for something more succinct here? No Object.values yet, but could do like

const allNumeric = Object.keys(returnedValue).every(key => Number.isFinite(returnedValue[key]));
if (!allNumeric) {
  // ...
}

?

Collaborator Author

patrickhulce Mar 9, 2017

done

lighthouse-core/audits/byte-efficiency/offscreen-images.js Outdated

+                      return results;
+                    }
+                    // Don't warn about an image that was also used appropriately

Member

brendankenny Mar 9, 2017

what about something like, "if an image was used more than once, warn about the least wasteful usage of it"

Collaborator Author

patrickhulce Mar 9, 2017

done

Collaborator Author

patrickhulce commented Mar 9, 2017 •

edited

Loading

does it make sense to punish for images that are partly on screen? Slicing them would not be a maintainable solution for developers, so I'm not sure how actionable the advice for those images will be

That's mostly what the extra margin of error is for, but I'm down for only notifying only if a minimum percent offscreen is reached, maybe 75+?

"Images that are not above the fold should be lazy loaded on interaction" - should they? What's the downside vs e.g. loading below the fold images after TTI?

Will reword, and we should probably check to see if that's already the case...

patrickhulce added 4 commits

March 9, 2017 09:49


          update tests, comments

f350914


          feat(trace-categories): add support for custom trace categories

a8016ab


          fix merge conflicts

c4e35a5


          description change

9bf51c8

patrickhulce mentioned this pull request

test(byte-efficiency): added tests to base byte efficiency audit #1865

Merged

patrickhulce added 2 commits

March 17, 2017 22:05


          Merge branch 'master' into unused_image_audit

54bfb23


          ignore lazily loaded images

26a3cff

Contributor

ebidel commented Mar 22, 2017

@patrickhulce this needs a rebase.

ebidel assigned brendankenny

patrickhulce added 2 commits

March 22, 2017 11:47


          Merge branch 'master' into unused_image_audit

c53b43b


          Merge branch 'master' into unused_image_audit

5a43312

brendankenny approved these changes

View reviewed changes

Member

brendankenny left a comment

👻🖼📐

looks good, only a few style nits (mostly around clearly marking units :)

lighthouse-core/audits/byte-efficiency/offscreen-images.js

+              'use strict';
+              const Audit = require('./byte-efficiency-audit');
+              const TTIAudit = require('../time-to-interactive');

Member

brendankenny Mar 22, 2017

using audits in audits, my new arch nemesis :P

Collaborator Author

patrickhulce Mar 23, 2017

i'm with ya, @paulirish not a fan of getting these values into computed artifact though 😩

lighthouse-core/audits/byte-efficiency/offscreen-images.js Outdated

+                  }, new Map());
+                  return TTIAudit.audit(artifacts).then(ttiResult => {
+                    const tti = ttiResult.extendedInfo.value.timestamps.timeToInteractive / 1000000;

Member

brendankenny Mar 23, 2017

ttiInSeconds or something like that? Any reason you aren't just using ttiResult.rawValue?

Collaborator Author

patrickhulce Mar 23, 2017

raw value isn't what I want, I need the actual devtools timestamp value

lighthouse-core/audits/byte-efficiency/offscreen-images.js Outdated

+                    const results = Array.from(resultsMap.values()).filter(item => {
+                      const isWasteful = item.wastedBytes > IGNORE_THRESHOLD_IN_BYTES &&
+                          item.wastedPercent > IGNORE_THRESHOLD_IN_PERCENT;
+                      const loadedEarly = item.requestStartTime < tti;

Member

brendankenny Mar 23, 2017

should add an explicit comment here (or in description or on devsite? we'll have to doc it somewhere) that offscreen images loaded after TTI are considered fine

Collaborator Author

patrickhulce Mar 23, 2017

done

lighthouse-core/test/audits/byte-efficiency/offscreen-images-test.js Outdated

+              describe('OffscreenImages audit', () => {
+                const _ttiAuditOriginal = TTIAudit.audit;
+                before(() => {
+                  const timeToInteractive = 2000000;

Member

brendankenny Mar 23, 2017

timeToInteractiveInμs? :) took me a second and some looking at the TTI audit itself to realize this wasn't just chosen to be a really high TTI so it would be longer than any trace we normally take

Collaborator Author

patrickhulce Mar 23, 2017

done

lighthouse-core/test/audits/byte-efficiency/offscreen-images-test.js Outdated

+              function generateRecord(resourceSizeInKb, startTime = 0, mimeType = 'image/png') {
+                return {
+                  mimeType,
+                  startTime,

Member

brendankenny Mar 23, 2017

mention this is in seconds?

Collaborator Author

patrickhulce Mar 23, 2017

done

lighthouse-core/test/audits/byte-efficiency/offscreen-images-test.js Outdated

+                  });
+                });
+                it('disregards lazily loaded images', () => {

Member

brendankenny Mar 23, 2017

maybe explicit 'disregards images loaded after TTI'?

Collaborator Author

patrickhulce Mar 23, 2017

done

Member

brendankenny commented Mar 23, 2017

Hmm, do we need to bring in something to slow down the byte efficiency tester page? Or induce more screenshots with a spinner? Now that TTI is needed for it (through the offscreen images audit) it looks like that page is flakey now when no screenshots are captured.

✘ difference at offscreen-images.score: found null, expected false
full found result: {
  "score": null,
  "rawValue": null,
  "error": true,
  "debugString": "Audit error: No screenshots found in trace",
  "name": "offscreen-images",
  "description": "Offscreen images"
}

Collaborator Author

patrickhulce commented Mar 23, 2017 •

edited

Loading

Hmm, do we need to bring in something to slow down the byte efficiency tester page? Or induce more screenshots with a spinner?

It's already being slowed to 5s as part of this PR and not sure why it's flaking on travis, would love to get a reproducible failure case to have more ammo against that bug though. 6/6 smoke test successes locally as I wrote this

UPDATE: now 12/12 :)


          more nits

d8eb491

Member

brendankenny commented Mar 23, 2017

ok, so travis is happy on a few reruns, so lets ship it. Not sure how to restart the appveyor builds

brendankenny merged commit 902585b into master

brendankenny deleted the unused_image_audit branch

March 23, 2017 18:59

paulirish mentioned this pull request

ContentWidth audit doesn't measure content width #1836

Open

ebidel mentioned this pull request

Audits Meta: Feature parity with PSI, MFT, DevTools #909

Closed

16 tasks

paulirish mentioned this pull request

[Proposal] Report downloaded but offscreen images #821

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment