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(viewport): include meta viewport string in debugDetails #15727

Merged
merged 2 commits into from
Jan 4, 2024
Merged

core(viewport): include meta viewport string in debugDetails #15727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2dffa1
core(viewport): log raw viewport content in debugDetails
brendankenny Jan 4, 2024
2ed1eae
make dbw use a dynamically-set mobile viewport
brendankenny Jan 4, 2024
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
8 changes: 7 additions & 1 deletion cli/test/fixtures/dobetterweb/dbw_tester.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
<head>
<title>DoBetterWeb Mega Tester... Of Death</title>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<meta name="viewport" content="width=2899">
<meta property="og:description" content="Open Graph smoke test description">

<template id="links-blocking-first-paint-tmpl">
Expand Down Expand Up @@ -48,6 +48,12 @@
<!-- Malformed links should not show in TagsBlockingFirstPaint artifact -->
<link rel="stylesheet" href="">

<script>
// Set a mobile-friendly viewport only on load.
const metaViewport = document.querySelector('meta[name="viewport"]');
metaViewport.content = 'width=device-width, initial-scale=1, minimum-scale=1';
Comment on lines +52 to +54
Copy link
Member Author

Choose a reason for hiding this comment

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

It's really important that we take the viewport content from the element (not parsed from the tag in the html) because site can and do change it.

We already do this, but we don't currently have any test coverage for that fact :)

It probably makes sense for the terrible dbw page to have a weird viewport instead of a nice one, but doing so changes test expectations for way too many assertions, so it isn't worth the effort.

</script>

<!-- FAIL: block rendering -->
<script src="./dbw_tester.js" id="dbw-tester-script"></script>

Expand Down
6 changes: 6 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 @@ -615,6 +615,12 @@ const expectations = {
],
},
},
'viewport': {
score: 1,
details: {
viewportContent: 'width=device-width, initial-scale=1, minimum-scale=1',
},
},
},
fullPageScreenshot: {
screenshot: {
Expand Down
4 changes: 4 additions & 0 deletions cli/test/smokehouse/test-definitions/seo-failing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ const expectations = {
audits: {
'viewport': {
score: 0,
details: {
type: 'debugdata',
viewportContent: 'invalid-content=should_have_looked_it_up',
},
},
'document-title': {
score: 0,
Expand Down
10 changes: 10 additions & 0 deletions core/audits/viewport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,22 @@ class Viewport extends Audit {
inpSavings = 0;
}

/** @type {LH.Audit.Details.DebugData|undefined} */
let details;
if (viewportMeta.rawContentString !== undefined) {
details = {
type: 'debugdata',
viewportContent: viewportMeta.rawContentString,
};
}

return {
score: Number(viewportMeta.isMobileOptimized),
metricSavings: {
INP: inpSavings,
},
warnings: viewportMeta.parserWarnings,
details,
};
}
}
Expand Down
7 changes: 6 additions & 1 deletion core/computed/viewport-meta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@
hasViewportTag: false,
isMobileOptimized: false,
parserWarnings: [],
rawContentString: undefined,
};
}

const warnings = [];
const parsedProps = Parser.parseMetaViewPortContent(viewportMeta.content || '');
const rawContentString = viewportMeta.content || '';
const parsedProps = Parser.parseMetaViewPortContent(rawContentString);

if (Object.keys(parsedProps.unknownProperties).length) {
warnings.push(`Invalid properties found: ${JSON.stringify(parsedProps.unknownProperties)}`);
Expand All @@ -42,6 +44,7 @@
hasViewportTag: true,
isMobileOptimized: false,
parserWarnings: warnings,
rawContentString,

Check warning on line 47 in core/computed/viewport-meta.js

View check run for this annotation

Codecov / codecov/patch

core/computed/viewport-meta.js#L47 

Added line #L47 was not covered by tests
};
}

