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

audit: add apple-touch-icon audit #8857

Merged
merged 15 commits into from
May 7, 2019
Merged

audit: add apple-touch-icon audit #8857

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
85b6605
add ios-pwa audit to pwa category.
exterkamp May 5, 2019
560dbe7
nits
exterkamp May 5, 2019
00a9235
added tests, updated sample json
exterkamp May 5, 2019
e43efc6
update tests, i18n audit.
exterkamp May 5, 2019
5239b85
renamed files, updated a lot of things. testing next.
exterkamp May 5, 2019
bbc3374
tests
exterkamp May 6, 2019
4f11781
rename test var
exterkamp May 6, 2019
95d15c0
added apple-touch-icon to pwa expectations
exterkamp May 6, 2019
910ed1f
Paul's wording. Figured out base_url
exterkamp May 6, 2019
66fd11c
updated assets
exterkamp May 6, 2019
a45afbc
null -> {}
exterkamp May 6, 2019
52e0528
Remove URL checks. Update pwa expectations, don't know how they pass…
exterkamp May 6, 2019
fe0e3fe
description whitespace
paulirish May 6, 2019
35542b7
sample json
paulirish May 6, 2019
698e0c0
more tests. updates
exterkamp May 6, 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
8 changes: 8 additions & 0 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ Object {
Object {
"path": "installable-manifest",
},
Object {
"path": "ios-pwa-icon",
},
Object {
"path": "splash-screen",
},
Expand Down Expand Up @@ -893,6 +896,11 @@ Object {
"id": "installable-manifest",
"weight": 2,
},
Object {
"group": "pwa-installable",
"id": "ios-pwa-icon",
"weight": 1,
},
Object {
"group": "pwa-optimized",
"id": "redirects-http",
Expand Down
92 changes: 92 additions & 0 deletions lighthouse-core/audits/apple-touch-icon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/**
* @license Copyright 2019 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 URL = require('../lib/url-shim');
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
const i18n = require('../lib/i18n/i18n.js');

/**
* @fileoverview Audits if a page's web app has an icon link for iOS installability.
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
*/

const UIStrings = {
/** Title of a Lighthouse audit that tells the user that their site contains a vaild touch icon. This descriptive title is shown when the page contains a valid iOS touch icon. */
title: 'Provides a valid apple-touch-icon',
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
/** Title of a Lighthouse audit that tells the user that their site contains a vaild touch icon. This descriptive title is shown when the page does not contain a valid iOS touch icon. */
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
failureTitle: 'Does not provide a valid apple-touch-icon',
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
/** Description of a Lighthouse audit that tells the user what having a valid apple-touch-icon does. 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: 'In order to have a custom icon on iOS once ' +
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
'installed, a site must have a valid apple-touch-icon. ' +
'[Learn More](https://developers.google.com/web/fundamentals/design-and-ux/browser-customization/).',
/** Warning that HTML attribute `apple-touch-icon-precomposed` should not be used in favor of `apple-touch-icon`. "apple-touch-icon-precomposed" and "apple-touch-icon" are HTML attributes and should not be translated. */
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
precomposedWarning: '`apple-touch-icon-precomposed` is out of date, ' +
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
'`apple-touch-icon` is preferred.',
/** Explanatory message stating that there was a failure in an audit caused by the page's `apple-touch-icon` having an invalide `href` attribute. `apple-touch-icon` and `href` are HTML tag values and should not be translated. */
explanation: '`apple-touch-icon`\'s `href` attribute is not valid',
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
};

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

class IosPwaIcon extends Audit {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'apple-touch-icon',
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['LinkElements'],
};
}

/**
* @param {LH.Artifacts} artifacts
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
let explanation;
const appleTouchIcons = artifacts.LinkElements.filter(
el => {
if (el.rel !== 'apple-touch-icon' && el.rel !== 'apple-touch-icon-precomposed'
|| !el.href) {
return false;
}
// Check that the href is valid
Copy link
Member

Choose a reason for hiding this comment

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

this section makes my eyes bleed. 🤕

i prefer the arr.filter().filter().filter() we do elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

lol i can do that instead, but the href stuff is blegh, because it will fail 1 of the 2, so it has to be this kind of garbage somwhere

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW @exterkamp you shouldn't need to do this at all, LinkElements already does the normalization on href, just checking if href is present should be enough (hrefRaw contains the potentially invalid one)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize it was my comment that was probably misleading with the validation, I just meant checking if href was truthy and potentially fetching the url to see if it's a valid icon :)

try {
new URL(el.href);
return true;
} catch (e) {}
// Check if the href needs the base url to be valid
Copy link
Member

Choose a reason for hiding this comment

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

you can just combine these checks (the base url is ignored if the first argument is already absolute)

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 doesn't seeeeeem like that works....

Copy link
Member Author

Choose a reason for hiding this comment

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

got it! finalUrl was undefined, and was silently throwing and caught by the audit.

try {
new URL(el.href, artifacts.URL.finalUrl);
return true;
} catch (e) {
explanation = str_(UIStrings.explanation);
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
});

// Audit passes if an `apple-touch-icon` exists.
const passed = appleTouchIcons.length !== 0;

const warnings = [];
if (appleTouchIcons.filter(el => el.rel === 'apple-touch-icon-precomposed').length !== 0) {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
warnings.push(str_(UIStrings.precomposedWarning));
}

return {
score: passed ? 1 : 0,
warnings,
explanation,
};
}
}

module.exports = IosPwaIcon;
module.exports.UIStrings = UIStrings;
2 changes: 2 additions & 0 deletions lighthouse-core/config/default-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ const defaultConfig = {
'critical-request-chains',
'redirects',
'installable-manifest',
'apple-touch-icon',
'splash-screen',
'themed-omnibox',
'content-width',
Expand Down Expand Up @@ -523,6 +524,7 @@ const defaultConfig = {
{id: 'content-width', weight: 1, group: 'pwa-optimized'},
{id: 'viewport', weight: 2, group: 'pwa-optimized'},
{id: 'without-javascript', weight: 1, group: 'pwa-optimized'},
{id: 'apple-touch-icon', weight: 1, group: 'pwa-optimized'},
// Manual audits
{id: 'pwa-cross-browser', weight: 0},
{id: 'pwa-page-transitions', weight: 0},
Expand Down
20 changes: 20 additions & 0 deletions lighthouse-core/lib/i18n/en-US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,26 @@
"message": "`<video>` elements contain a `<track>` element with `[kind=\"description\"]`",
"description": "Title of an accesibility audit that evaluates if all video elements have child track elements that contain a description of the video content. This title is descriptive of the successful state and is shown to users when no user action is required."
},
"lighthouse-core/audits/apple-touch-icon.js | description": {
"message": "In order to have a custom icon on iOS once installed, a site must have a valid apple-touch-icon. [Learn More](https://developers.google.com/web/fundamentals/design-and-ux/browser-customization/).",
"description": "Description of a Lighthouse audit that tells the user what having a valid apple-touch-icon does. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation."
},
"lighthouse-core/audits/apple-touch-icon.js | explanation": {
"message": "`apple-touch-icon`'s `href` attribute is not valid",
"description": "Explanatory message stating that there was a failure in an audit caused by the page's `apple-touch-icon` having an invalide `href` attribute. `apple-touch-icon` and `href` are HTML tag values and should not be translated."
},
"lighthouse-core/audits/apple-touch-icon.js | failureTitle": {
"message": "Does not provide a valid apple-touch-icon",
"description": "Title of a Lighthouse audit that tells the user that their site contains a vaild touch icon. This descriptive title is shown when the page does not contain a valid iOS touch icon."
},
"lighthouse-core/audits/apple-touch-icon.js | precomposedWarning": {
"message": "`apple-touch-icon-precomposed` is out of date, `apple-touch-icon` is preferred.",
"description": "Warning that HTML attribute `apple-touch-icon-precomposed` should not be used in favor of `apple-touch-icon`. \"apple-touch-icon-precomposed\" and \"apple-touch-icon\" are HTML attributes and should not be translated."
},
"lighthouse-core/audits/apple-touch-icon.js | title": {
"message": "Provides a valid apple-touch-icon",
"description": "Title of a Lighthouse audit that tells the user that their site contains a vaild touch icon. This descriptive title is shown when the page contains a valid iOS touch icon."
},
"lighthouse-core/audits/bootup-time.js | chromeExtensionsWarning": {
"message": "Chrome extensions negatively affected this page's load performance. Try auditing the page in incognito mode or from a Chrome profile without extensions.",
"description": "A message displayed in a Lighthouse audit result warning that Chrome extensions on the user's system substantially affected Lighthouse's measurements and instructs the user on how to run again without those extensions."
Expand Down
48 changes: 48 additions & 0 deletions lighthouse-core/test/audits/apple-touch-icon-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* @license Copyright 2019 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 IosPwaIcon = require('../../audits/apple-touch-icon.js');

/* eslint-env jest */

describe('PWA: apple-touch-icon audit', () => {
it(`fails when apple-touch-icon is not present`, async () => {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
const artifacts = {
LinkElements: [],
};

const context = {settings: {}, computedCache: new Map()};
const {score, explanation} = IosPwaIcon.audit(artifacts, context);

expect(score).toBe(0);
expect(explanation).toBeDisplayString('No valid `apple-touch-icon` link found.');
});

it(`fails when apple-touch-icon does not have an href`, async () => {
const artifacts = {
LinkElements: [{rel: 'apple-touch-icon'}],
};

const context = {settings: {}, computedCache: new Map()};
const {score, explanation} = IosPwaIcon.audit(artifacts, context);

expect(score).toBe(0);
expect(explanation).toBeDisplayString('No valid `apple-touch-icon` link found.');
});

it(`passes when apple-touch-icon is on page`, async () => {
const artifacts = {
LinkElements: [{rel: 'apple-touch-icon', href: 'https://example.com/touch-icon.png'}],
};

const context = {settings: {}, computedCache: new Map()};
const {score, explanation} = IosPwaIcon.audit(artifacts, context);

expect(score).toBe(1);
expect(explanation).toBeUndefined();
});
});
4 changes: 2 additions & 2 deletions lighthouse-core/test/report/html/renderer/category-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ describe('CategoryRenderer', () => {
const manualAudits = elem.querySelectorAll('.lh-clump--manual .lh-audit');

assert.equal(passedAudits.length, 2);
assert.equal(failedAudits.length, 8);
assert.equal(failedAudits.length, 9);
assert.equal(warningAudits.length, 2);
assert.equal(manualAudits.length, 3);
});
Expand All @@ -380,7 +380,7 @@ describe('CategoryRenderer', () => {
const failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit');

assert.equal(passedAudits.length, 0);
assert.equal(failedAudits.length, 12);
assert.equal(failedAudits.length, 13);
});

it('expands warning audit group', () => {
Expand Down
27 changes: 26 additions & 1 deletion lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,14 @@
]
}
},
"apple-touch-icon": {
"id": "apple-touch-icon",
"title": "Does not provide a valid apple-touch-icon",
"description": "In order to have a custom icon on iOS once installed, a site must have a valid apple-touch-icon. [Learn More](https://developers.google.com/web/fundamentals/design-and-ux/browser-customization/).",
"score": 0,
"scoreDisplayMode": "binary",
"warnings": []
},
"splash-screen": {
"id": "splash-screen",
"title": "Is not configured for a custom splash screen",
Expand Down Expand Up @@ -3690,6 +3698,11 @@
"weight": 1,
"group": "pwa-optimized"
},
{
"id": "apple-touch-icon",
"weight": 1,
"group": "pwa-optimized"
},
{
"id": "pwa-cross-browser",
"weight": 0
Expand All @@ -3704,7 +3717,7 @@
}
],
"id": "pwa",
"score": 0.42
"score": 0.41
}
},
"categoryGroups": {
Expand Down Expand Up @@ -4073,6 +4086,12 @@
"duration": 100,
"entryType": "measure"
},
{
"startTime": 0,
"name": "lh:audit:apple-touch-icon",
"duration": 100,
"entryType": "measure"
},
{
"startTime": 0,
"name": "lh:audit:splash-screen",
Expand Down Expand Up @@ -4911,6 +4930,12 @@
"lighthouse-core/audits/redirects.js | description": [
"audits.redirects.description"
],
"lighthouse-core/audits/apple-touch-icon.js | failureTitle": [
"audits[apple-touch-icon].title"
],
"lighthouse-core/audits/apple-touch-icon.js | description": [
"audits[apple-touch-icon].description"
],
"lighthouse-core/audits/mainthread-work-breakdown.js | title": [
"audits[mainthread-work-breakdown].title"
],
Expand Down
27 changes: 23 additions & 4 deletions proto/sample_v2_round_trip.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@
"scoreDisplayMode": "binary",
"title": "Uses Application Cache"
},
"apple-touch-icon": {
"description": "In order to have a custom icon on iOS once installed, a site must have a valid apple-touch-icon. [Learn More](https://developers.google.com/web/fundamentals/design-and-ux/browser-customization/).",
"id": "apple-touch-icon",
"score": 0.0,
"scoreDisplayMode": "binary",
"title": "Does not provide a valid apple-touch-icon",
"warnings": []
},
"aria-allowed-attr": {
"description": "Each ARIA `role` supports a specific subset of `aria-*` attributes. Mismatching these invalidates the `aria-*` attributes. [Learn more](https://dequeuniversity.com/rules/axe/3.1/aria-allowed-attr?application=lighthouse).",
"id": "aria-allowed-attr",
Expand Down Expand Up @@ -1100,7 +1108,7 @@
"name": "WordPress"
}
],
"summary": {},
"summary": null,
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this need to be fixed seperately? Not handled by #8605?

