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.

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

new_audit(pwa): maskable icon audit #10370

Merged
merged 15 commits into from
Feb 26, 2020
Merged
68 changes: 68 additions & 0 deletions lighthouse-core/audits/maskable-icon.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
* @license Copyright 2020 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Audit = require('./audit.js');
const ManifestValues = require('../computed/manifest-values.js');
const i18n = require('../lib/i18n/i18n.js');

const UIStrings = {
/** Title of a Lighthouse audit that provides detail on if the manifest contains a maskable icon. This descriptive title is shown to users when the manifest does contains at least one maskable icon. */
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
title: 'Manifest has a maskable icon',
/** Title of a Lighthouse audit that provides detial on if the manifest contains a maskable icon. this descriptive title is shown to users when the manifest contains no icons that are maskable. */
failureTitle: 'Manifest doesn\'t have a maskable icon',
/** Description of a Lighthouse audit that tells the user why they their manifest should have at least one maskable icon. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'A maskable icon ensures that the icon fills the entire shape ' +
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
'when installing the app on a device. [Learn more](https://web.dev/maskable-icon/).',
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

/**
* @fileoverview
* Audits if a manifest contains at least one icon that is maskable
*
* Requirements:
* * manifest is not empty
* * manifest has valid icons
* * at least one of the icons has a purpose of 'maskable'
*/

class MaskableIcon extends Audit {
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'maskable-icon',
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors'],
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
const manifestValues = await ManifestValues.request(artifacts, context);
if (manifestValues.isParseFailure) {
return {
score: 0,
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
};
}
const maskableIconCheck = manifestValues.allChecks.find(i => i.id === 'hasMaskableIcon');
return {
score: (maskableIconCheck && maskableIconCheck.passing) ? 1 : 0,
};
}
}

module.exports = MaskableIcon;
module.exports.UIStrings = UIStrings;
6 changes: 6 additions & 0 deletions lighthouse-core/computed/manifest-values.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ class ManifestValues {
failureText: 'Manifest does not have `name`',
validate: manifestValue => !!manifestValue.name.value,
},
{
id: 'hasMaskableIcon',
failureText: 'Manifest does not have at least one icon that is maskable',
validate: ManifestValue => icons.doExist(ManifestValue) &&
icons.containsMaskableIcon(ManifestValue),
},
];
}

Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ const defaultConfig = {
'apple-touch-icon',
'splash-screen',
'themed-omnibox',
'maskable-icon',
'content-width',
'image-aspect-ratio',
'deprecations',
Expand Down Expand Up @@ -559,6 +560,7 @@ const defaultConfig = {
{id: 'viewport', weight: 2, group: 'pwa-optimized'},
{id: 'without-javascript', weight: 1, group: 'pwa-optimized'},
{id: 'apple-touch-icon', weight: 1, group: 'pwa-optimized'},
{id: 'maskable-icon', weight: 1, group: 'pwa-optimized'},
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
// Manual audits
{id: 'pwa-cross-browser', weight: 0},
{id: 'pwa-page-transitions', weight: 0},
Expand Down
9 changes: 9 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,15 @@
"lighthouse-core/audits/manual/pwa-page-transitions.js | title": {
"message": "Page transitions don't feel like they block on the network"
},
"lighthouse-core/audits/maskable-icon.js | description": {
"message": "A maskable icon ensures that the icon fills the entire shape when installing the app on a device. [Learn more](https://web.dev/maskable-icon/)."
},
"lighthouse-core/audits/maskable-icon.js | failureTitle": {
"message": "Manifest doesn't have a maskable icon"
},
"lighthouse-core/audits/maskable-icon.js | title": {
"message": "Manifest has a maskable icon"
},
"lighthouse-core/audits/metrics/estimated-input-latency.js | description": {
"message": "Estimated Input Latency is an estimate of how long your app takes to respond to user input, in milliseconds, during the busiest 5s window of page load. If your latency is higher than 50 ms, users may perceive your app as laggy. [Learn more](https://web.dev/estimated-input-latency)."
},
Expand Down
9 changes: 9 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-XL.json
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,15 @@
"lighthouse-core/audits/manual/pwa-page-transitions.js | title": {
"message": "P̂áĝé t̂ŕâńŝít̂íôńŝ d́ôń't̂ f́êél̂ ĺîḱê t́ĥéŷ b́l̂óĉḱ ôń t̂h́ê ńêt́ŵór̂ḱ"
},
"lighthouse-core/audits/maskable-icon.js | description": {
"message": "Â ḿâśk̂áb̂ĺê íĉón̂ én̂śûŕêś t̂h́ât́ t̂h́ê íĉón̂ f́îĺl̂ś t̂h́ê én̂t́îŕê śĥáp̂é ŵh́êń îńŝt́âĺl̂ín̂ǵ t̂h́ê áp̂ṕ ôń â d́êv́îćê. [Ĺêár̂ń m̂ór̂é](https://web.dev/maskable-icon/)."
},
"lighthouse-core/audits/maskable-icon.js | failureTitle": {
"message": "M̂án̂íf̂éŝt́ d̂óêśn̂'t́ ĥáv̂é â ḿâśk̂áb̂ĺê íĉón̂"
},
"lighthouse-core/audits/maskable-icon.js | title": {
"message": "M̂án̂íf̂éŝt́ ĥáŝ á m̂áŝḱâb́l̂é îćôń"
},
"lighthouse-core/audits/metrics/estimated-input-latency.js | description": {
"message": "Êśt̂ím̂át̂éd̂ Ín̂ṕût́ L̂át̂én̂ćŷ íŝ án̂ éŝt́îḿât́ê óf̂ h́ôẃ l̂ón̂ǵ ŷóûŕ âṕp̂ t́âḱêś t̂ó r̂éŝṕôńd̂ t́ô úŝér̂ ín̂ṕût́, îń m̂íl̂ĺîśêćôńd̂ś, d̂úr̂ín̂ǵ t̂h́ê b́ûśîéŝt́ 5ŝ ẃîńd̂óŵ óf̂ ṕâǵê ĺôád̂. Íf̂ ýôúr̂ ĺât́êńĉý îś ĥíĝh́êŕ t̂h́âń 50 m̂ś, ûśêŕŝ ḿâý p̂ér̂ćêív̂é ŷóûŕ âṕp̂ áŝ ĺâǵĝý. [L̂éâŕn̂ ḿôŕê](https://web.dev/estimated-input-latency)."
},
Expand Down
14 changes: 14 additions & 0 deletions lighthouse-core/lib/icons.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,21 @@ function pngSizedAtLeast(sizeRequirement, manifest) {
});
}