Expand All @@ -51,6 +54,7 @@
hasViewportTag: true,
isMobileOptimized,
parserWarnings: warnings,
rawContentString,
};
}
}
Expand All @@ -63,4 +67,5 @@
* @property {boolean} hasViewportTag Whether the page has any viewport tag.
* @property {boolean} isMobileOptimized Whether the viewport tag is optimized for mobile screens.
* @property {Array<string>} parserWarnings Warnings if the parser encountered invalid content in the viewport tag.
* @property {string|undefined} rawContentString The `content` attribute value, if a viewport was defined.
*/
31 changes: 20 additions & 11 deletions core/test/computed/viewport-meta-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,44 +20,49 @@ describe('ViewportMeta computed artifact', () => {
/* eslint-disable-next-line max-len */
it('is not mobile optimized when HTML contains a non-mobile friendly viewport meta tag', async () => {
const viewport = 'maximum-scale=1';
const {hasViewportTag, isMobileOptimized} =
const {hasViewportTag, isMobileOptimized, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(hasViewportTag, true);
assert.equal(isMobileOptimized, false);
assert.equal(rawContentString, viewport);
});

it('is not mobile optimized when HTML contains an invalid viewport meta tag key', async () => {
const viewport = 'nonsense=true';
const {hasViewportTag, isMobileOptimized} =
const {hasViewportTag, isMobileOptimized, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(hasViewportTag, true);
assert.equal(isMobileOptimized, false);
assert.equal(rawContentString, viewport);
});

it('is not mobile optimized when HTML contains an invalid viewport meta tag value', async () => {
const viewport = 'initial-scale=microscopic';
const {isMobileOptimized, parserWarnings} =
const {isMobileOptimized, parserWarnings, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, false);
assert.equal(rawContentString, viewport);
assert.equal(parserWarnings[0], 'Invalid values found: {"initial-scale":"microscopic"}');
});

/* eslint-disable-next-line max-len */
it('is not mobile optimized when HTML contains an invalid viewport meta tag key and value', async () => {
const viewport = 'nonsense=true, initial-scale=microscopic';
const {isMobileOptimized, parserWarnings} =
const {isMobileOptimized, parserWarnings, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, false);
assert.equal(rawContentString, viewport);
assert.equal(parserWarnings[0], 'Invalid properties found: {"nonsense":"true"}');
assert.equal(parserWarnings[1], 'Invalid values found: {"initial-scale":"microscopic"}');
});

// eslint-disable-next-line max-len
it('is not mobile optimized when a viewport contains an initial-scale value lower than 1', async () => {
const viewport = 'width=device-width, initial-scale=0.9';
const {isMobileOptimized} =
const {isMobileOptimized, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, false);
assert.equal(rawContentString, viewport);
});

it('is mobile optimized when a valid viewport is provided', async () => {
Expand All @@ -69,16 +74,20 @@ describe('ViewportMeta computed artifact', () => {
];

await Promise.all(viewports.map(async viewport => {
const {isMobileOptimized} =
const {isMobileOptimized, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, true);
assert.equal(rawContentString, viewport);
}));
});

it('recognizes interactive-widget property', async () => {
const viewport = 'width=device-width, interactive-widget=resizes-content';
const {parserWarnings} = await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(parserWarnings[0], undefined);
Copy link
Member Author

@brendankenny brendankenny Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

driveby change of assert.equal(parserWarnings[0], undefined) to assert.equal(parserWarnings.length, 0) because the first one seems like it's asserting that there is at least one warning, but its value is undefined. Not sure if this was important at the time, but the length check seems correct now, at least (happened in #14664 @connorjclark, maybe it had to do with the metaviewport-parser release workaround or something?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I probably just copied it from the place you changed below 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

ya ancient copy pasta https://github.com/GoogleChrome/lighthouse/blame/2bc5ae284c697394f00228967fc6dc56a99f0af5/core/test/computed/viewport-meta-test.js

const {parserWarnings, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(rawContentString, viewport);
assert.equal(parserWarnings.length, 0);
assert.equal(rawContentString, viewport);
});

it('doesn\'t throw when viewport contains "invalid" iOS properties', async () => {
Expand All @@ -87,11 +96,11 @@ describe('ViewportMeta computed artifact', () => {
'width=device-width, viewport-fit=cover',
];
await Promise.all(viewports.map(async viewport => {
const {isMobileOptimized, parserWarnings} =
const {isMobileOptimized, parserWarnings, rawContentString} =
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, true);
assert.equal(parserWarnings[0], undefined);
assert.equal(parserWarnings.length, 0);
assert.equal(rawContentString, viewport);
}));
});
});

12 changes: 12 additions & 0 deletions core/test/fixtures/user-flows/reports/sample-flow-result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
"metricSavings": {
"INP": 0
},
"details": {
"type": "debugdata",
"viewportContent": "width=device-width"
},
"guidanceLevel": 3
},
"first-contentful-paint": {
Expand Down Expand Up @@ -12757,6 +12761,10 @@
"metricSavings": {
"INP": 0
},
"details": {
"type": "debugdata",
"viewportContent": "width=device-width"
},
"guidanceLevel": 3
},
"image-aspect-ratio": {
Expand Down Expand Up @@ -17994,6 +18002,10 @@
"metricSavings": {
"INP": 0
},
"details": {
"type": "debugdata",
"viewportContent": "width=device-width"
},
"guidanceLevel": 3
},
"first-contentful-paint": {
Expand Down
4 changes: 4 additions & 0 deletions core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
"metricSavings": {
"INP": 0
},
"details": {
"type": "debugdata",
"viewportContent": "width=device-width, initial-scale=1, minimum-scale=1"
},
"guidanceLevel": 3
},
"first-contentful-paint": {
Expand Down
Loading