Copy link
Member

@brendankenny brendankenny May 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not handled by #8605?

Nope

Does this need to be fixed seperately?

You tell me :) Why did it change these unrelated ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just did it on master, same thing. I'm not 100% sure why. It is converting {} into null inside of detail elements. I'm not overly concerned with it, since its a conversion difference within details. I can look more into it. I think it has to do with #8605 somehow tho 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Found it. In #8539 it was changed (I think in error) and I think it is supposed to be null.

Copy link
Collaborator

@connorjclark connorjclark May 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@exterkamp maybe we should assert on proto --version in compile-proto. #8863

Copy link
Member

Choose a reason for hiding this comment

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

Found it. In #8539 it was changed (I think in error) and I think it is supposed to be null.

nice catch. One issue is that the details roundtrip check is using toMatchObject, which while also helpful here, will also happily assert true for expect({summary: null}).toMatchObject({summary: {}}) or expect({summary:{}}).toMatchObject({summary: {}})

Is it supposed to be null, though? It's {} in sample_v2.json

Copy link
Member

Choose a reason for hiding this comment

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

after #8867, these will need to be {}. Update to > 3.7.1 to get that automatically :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I upgraded to 3.7.1 and its still null...

"type": "table"
},
"id": "js-libraries",
Expand Down Expand Up @@ -1227,7 +1235,7 @@
"details": {
"headings": [],
"items": [],
"summary": {},
"summary": null,
"type": "table"
},
"id": "link-text",
Expand Down Expand Up @@ -1883,7 +1891,7 @@
"vulnCount": 2.0
}
],
"summary": {},
"summary": null,
"type": "table"
},
"displayValue": "2 vulnerabilities detected",
Expand Down Expand Up @@ -3551,6 +3559,11 @@
"id": "without-javascript",
"weight": 1.0
},
{
"group": "pwa-optimized",
"id": "apple-touch-icon",
"weight": 1.0
},
{
"id": "pwa-cross-browser",
"weight": 0.0
Expand All @@ -3567,7 +3580,7 @@
"description": "These checks validate the aspects of a Progressive Web App. [Learn more](https://developers.google.com/web/progressive-web-apps/checklist).",
"id": "pwa",
"manualDescription": "These checks are required by the baseline [PWA Checklist](https://developers.google.com/web/progressive-web-apps/checklist) but are not automatically checked by Lighthouse. They do not affect your score but it's important that you verify them manually.",
"score": 0.42,
"score": 0.41,
"title": "Progressive Web App"
},
"seo": {
Expand Down Expand Up @@ -4076,6 +4089,12 @@
"name": "lh:computed:ManifestValues",
"startTime": 0.0
},
{
"duration": 100.0,
"entryType": "measure",
"name": "lh:audit:apple-touch-icon",
"startTime": 0.0
},
{
"duration": 100.0,
"entryType": "measure",
Expand Down