/**
* @param {NonNullable<LH.Artifacts.Manifest['value']>} manifest
* @return {boolean} Does the manifest icons value contain at least one icon with purpose equal to "maskable"
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
*/
function containsMaskableIcon(manifest) {
const iconValues = manifest.icons.value;
return iconValues.some(icon => {
return icon.value.purpose &&
icon.value.purpose.value &&
icon.value.purpose.value.includes('maskable');
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
});
}

module.exports = {
doExist,
pngSizedAtLeast,
containsMaskableIcon,
};
2 changes: 2 additions & 0 deletions lighthouse-core/lib/manifest-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ function parseIcon(raw, manifestUrl) {
}

const type = parseString(raw.type, true);
const purpose = parseString(raw.purpose, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like the default is "any" if we want to stick to the spec


const density = {
raw: raw.density,
Expand Down Expand Up @@ -278,6 +279,7 @@ function parseIcon(raw, manifestUrl) {
type,
density,
sizes,
purpose,
},
warning: undefined,
};
Expand Down
61 changes: 61 additions & 0 deletions lighthouse-core/test/audits/maskable-icon-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* @license Copyright 2020 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const MaskableIconAudit = require('../../audits/maskable-icon.js');
const assert = require('assert');
const manifestParser = require('../../lib/manifest-parser.js');

const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
const manifestWithMaskableSrc = JSON.stringify(require('../fixtures/manifest-maskable-icon.json'));
const EXAMPLE_MANIFEST_URL = 'https://example.com/manifest.json';
const EXAMPLE_DOC_URL = 'https://example.com/index.html';

/**
* @param {string}
*/
function generateMockArtifacts(src = manifestSrc) {
const exampleManifest = manifestParser(src, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);

return {
WebAppManifest: exampleManifest,
InstallabilityErrors: {errors: []},
};
}

function generateMockAuditContext() {
return {
computedCache: new Map(),
};
}

/* eslint-env jest */

describe('Maskable Icon Audit', () => {
const context = generateMockAuditContext();

it('fails when the manifest fails to be parsed', async () => {
const artifacts = generateMockArtifacts();
artifacts.WebAppManifest = null;

const auditResult = await MaskableIconAudit.audit(artifacts, context);
assert.equal(auditResult.score, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚨 totally optional personal appeal below 🚨

since this is a brand new test file I'll attempt to pitch you on why new tests should use expect instead of assert :)

while it has pretty much no impact on this particular file, expect gives us some nice powers over assert, namely

  • partial matching
    expect({foo: 1, ...giantListOfOtherProperties}).toMatchObject({foo: 1}) will pass and on failure still print out the rest of the object for better debugging
  • better array length debugging
    expect(arr).toHaveLength(5) will print the contents of the array on failure for debugging instead of just saying Expected 5 got 4
  • snapshot testing
    expect(someObject).toMatchInlineSnapshot() is super handy for tests where we don't necessarily have an absolute correct answer but we want to be aware when anything affects the behavior (think lantern metrics, complicated byte or time savings estimation logic, etc) jest automatically manages these golden file expectations inline and can update them with the -u flag to jest
  • mock assertions
    expect(jest.fn()).toHaveBeenCalledWith('foo'), much easier to read and grok than the manual mock functions we have crafted elsewhere with assert

on the whole, most of the codebase is still assert and when adding individual tests that don't benefit from expect to thsoe files, totally expected to follow the surrounding style, but in new places where we might want to reap the benefits of expect I strongly support the use expect

and that's my spiel, feel free to decide for yourself moving forward when to use what appropriately :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You've sold me! Made the switch.

});

it('fails when the manifest contains no maskable icons', async () => {
const artifacts = generateMockArtifacts();

const auditResult = await MaskableIconAudit.audit(artifacts, context);
assert.equal(auditResult.score, 0);
});

it('passes when the manifest contains at least one maskable icon', async () => {
const artifacts = generateMockArtifacts(manifestWithMaskableSrc);

const auditResult = await MaskableIconAudit.audit(artifacts, context);
assert.equal(auditResult.score, 1);
});
});
38 changes: 38 additions & 0 deletions lighthouse-core/test/computed/manifest-values-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -252,5 +252,43 @@ describe('ManifestValues computed artifact', () => {
assert.equal(iconResults.every(i => i.passing === false), true);
});
});

