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 4 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
70 changes: 70 additions & 0 deletions lighthouse-core/audits/ios-pwa-icon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* @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 i18n = require('../lib/i18n/i18n.js');

/**
* @fileoverview Audits if a page's web app has an icon link for iOS installability.
*/

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. "iOS" is the name of the Apple operating system and should not be translated. */
title: 'Site a valid iOS touch icon',
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
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. "iOS" is the name of the Apple operating system and should not be translated. */
failureTitle: 'Site does not have a valid iOS 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 be installable as an iOS PWA ' +
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
'sites must have a valid apple-touch-icon. ' +
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
'[Learn More](https://developer.apple.com/library/archive/documentation/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html).',
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@paulirish paulirish 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.

wild idea. let's link to https://webhint.io/docs/user-guide/hints/hint-apple-touch-icons/ for now. tbh it's the best resource i can find, by far.

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 like brendan's developers.goog link

Copy link
Collaborator

Choose a reason for hiding this comment

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

as a wise man once told me, s/crazy idea/ 💭/ 😉

anyhoo, while the webhint docs do an awesome job explaining, I think it might be confusing to see the "what passes what fails" section of a different tool that isn't guaranteed to be in sync with the Lighthouse version they're coming from

/** Explanatory message stating that there was a failure in an audit caused by the page having an invalid apple-touch-icon link. `apple-touch-icon` is a HTML tag value and should not be translated. */
explanation: 'No valid `apple-touch-icon` link found.',
Copy link
Member

Choose a reason for hiding this comment

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

IMO we can leave out an explanation. i know other PWA audits have them but it's just repeating the title here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call.

