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

Jump to bottom

core: collect fetchpriority for images and link rel preload #14925

Merged
merged 4 commits into from
Mar 23, 2023
Merged

core: collect fetchpriority for images and link rel preload #14925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f17829
core: collect fetchpriority for images and link rel preload
brendankenny Mar 23, 2023
fc90a5c
update sample ImageElements
brendankenny Mar 23, 2023
2136897
update sample LinkElements
brendankenny Mar 23, 2023
903e455
add fetchPriority DOM types
brendankenny Mar 23, 2023
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
3 changes: 1 addition & 2 deletions cli/test/fixtures/dobetterweb/dbw_tester.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,7 @@ <h2 id="toppy" style="background-image:url('');">Do better web tester page</h2>
<!-- FAIL(image-aspect-ratio): image is naturally 1024x680 -->
<img src="lighthouse-1024x680.jpg?iar1" width="120" height="15">
<!-- PASS(image-aspect-ratio) -->
<!-- FAIL(lcp-lazy-loaded) -->
Copy link
Member Author

Choose a reason for hiding this comment

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

old comment; this isn't the LCP and doesn't fail that audit :)

<img loading="lazy" src="lighthouse-1024x680.jpg?iar2" width="120" height="80">
<img loading="lazy" src="lighthouse-1024x680.jpg?iar2" width="120" height="80" fetchpriority="low">

