Skip to content

core(link-elements): ignore non-HTMLLinkElements #9765

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
brendankenny merged 3 commits into from
Oct 3, 2019
Merged

core(link-elements): ignore non-HTMLLinkElements #9765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9cd867
core(link-elements): ignore non-HTMLLinkElements
patrickhulce Oct 1, 2019
c86c67b
add failing smoketest
patrickhulce Oct 1, 2019
7ad0149
fixes
patrickhulce Oct 2, 2019
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
7 changes: 7 additions & 0 deletions lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,13 @@
<div>
<h2>Do better web tester page</h2>
<span>Hi there!</span>

<!-- Invalid SVG with a link just to ensure the `LinkElements` gatherer can handle it. -->
<svg role="img" class="social-facebook" aria-labelledby="social-facebook-5">
<title id="social-facebook-5">Facebook</title>
<link rel="preload" href="https://fonts.googleapis.com/made-up-url"></link>
<use xlink:href=""></use>
</svg>
</div>

<section id="touchmove-section">touchmove section</section>
Expand Down
1 change: 1 addition & 0 deletions lighthouse-cli/test/smokehouse/dbw-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module.exports = {
'render-blocking-resources',
'errors-in-console',
'efficient-animated-content',
'apple-touch-icon', // pull in apple touch icon to test `LinkElements`
Copy link
Contributor

@brendankenny brendankenny Oct 2, 2019

Choose a reason for hiding this comment

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

any reason to do this in dbw and not in perf or seo where LinkElements are already being used?

Copy link
Collaborator Author

@patrickhulce patrickhulce Oct 2, 2019

Choose a reason for hiding this comment

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

just that dbw felt like the dumping ground of error catching appropriate for this sort of thing 😆

the others are so focused it felt mean to pollute them with SVG nonsense

],
},
};
107 changes: 104 additions & 3 deletions lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,107 @@ module.exports = [
}, {
id: 'wordpress',
}],
LinkElements: [
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=100',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=100',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
hrefRaw: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/dbw_disabled.css?delay=200&isdisabled',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_disabled.css?delay=200&isdisabled',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'import',
href: 'http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'import',
href: 'http://localhost:10200/dobetterweb/dbw_partial_b.html?delay=200&isasync',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_partial_b.html?delay=200&isasync',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true',
hreflang: '',
as: 'style',
crossOrigin: null,
source: 'head',
},
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&async=true',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&async=true',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'alternate stylesheet',
href: 'http://localhost:10200/dobetterweb/empty.css',
hrefRaw: 'http://localhost:10200/dobetterweb/empty.css',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
{
rel: 'stylesheet',
href: 'http://localhost:10200/dobetterweb/dbw_tester.css?scriptActivated&delay=200',
hrefRaw: 'http://localhost:10200/dobetterweb/dbw_tester.css?scriptActivated&delay=200',
hreflang: '',
as: '',
crossOrigin: null,
source: 'head',
},
],
TagsBlockingFirstPaint: [
{
tag: {
Expand Down Expand Up @@ -261,11 +362,11 @@ module.exports = [
},
'dom-size': {
score: 1,
numericValue: 137,
numericValue: 141,
details: {
items: [
{statistic: 'Total DOM Elements', value: '137'},
{statistic: 'Maximum DOM Depth', value: '3'},
{statistic: 'Total DOM Elements', value: '141'},
{statistic: 'Maximum DOM Depth', value: '4'},
{
statistic: 'Maximum Child Elements',
value: '100',
Expand Down
48 changes: 35 additions & 13 deletions lighthouse-core/gather/gatherers/link-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ const Gatherer = require('./gatherer.js');
const URL = require('../../lib/url-shim.js').URL;
const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js');
const LinkHeader = require('http-link-header');
const getElementsInDocumentString = require('../../lib/page-functions.js')
.getElementsInDocumentString; // eslint-disable-line max-len
const {getElementsInDocumentString} = require('../../lib/page-functions.js');

/* globals HTMLLinkElement */

/**
* @fileoverview
Expand Down Expand Up @@ -42,6 +43,36 @@ function getCrossoriginFromHeader(value) {
return null;
}

/**
* @return {LH.Artifacts['LinkElements']}
*/
/* istanbul ignore next */
function getLinkElementsInDOM() {
/** @type {Array<HTMLOrSVGElement>} */
// @ts-ignore - getElementsInDocument put into scope via stringification
const browserElements = getElementsInDocument('link'); // eslint-disable-line no-undef
/** @type {LH.Artifacts['LinkElements']} */
const linkElements = [];

for (const link of browserElements) {
// We're only interested in actual LinkElements, not `<link>` tagName elements inside SVGs.
// https://github.com/GoogleChrome/lighthouse/issues/9764
if (!(link instanceof HTMLLinkElement)) continue;

linkElements.push({
rel: link.rel,
href: link.href,
hrefRaw: link.href,
hreflang: link.hreflang,
as: link.as,
crossOrigin: link.crossOrigin,
source: link.closest('head') ? 'head' : 'body',
});
}

return linkElements;
}

class LinkElements extends Gatherer {
/**
* @param {LH.Gatherer.PassContext} passContext
Expand All @@ -52,18 +83,9 @@ class LinkElements extends Gatherer {
// the values like access from JavaScript does.
return passContext.driver.evaluateAsync(`(() => {
${getElementsInDocumentString};
${getLinkElementsInDOM};

return getElementsInDocument('link').map(link => {
return {
rel: link.rel,
href: link.href,
hrefRaw: link.href,
hreflang: link.hreflang,
as: link.as,
crossOrigin: link.crossOrigin,
source: link.closest('head') ? 'head' : 'body',
};
});
return getLinkElementsInDOM();
})()`, {useIsolation: true});
}

Expand Down
2 changes: 1 addition & 1 deletion types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ declare global {
/** The `as` attribute of the link */
as: string
/** The `crossOrigin` attribute of the link */
crossOrigin: 'anonymous'|'use-credentials'|null
crossOrigin: string | null
Copy link
Contributor

@brendankenny brendankenny Oct 3, 2019

Choose a reason for hiding this comment

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

this is a loss for documentation but I suppose there's always mdn :)

/** Where the link was found, either in the DOM or in the headers of the main document */
source: 'head'|'body'|'headers'
}
Expand Down