describe('manifest has at least one maskable icon', () => {
it('fails when no maskable icon exists', async () => {
const manifestSrc = JSON.stringify({
icons: [{
src: 'icon.png',
purpose: 'any',
}],
});
const WebAppManifest = noUrlManifestParser(manifestSrc);
const InstallabilityErrors = {errors: []};
const artifacts = {WebAppManifest, InstallabilityErrors};

const results = await ManifestValues.request(artifacts, getMockContext());
const iconResults = results.allChecks.filter(i => i.id.includes('Maskable'));

assert.equal(iconResults.every(i => i.passing === false), true);
});

it('passes when an icon has the maskable purpose property', async () => {
const manifestSrc = JSON.stringify({
icons: [{
src: 'icon.png',
}, {
src: 'icon2.png',
purpose: 'maskable',
}],
});
const WebAppManifest = noUrlManifestParser(manifestSrc);
const InstallabilityErrors = {errors: []};
const artifacts = {WebAppManifest, InstallabilityErrors};

const results = await ManifestValues.request(artifacts, getMockContext());
const iconResults = results.allChecks.filter(i => i.id.includes('Maskable'));

assert.equal(iconResults.every(i => i.passing === true), true);
});
});
});
});
32 changes: 32 additions & 0 deletions lighthouse-core/test/fixtures/manifest-maskable-icon.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"short_name": "ExApp",
"name": "Example App",
"start_url": "./",
"icons": [
{
"src": "/images/chrome-touch-icon-96x96.png",
"sizes": "96x96",
"type": "image/png"
},
{
"src": "/images/chrome-touch-icon-192x192.png",
"sizes": "192x192",
"type": "image/png",
"purpose": "any maskable"
},
{
"src": "/images/chrome-touch-icon-512x512.png",
"sizes": "512x512",
"type": "image/png"
},
{
"src": "/images/chrome-touch-icon-384x384.png",
"sizes": "128x128 384x384",
"type": "image/png"
}
],
"background_color": "#FAFAFA",
"theme_color": "#123123",
"display": "standalone",
"orientation": "portrait"
}
69 changes: 69 additions & 0 deletions lighthouse-core/test/lib/icons-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,4 +328,73 @@ describe('Icons helper', () => {
assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 1);
});
});

describe('icons at least one maskable check', () => {
it('succeeds when at least one icon has a purpose value of maskable', () => {
const manifestSrc = JSON.stringify({
icons: [{
src: 'icon.png',
sizes: '200x200',
type: 'image/png',
purpose: 'any',
}, {
src: 'icon-vector.svg',
sizes: '100x100',
purpose: 'maskable',
}],
});
const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
assert.equal(icons.containsMaskableIcon(manifest.value), true);
});

it('succeeds when multiple icons have a purpose value of maskable', () => {
const manifestSrc = JSON.stringify({
icons: [{
src: 'icon.png',
sizes: '200x200',
type: 'image/png',
purpose: 'maskable',
}, {
src: 'icon-vector.svg',
sizes: '100x100',
purpose: 'maskable',
}],
});
const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
assert.equal(icons.containsMaskableIcon(manifest.value), true);
});

it('succeeds when an icon has multiple purpose values, including maskable', () => {
const manifestSrc = JSON.stringify({
icons: [{
src: 'icon.png',
sizes: '200x200',
type: 'image/png',
purpose: 'any maskable',
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
}, {
src: 'icon-vector.svg',
sizes: '100x100',
purpose: 'any',
}],
});
const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
assert.equal(icons.containsMaskableIcon(manifest.value), true);
});

it('fails when no icons have a purpose value of maskable', () => {
const manifestSrc = JSON.stringify({
icons: [{
src: 'icon.png',
sizes: '200x200',
type: 'image/png',
purpose: 'any',
}, {
src: 'icon-vector.svg',
sizes: '100x100',
}],
});
const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
assert.equal(icons.containsMaskableIcon(manifest.value), false);
});
});
});
Loading