Made an href explanation for the new href checks failing.

};

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

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

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} _
* @return {LH.Audit.Product}
*/
static audit(artifacts, _) {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
const appleTouchIcons = artifacts.LinkElements.filter(
el => el.rel === 'apple-touch-icon' && el.href !== undefined);
Copy link
Member

Choose a reason for hiding this comment

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

some sources seem to indicate it only these if in the <head> (so would also need to check el.source === 'head'), but I can't find a definitive answer on that yet

Copy link
Member

Choose a reason for hiding this comment

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

there's 2 extra checks here that could make sense

  1. they don't provide the [sizes] attribute (on at least one of them).
    • see view-source:https://caltrainschedule.io/ for an example
    • this gets a little tricky in our UX so i think we punt for today.
  2. apple-touch-icon-precomposed is basically equivalent.
    • a bunch of PWAs including https://jakearchibald.github.io/svgomg/ use this instead of the original.
    • It's basically not necessary these days, but i think we should allow this rel for now. later we can add a warning that it should be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 2. with a warning.


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

Choose a reason for hiding this comment

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

Probably cant do much without fetching the icon but at least make sure it has an href or something? Does it have to be on same origin or any other requirement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added href existence check, I don't think there are any origin requirements.

Copy link
Member

Choose a reason for hiding this comment

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

maybe also a valid href? Can do new URL(el.href, artifacts.URL.finalUrl) (finalUrl to handle the relative URL) in a try catch to check that it's valid. Kind of dumb but an easy check to add.


let explanation;
if (!passed) {
explanation = str_(UIStrings.explanation);
}
exterkamp marked this conversation as resolved.
Show resolved Hide resolved

return {
score: passed ? 1 : 0,
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',
'ios-pwa-icon',
'splash-screen',
'themed-omnibox',
'content-width',
Expand Down Expand Up @@ -516,6 +517,7 @@ const defaultConfig = {
{id: 'is-on-https', weight: 2, group: 'pwa-installable'},
{id: 'service-worker', weight: 1, group: 'pwa-installable'},
{id: 'installable-manifest', weight: 2, group: 'pwa-installable'},
{id: 'ios-pwa-icon', weight: 1, group: 'pwa-installable'},
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

as above, I don't believe this is required for installability (though we should check). Would be pwa-optimized group in that case.

We also have talked about making the current group "Chrome installable" and having separate groups for Firefox, Safari, etc as we add them so we don't end up with a mixed browser checklist, which can be kind of confusing when testing. We should revisit that decision before (if) adding it to the current installable list

// PWA Optimized
{id: 'redirects-http', weight: 2, group: 'pwa-optimized'},
{id: 'splash-screen', weight: 1, group: 'pwa-optimized'},
Expand Down
16 changes: 16 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 @@ -635,6 +635,22 @@
"message": "All text remains visible during webfont loads",
"description": "Title of a diagnostic audit that provides detail on if all the text on a webpage was visible while the page was loading its webfonts. This descriptive title is shown to users when the amount is acceptable and no user action is required."
},
"lighthouse-core/audits/ios-pwa-icon.js | description": {
"message": "In order to be installable as an iOS PWA sites must have a valid apple-touch-icon. [Learn More](https://developer.apple.com/library/archive/documentation/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html).",
"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/ios-pwa-icon.js | explanation": {
"message": "No valid `apple-touch-icon` link found.",
"description": "Explanatory message stating that there was a failure in an audit caused by the page having an invalid apple-touch-icon link. `apple-touch-icon` is a HTML tag value and should not be translated."
},
"lighthouse-core/audits/ios-pwa-icon.js | failureTitle": {
"message": "Site does not have a valid iOS 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. \"iOS\" is the name of the Apple operating system and should not be translated."
},
"lighthouse-core/audits/ios-pwa-icon.js | title": {
"message": "Site a valid iOS 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. \"iOS\" is the name of the Apple operating system and should not be translated."
},
"lighthouse-core/audits/load-fast-enough-for-pwa.js | description": {
"message": "A fast page load over a cellular network ensures a good mobile user experience. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/fast-3g).",
"description": "Description of a Lighthouse audit that tells the user *why* they need to load fast enough on mobile networks. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation."
Expand Down
48 changes: 48 additions & 0 deletions lighthouse-core/test/audits/ios-pwa-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/ios-pwa-icon.js');

/* eslint-env jest */

describe('PWA: ios-pwa-icon audit', () => {
it(`fails when apple-touch-icon is not present`, async () => {
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
30 changes: 29 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 @@
]
}
},
"ios-pwa-icon": {
"id": "ios-pwa-icon",
"title": "Site does not have a valid iOS touch icon",
"description": "In order to be installable as an iOS PWA sites must have a valid apple-touch-icon. [Learn More](https://developer.apple.com/library/archive/documentation/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html).",
"score": 0,
"scoreDisplayMode": "binary",
"explanation": "No valid `apple-touch-icon` link found."
},
"splash-screen": {
"id": "splash-screen",
"title": "Is not configured for a custom splash screen",
Expand Down Expand Up @@ -3660,6 +3668,11 @@
"weight": 2,
"group": "pwa-installable"
},
{
"id": "ios-pwa-icon",
"weight": 1,
"group": "pwa-installable"
},
{
"id": "redirects-http",
"weight": 2,
Expand Down Expand Up @@ -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:ios-pwa-icon",
"duration": 100,
"entryType": "measure"
},
{
"startTime": 0,
"name": "lh:audit:splash-screen",
Expand Down Expand Up @@ -4911,6 +4930,15 @@
"lighthouse-core/audits/redirects.js | description": [
"audits.redirects.description"
],
"lighthouse-core/audits/ios-pwa-icon.js | failureTitle": [
"audits[ios-pwa-icon].title"
],
"lighthouse-core/audits/ios-pwa-icon.js | description": [
"audits[ios-pwa-icon].description"
],
"lighthouse-core/audits/ios-pwa-icon.js | explanation": [
"audits[ios-pwa-icon].explanation"
],
"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 @@ -1040,6 +1040,14 @@
"scoreDisplayMode": "manual",
"title": "Interactive elements indicate their purpose and state"
},
"ios-pwa-icon": {
"description": "In order to be installable as an iOS PWA sites must have a valid apple-touch-icon. [Learn More](https://developer.apple.com/library/archive/documentation/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html).",
"explanation": "No valid `apple-touch-icon` link found.",
"id": "ios-pwa-icon",
"score": 0.0,
"scoreDisplayMode": "binary",
"title": "Site does not have a valid iOS touch icon"
},
"is-crawlable": {
"description": "Search engines are unable to include your pages in search results if they don't have permission to crawl them. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/indexing).",
"details": {
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 @@ -3521,6 +3529,11 @@
"id": "installable-manifest",
"weight": 2.0
},
{
"group": "pwa-installable",
"id": "ios-pwa-icon",
"weight": 1.0
},
{
"group": "pwa-optimized",
"id": "redirects-http",
Expand Down Expand Up @@ -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:ios-pwa-icon",
"startTime": 0.0
},
{
"duration": 100.0,
"entryType": "measure",
Expand Down