From 3474c3910d87563a09bd18284abf1df56529bcc9 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Thu, 28 Jun 2018 19:03:22 +0100 Subject: [PATCH] core(pwa): revamp and move short_name_length audit (#4860) --- .../audits/manifest-short-name-length.js | 49 +++++++++++-------- .../audits/webapp-install-banner.js | 6 +++ lighthouse-core/config/default-config.js | 2 +- .../gather/computed/manifest-values.js | 6 ++- .../audits/manifest-short-name-length-test.js | 25 ++++++---- lighthouse-core/test/results/sample_v2.json | 16 +++--- 6 files changed, 63 insertions(+), 41 deletions(-) diff --git a/lighthouse-core/audits/manifest-short-name-length.js b/lighthouse-core/audits/manifest-short-name-length.js index 3b03b40c03c4..1c2761631332 100644 --- a/lighthouse-core/audits/manifest-short-name-length.js +++ b/lighthouse-core/audits/manifest-short-name-length.js @@ -14,9 +14,8 @@ class ManifestShortNameLength extends Audit { static get meta() { return { id: 'manifest-short-name-length', - title: 'Manifest\'s `short_name` won\'t be truncated when displayed on homescreen', - failureTitle: 'Manifest\'s `short_name` will be truncated when displayed ' + - 'on homescreen', + title: 'The `short_name` won\'t be truncated on the homescreen', + failureTitle: 'The `short_name` will be truncated on the homescreen', description: 'Make your app\'s `short_name` fewer than 12 characters to ' + 'ensure that it\'s not truncated on homescreens. [Learn ' + 'more](https://developers.google.com/web/tools/lighthouse/audits/manifest-short_name-is-not-truncated).', @@ -28,27 +27,37 @@ class ManifestShortNameLength extends Audit { * @param {LH.Artifacts} artifacts * @return {Promise} */ - static audit(artifacts) { - return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => { - if (manifestValues.isParseFailure) { - return { - rawValue: false, - }; - } + static async audit(artifacts) { + const manifestValues = await artifacts.requestManifestValues(artifacts.Manifest); + // If there's no valid manifest, this audit is not applicable + if (manifestValues.isParseFailure) { + return { + rawValue: true, + notApplicable: true, + }; + } - const hasShortName = manifestValues.allChecks.find(i => i.id === 'hasShortName'); - if (!hasShortName || !hasShortName.passing) { - return { - rawValue: false, - explanation: 'No short_name found in manifest.', - }; - } + const shortNameCheck = manifestValues.allChecks.find(i => i.id === 'hasShortName'); + const shortNameLengthCheck = manifestValues.allChecks.find(i => i.id === 'shortNameLength'); - const isShortEnough = manifestValues.allChecks.find(i => i.id === 'shortNameLength'); + // If there's no short_name present, this audit is not applicable + if (shortNameCheck && !shortNameCheck.passing) { + return { + rawValue: true, + notApplicable: true, + }; + } + // Shortname is present, but it's too long + if (shortNameLengthCheck && !shortNameLengthCheck.passing) { return { - rawValue: !!isShortEnough && isShortEnough.passing, + rawValue: false, + explanation: `Failure: ${shortNameLengthCheck.failureText}.`, }; - }); + } + // Has a shortname that's under the threshold + return { + rawValue: true, + }; } } diff --git a/lighthouse-core/audits/webapp-install-banner.js b/lighthouse-core/audits/webapp-install-banner.js index 5e41e351c48e..be4f7a1142ce 100644 --- a/lighthouse-core/audits/webapp-install-banner.js +++ b/lighthouse-core/audits/webapp-install-banner.js @@ -57,6 +57,12 @@ class WebappInstallBanner extends MultiCheckAudit { const failures = []; const bannerCheckIds = [ 'hasName', + // Technically shortname isn't required (if name is defined): + // https://cs.chromium.org/chromium/src/chrome/browser/installable/installable_manager.cc?type=cs&q=IsManifestValidForWebApp+f:cc+-f:test&sq=package:chromium&l=473 + // Despite this, we think it's better to require it anyway. + // short_name is preferred for the homescreen icon, but a longer name can be used in + // the splash screen and app title. Given the different usecases, we'd like to make it clearer + // that the developer has two possible strings to work with. 'hasShortName', 'hasStartUrl', 'hasPWADisplayValue', diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js index 6a4f67740beb..f539718842c6 100644 --- a/lighthouse-core/config/default-config.js +++ b/lighthouse-core/config/default-config.js @@ -302,6 +302,7 @@ module.exports = { {id: 'splash-screen', weight: 1}, {id: 'themed-omnibox', weight: 1}, {id: 'content-width', weight: 1}, + {id: 'manifest-short-name-length', weight: 0}, // Manual audits {id: 'pwa-cross-browser', weight: 0}, {id: 'pwa-page-transitions', weight: 0}, @@ -375,7 +376,6 @@ module.exports = { {id: 'no-vulnerable-libraries', weight: 1}, {id: 'notification-on-start', weight: 1}, {id: 'deprecations', weight: 1}, - {id: 'manifest-short-name-length', weight: 1}, {id: 'password-inputs-can-be-pasted-into', weight: 1}, {id: 'errors-in-console', weight: 1}, {id: 'image-aspect-ratio', weight: 1}, diff --git a/lighthouse-core/gather/computed/manifest-values.js b/lighthouse-core/gather/computed/manifest-values.js index 11b71b91d540..ba73c12b2760 100644 --- a/lighthouse-core/gather/computed/manifest-values.js +++ b/lighthouse-core/gather/computed/manifest-values.js @@ -11,7 +11,7 @@ const icons = require('../../lib/icons'); const PWA_DISPLAY_VALUES = ['minimal-ui', 'fullscreen', 'standalone']; // Historically, Chrome recommended 12 chars as the maximum short_name length to prevent truncation. -// See #69 for more discussion & https://developer.chrome.com/apps/manifest/name#short_name +// For more discussion, see https://github.com/GoogleChrome/lighthouse/issues/69 and https://developer.chrome.com/apps/manifest/name#short_name const SUGGESTED_SHORTNAME_LENGTH = 12; class ManifestValues extends ComputedArtifact { @@ -69,7 +69,9 @@ class ManifestValues extends ComputedArtifact { }, { id: 'shortNameLength', - failureText: 'Manifest `short_name` will be truncated when displayed on the homescreen', + failureText: `Manifest's \`short_name\` is too long (>${SUGGESTED_SHORTNAME_LENGTH} ` + + `characters) to be displayed on a homescreen without truncation`, + // Pass if there's no short_name. Don't want to report a non-existent string is too long validate: manifestValue => !!manifestValue.short_name.value && manifestValue.short_name.value.length <= SUGGESTED_SHORTNAME_LENGTH, }, diff --git a/lighthouse-core/test/audits/manifest-short-name-length-test.js b/lighthouse-core/test/audits/manifest-short-name-length-test.js index 44a844469fa7..bc5adefe9037 100644 --- a/lighthouse-core/test/audits/manifest-short-name-length-test.js +++ b/lighthouse-core/test/audits/manifest-short-name-length-test.js @@ -25,34 +25,35 @@ function generateMockArtifacts() { /* eslint-env mocha */ describe('Manifest: short_name_length audit', () => { - it('fails with no explanation if page had no manifest', () => { + it('marked as notApplicable if page had no manifest', () => { const artifacts = generateMockArtifacts(); artifacts.Manifest = null; return ManifestShortNameLengthAudit.audit(artifacts).then(result => { - assert.strictEqual(result.rawValue, false); - assert.strictEqual(result.explanation, undefined); + assert.strictEqual(result.rawValue, true); + assert.strictEqual(result.notApplicable, true); }); }); - it('fails when an empty manifest is present', () => { + it('marked as notApplicable if manifest is present but empty', () => { const artifacts = generateMockArtifacts(); artifacts.Manifest = manifestParser('{}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); return ManifestShortNameLengthAudit.audit(artifacts).then(result => { - assert.equal(result.rawValue, false); - assert.equal(result.explanation, 'No short_name found in manifest.'); + assert.strictEqual(result.rawValue, true); + assert.strictEqual(result.notApplicable, true); }); }); - it('fails when a manifest contains no short_name and too long name', () => { + it('marked as notApplicable when a manifest contains no short_name', () => { const artifacts = generateMockArtifacts(); const manifestSrc = JSON.stringify({ name: 'i\'m much longer than the recommended size', }); artifacts.Manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); return ManifestShortNameLengthAudit.audit(artifacts).then(result => { - assert.equal(result.rawValue, false); - assert.notEqual(result.explanation, undefined); + assert.strictEqual(result.rawValue, true); + assert.strictEqual(result.notApplicable, true); + assert.equal(result.explanation, undefined); }); }); @@ -66,10 +67,12 @@ describe('Manifest: short_name_length audit', () => { artifacts.Manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); return ManifestShortNameLengthAudit.audit(artifacts).then(result => { assert.equal(result.rawValue, false); + assert.ok(result.explanation.includes('without truncation'), result.explanation); + assert.equal(result.notApplicable, undefined); }); }); - it('succeeds when a manifest contains a short_name', () => { + it('passes when a manifest contains a short_name', () => { const artifacts = generateMockArtifacts(); const manifestSrc = JSON.stringify({ short_name: 'Lighthouse', @@ -77,6 +80,8 @@ describe('Manifest: short_name_length audit', () => { artifacts.Manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); return ManifestShortNameLengthAudit.audit(artifacts).then(result => { assert.equal(result.rawValue, true); + assert.equal(result.explanation, undefined); + assert.equal(result.notApplicable, undefined); }); }); /* eslint-enable camelcase */ diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json index cbc7b5a1335d..ce44c584681e 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -543,11 +543,11 @@ }, "manifest-short-name-length": { "id": "manifest-short-name-length", - "title": "Manifest's `short_name` will be truncated when displayed on homescreen", + "title": "The `short_name` won't be truncated on the homescreen", "description": "Make your app's `short_name` fewer than 12 characters to ensure that it's not truncated on homescreens. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/manifest-short_name-is-not-truncated).", - "score": 0, - "scoreDisplayMode": "binary", - "rawValue": false + "score": null, + "scoreDisplayMode": "not-applicable", + "rawValue": true }, "content-width": { "id": "content-width", @@ -2982,6 +2982,10 @@ "id": "content-width", "weight": 1 }, + { + "id": "manifest-short-name-length", + "weight": 0 + }, { "id": "pwa-cross-browser", "weight": 0 @@ -3269,10 +3273,6 @@ "id": "deprecations", "weight": 1 }, - { - "id": "manifest-short-name-length", - "weight": 1 - }, { "id": "password-inputs-can-be-pasted-into", "weight": 1