<!-- FAIL(image-size-responsive): image is naturally 480x318 -->
<img src="lighthouse-480x318.jpg?isr1" width="400" height="360" style="position: absolute;">
Expand Down
2 changes: 1 addition & 1 deletion cli/test/fixtures/preload.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<link rel="stylesheet" href="/perf/preload_style.css" />
<!-- WARN(preload): This should be no crossorigin -->
<link rel="preload" href="/perf/level-2.js?warning&delay=500" as="script" crossorigin="use-credentials"/>
<link rel="preload" href="/perf/level-2.js?warning&delay=500" as="script" crossorigin="use-credentials" fetchpriority="high" />
<!-- WARN(preconnect): This should be no crossorigin -->
<link rel="preconnect" href="http://localhost:10503" crossorigin="anonymous" />
<!-- PASS(preconnect): This uses crossorigin correctly -->
Expand Down
19 changes: 19 additions & 0 deletions cli/test/smokehouse/test-definitions/dobetterweb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,25 @@ const expectations = {
},
],
},
ImageElements: {
_includes: [{
src: 'http://localhost:10200/dobetterweb/lighthouse-1024x680.jpg?iar2',
srcset: '',
displayedWidth: 120,
displayedHeight: 80,
attributeWidth: '120',
attributeHeight: '80',
naturalDimensions: {
width: 1024,
height: 678,
Copy link
Member Author

Choose a reason for hiding this comment

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

notice the image is called lighthouse-1024x680.jpg... :P

Copy link
Member

Choose a reason for hiding this comment

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

So the file name is just misleading?

Copy link
Member Author

@brendankenny brendankenny Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So the file name is just misleading?

yes, but to be fair, it's copied from the byte-efficiency smoke test image of the same name, which is (we're now learning) also actually 1024x678, added wayyyy back in #1635

Copy link
Member

Choose a reason for hiding this comment

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

Yeah not suggesting we change it here, just wanted to confirm

},
isCss: false,
isPicture: false,
isInShadowDOM: false,
loading: 'lazy',
fetchPriority: 'low',
}],
},
},
lhr: {
requestedUrl: 'http://localhost:10200/dobetterweb/dbw_tester.html',
Expand Down
14 changes: 14 additions & 0 deletions cli/test/smokehouse/test-definitions/perf-preload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,20 @@ const config = {
* Expected Lighthouse audit values for preload tests.
*/
const expectations = {
artifacts: {
LinkElements: {
_includes: [{
rel: 'preload',
href: 'http://localhost:10200/perf/level-2.js?warning&delay=500',
hrefRaw: '/perf/level-2.js?warning&delay=500',
hreflang: '',
as: 'script',
crossOrigin: 'use-credentials',
source: 'head',
fetchPriority: 'high',
}],
},
},
networkRequests: {
// DevTools loads the page three times, so this request count will not be accurate.
_excludeRunner: 'devtools',
Expand Down
1 change: 1 addition & 0 deletions core/gather/gatherers/image-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ function getHTMLImages(allElements) {
isPicture,
loading: element.loading,
isInShadowDOM: element.getRootNode() instanceof ShadowRoot,
fetchPriority: element.fetchPriority,
// @ts-expect-error - getNodeDetails put into scope via stringification
node: getNodeDetails(element),
};
Expand Down
2 changes: 2 additions & 0 deletions core/gather/gatherers/link-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ function getLinkElementsInDOM() {
crossOrigin: link.crossOrigin,
hrefRaw,
source,
fetchPriority: link.fetchPriority,
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add an internal type declaration for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible to add an internal type declaration for this?

Yeah, it felt like overkill because it's part of the html spec and so it will eventually get rolled up into the tsc types, but OTOH @ts-expect-error is pretty heavy handed. Added to the DOM types in the somewhat misleadingly named node.d.ts

// @ts-expect-error - put into scope via stringification
node: getNodeDetails(link),
});
Expand Down Expand Up @@ -134,6 +135,7 @@ class LinkElements extends FRGatherer {
as: link.as || '',
crossOrigin: getCrossoriginFromHeader(link.crossorigin),
source: 'headers',
fetchPriority: link.fetchpriority,
node: null,
});
}
Expand Down
20 changes: 19 additions & 1 deletion core/test/results/artifacts/artifacts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9454,6 +9454,7 @@
"snippet": "<img src=\"lighthouse-1024x680.jpg?iar1\" width=\"120\" height=\"15\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": "120px",
"height": "15px",
Expand Down Expand Up @@ -9498,9 +9499,10 @@
"width": 120,
"height": 80
},
"snippet": "<img loading=\"lazy\" src=\"lighthouse-1024x680.jpg?iar2\" width=\"120\" height=\"80\">",
"snippet": "<img loading=\"lazy\" src=\"lighthouse-1024x680.jpg?iar2\" width=\"120\" height=\"80\" fetchpriority=\"low\">",
"nodeLabel": "body > img"
},
"fetchPriority": "low",
"cssEffectiveRules": {
"width": "120px",
"height": "80px",
Expand Down Expand Up @@ -9548,6 +9550,7 @@
"snippet": "<img src=\"lighthouse-1024x680.jpg?isr2\" width=\"120\" height=\"80\" style=\"position: absolute;\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": "120px",
"height": "80px",
Expand Down Expand Up @@ -9595,6 +9598,7 @@
"snippet": "<img src=\"lighthouse-1024x680.jpg?isr3\" width=\"360\" height=\"240\" style=\"image-rendering: pixelated; position: absolute;\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": "360px",
"height": "240px",
Expand Down Expand Up @@ -9642,6 +9646,7 @@
"snippet": "<img src=\"lighthouse-rotating.gif\" width=\"811\" height=\"462\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": "811px",
"height": "462px",
Expand Down Expand Up @@ -9689,6 +9694,7 @@
"snippet": "<img src=\"lighthouse-480x318.jpg?isr1\" width=\"400\" height=\"360\" style=\"position: absolute;\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": "400px",
"height": "360px",
Expand Down Expand Up @@ -9732,6 +9738,7 @@
"snippet": "<img src=\"http://localhost:10200/dobetterweb/lighthouse-1024x680.jpg\" srcset=\"lighthouse-1024x680.jpg 2x\" width=\"360\" height=\"240\" style=\"position: absolute;\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": "360px",
"height": "240px",
Expand Down Expand Up @@ -9820,6 +9827,7 @@
"snippet": "<img src=\"blob:http://localhost:10200/8081f274-a0c6-440f-a9cc-be826a0e01d5\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": null,
"height": null,
Expand Down Expand Up @@ -9867,6 +9875,7 @@
"snippet": "<img src=\"filesystem:http://localhost:10200/temporary/empty-0.30045073591260474.png\">",
"nodeLabel": "body > img"
},
"fetchPriority": "auto",
"cssEffectiveRules": {
"width": null,
"height": null,
Expand All @@ -9883,6 +9892,7 @@
"crossOrigin": null,
"hrefRaw": "./dbw_tester.css?delay=100",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-11-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,9,LINK",
Expand All @@ -9907,6 +9917,7 @@
"crossOrigin": null,
"hrefRaw": "./unknown404.css?delay=200",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-12-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,10,LINK",
Expand All @@ -9931,6 +9942,7 @@
"crossOrigin": null,
"hrefRaw": "./dbw_tester.css?delay=2200",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-13-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,12,LINK",
Expand All @@ -9955,6 +9967,7 @@
"crossOrigin": null,
"hrefRaw": "./dbw_disabled.css?delay=200&isdisabled",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-14-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,14,LINK",
Expand All @@ -9979,6 +9992,7 @@
"crossOrigin": null,
"hrefRaw": "./dbw_tester.css?delay=3000&capped",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-15-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,17,LINK",
Expand All @@ -10003,6 +10017,7 @@
"crossOrigin": null,
"hrefRaw": "./dbw_tester.css?delay=2000&async=true",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-16-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,19,LINK",
Expand All @@ -10027,6 +10042,7 @@
"crossOrigin": null,
"hrefRaw": "./dbw_tester.css?delay=3000&async=true",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-17-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,21,LINK",
Expand All @@ -10051,6 +10067,7 @@
"crossOrigin": null,
"hrefRaw": "./empty.css",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-18-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,23,LINK",
Expand All @@ -10075,6 +10092,7 @@
"crossOrigin": null,
"hrefRaw": "./dbw_tester.css?scriptActivated&delay=200",
"source": "head",
"fetchPriority": "auto",
"node": {
"lhId": "5-19-LINK",
"devtoolsNodePath": "3,HTML,0,HEAD,40,LINK",
Expand Down
4 changes: 4 additions & 0 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,8 @@ declare module Artifacts {
/** Where the link was found, either in the DOM or in the headers of the main document */
source: 'head'|'body'|'headers'
node: NodeDetails | null
/** The fetch priority hint for preload links. */
fetchPriority?: string;
}

interface Script extends Omit<Crdp.Debugger.ScriptParsedEvent, 'url'|'embedderName'> {
Expand Down Expand Up @@ -534,6 +536,8 @@ declare module Artifacts {
node: NodeDetails;
/** The loading attribute of the image. */
loading?: string;
/** The fetch priority hint for HTMLImageElements. */
fetchPriority?: string;
}

interface OptimizedImage {
Expand Down
17 changes: 15 additions & 2 deletions types/internal/node.d.ts
View file
Open in desktop
Copy link

Choose a reason for hiding this comment

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

Love 🔥

Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,22 @@ declare global {

/** Injected into the page when the `--debug` flag is used. */
continueLighthouseRun(): void;
}

// Not defined in tsc yet: https://github.com/microsoft/TypeScript/issues/40807
requestIdleCallback(callback: (deadline: {didTimeout: boolean, timeRemaining: () => DOMHighResTimeStamp}) => void, options?: {timeout: number}): number;
// `fetchPriority` not defined in tsc as of 4.9.4.
interface HTMLImageElement {
/**
* Sets the priority for fetches initiated by the element.
* @see https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-fetchpriority
*/
fetchPriority: string;
}
interface HTMLLinkElement {
/**
* Sets the priority for fetches initiated by the element.
* @see https://html.spec.whatwg.org/multipage/semantics.html#dom-link-fetchpriority
*/
fetchPriority: string;
}
}

Expand Down