audit: add apple-touch-icon audit #8857

exterkamp · 2019-05-05T00:19:51Z

Summary
Adds a new audit to the PWA category to check for apple-touch-icon link for iOS installability.

Related Issues/PRs
fixes: #7890

exterkamp · 2019-05-05T01:33:56Z

proto/sample_v2_round_trip.json

@@ -1100,7 +1108,7 @@
                        "name": "WordPress"
                    }
                ],
-                "summary": {},
+                "summary": null,


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

Not handled by #8605?

Nope

Does this need to be fixed seperately?

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

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 🤔

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

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

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

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

I upgraded to 3.7.1 and its still null...

lighthouse-core/config/default-config.js

exterkamp · 2019-05-05T01:44:57Z

lighthouse-core/audits/ios-pwa-icon.js

+
+    let explanation;
+    if (!passed) {
+      explanation = 'No `apple-touch-icon` link found.';


exterkamp · 2019-05-05T01:45:26Z

lighthouse-core/audits/ios-pwa-icon.js

+  static get meta() {
+    return {
+      id: 'ios-pwa-icon',
+      title: 'Web app has a valid iOS touch icon',


Open for wording nits on title, failureTitle, and description.

Seems fine to me! Though not sure if we should move away from "Web app" as the noun? Still see folks benefit if they dont consider themselves an app

I18n these?

i18n'd! 🇮🇨

patrickhulce

Nice!! I'm def on board for this for 5 😁

lighthouse-core/test/audits/ios-pwa-icon-test.js

lighthouse-core/audits/ios-pwa-icon.js

patrickhulce · 2019-05-05T01:45:27Z

lighthouse-core/audits/ios-pwa-icon.js

+    const appleTouchIcons = artifacts.LinkElements.filter(el => el.rel === 'apple-touch-icon');
+
+    // Audit passes if an `apple-touch-icon` exists.
+    const passed = appleTouchIcons.length !== 0;


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?

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

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.

patrickhulce · 2019-05-05T01:48:52Z

lighthouse-core/audits/ios-pwa-icon.js

+  static get meta() {
+    return {
+      id: 'ios-pwa-icon',
+      title: 'Web app has a valid iOS touch icon',


Seems fine to me! Though not sure if we should move away from "Web app" as the noun? Still see folks benefit if they dont consider themselves an app

I18n these?

lighthouse-core/config/default-config.js

exterkamp · 2019-05-05T04:13:23Z

Reading the docs more:

You can include a startup image as well <link rel="apple-touch-startup-image" href="/launch.png">
You can specify a launch icon title with the <title> or <meta name="apple-mobile-web-app-title" content="AppTitle">

Don't know if we want to include more optional-ish things like this in a more generic iOS PWA health audit, or more iOS specific PWA audits like this one.

paulirish · 2019-05-05T19:37:24Z

Don't know if we want to include more optional-ish things like this in a more generic iOS PWA health audit, or more iOS specific PWA audits like this one.

In the future, perhaps. But not in this sprint. :)

brendankenny

nice! thanks for taking this on

lighthouse-core/audits/ios-pwa-icon.js

brendankenny · 2019-05-05T22:12:38Z

lighthouse-core/audits/ios-pwa-icon.js

+   */
+  static audit(artifacts, _) {
+    const appleTouchIcons = artifacts.LinkElements.filter(
+      el => el.rel === 'apple-touch-icon' && el.href !== undefined);


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

brendankenny · 2019-05-05T22:14:27Z

lighthouse-core/audits/ios-pwa-icon.js

+    const appleTouchIcons = artifacts.LinkElements.filter(el => el.rel === 'apple-touch-icon');
+
+    // Audit passes if an `apple-touch-icon` exists.
+    const passed = appleTouchIcons.length !== 0;


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.

lighthouse-core/audits/ios-pwa-icon.js

brendankenny · 2019-05-05T22:17:52Z

lighthouse-core/config/default-config.js

@@ -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'},


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

lighthouse-core/audits/ios-pwa-icon.js

paulirish · 2019-05-05T22:09:51Z

lighthouse-core/audits/ios-pwa-icon.js

+  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).',


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.

i like brendan's developers.goog link

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

lighthouse-core/audits/ios-pwa-icon.js

paulirish · 2019-05-05T22:16:09Z

lighthouse-core/audits/ios-pwa-icon.js

+  'sites must have a valid apple-touch-icon. ' +
+  '[Learn More](https://developer.apple.com/library/archive/documentation/AppleApplications/Reference/SafariWebContent/ConfiguringWebApplications/ConfiguringWebApplications.html).',
+  /** 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.',


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

Good call.

Made an href explanation for the new href checks failing.

paulirish · 2019-05-05T22:25:47Z

lighthouse-core/audits/ios-pwa-icon.js

+   */
+  static audit(artifacts, _) {
+    const appleTouchIcons = artifacts.LinkElements.filter(
+      el => el.rel === 'apple-touch-icon' && el.href !== undefined);


there's 2 extra checks here that could make sense

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.

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.

Added 2. with a warning.

lighthouse-core/audits/ios-pwa-icon.js

brendankenny

one big thing I forgot, need to add these to the smoke test expectations for the pwa, pwa2, and pwa3 smoke tests.

I usually just let the smoke test do the investigation for each site by adding an entry for the audit in the expectations file that's obviously wrong, run the smoke test, and if the actual that gets printed looks good, I add that to the expectations file.

If we're going to have a warning and stuff, would be good to capture that for those real sites that have them

brendankenny · 2019-05-06T00:01:00Z

lighthouse-core/audits/apple-touch-icon.js

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


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

It doesn't seeeeeem like that works....

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

brendankenny · 2019-05-06T00:01:48Z

proto/sample_v2_round_trip.json

@@ -1100,7 +1108,7 @@
                        "name": "WordPress"
                    }
                ],
-                "summary": {},
+                "summary": null,


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

exterkamp · 2019-05-06T01:26:31Z

Got the base_url working. It was that the artifacts.URL.finalUrl was undefined in the tests. This seems like a fragile test for valid href's though, since the only way it will fail is if the href is invalid AND the final Url is invalid, as in the test. If either are valid, then it passes.

paulirish

potentially fetching the url to see if it's a valid icon

fetching is a bag of worms, so let's save that till later.

paulirish · 2019-05-06T01:28:17Z

lighthouse-core/audits/apple-touch-icon.js

-          || !el.href) {
+    const appleTouchIcons = artifacts.LinkElements
+      .filter(el => el.rel === 'apple-touch-icon' || el.rel === 'apple-touch-icon-precomposed')
+      .filter(el => {


.filter(el => !!el.href)

then el.href is string | null and would need to be el.href || '' to make ts be quiet. How do I make ts love me?

// Check that the href is valid try { new URL(el.href, artifacts.URL.finalUrl);

I think @paulirish was building on the point @patrickhulce made, which is that LinkElements already did this check (sorry, I had forgotten). If el.href exists, the URL is valid. so only need to do .filter(el => !!el.href) for this entire block of stuff.

OHHHHHHHH got it.

paulirish

high level lgtm

lighthouse-cli/test/smokehouse/pwa-expectations.js

lighthouse-core/audits/apple-touch-icon.js

brendankenny · 2019-05-06T06:53:44Z

lighthouse-core/audits/apple-touch-icon.js

-          || !el.href) {
+    const appleTouchIcons = artifacts.LinkElements
+      .filter(el => el.rel === 'apple-touch-icon' || el.rel === 'apple-touch-icon-precomposed')
+      .filter(el => {


I think @paulirish was building on the point @patrickhulce made, which is that LinkElements already did this check (sorry, I had forgotten). If el.href exists, the URL is valid. so only need to do .filter(el => !!el.href) for this entire block of stuff.

lighthouse-core/audits/apple-touch-icon.js

brendankenny

(switching review status for bookkeeping)

…d before? Removed href explanation. Updated en-US.

paulirish · 2019-05-06T19:54:28Z

FYI i put in a PR at google/WebFundamentals#7582 to update the WF article we link to.

Happy for upgrades there. I was doing the bare minimum :)

lighthouse-core/audits/apple-touch-icon.js

lighthouse-core/test/audits/apple-touch-icon-test.js

lighthouse-core/audits/apple-touch-icon.js

brendankenny

LGTM!

Thanks for taking this @exterkamp

add ios-pwa audit to pwa category.

85b6605

exterkamp added the new_audit label May 5, 2019

nits

560dbe7

vercel bot deployed to staging May 5, 2019 01:14 View deployment

added tests, updated sample json

00a9235

vercel bot deployed to staging May 5, 2019 01:30 View deployment

exterkamp commented May 5, 2019

View reviewed changes

exterkamp requested a review from brendankenny May 5, 2019 01:34

exterkamp commented May 5, 2019

View reviewed changes

lighthouse-core/config/default-config.js Outdated Show resolved Hide resolved

exterkamp commented May 5, 2019

View reviewed changes

patrickhulce reviewed May 5, 2019

View reviewed changes

exterkamp marked this pull request as ready for review May 5, 2019 02:03

exterkamp requested a review from paulirish as a code owner May 5, 2019 02:03

update tests, i18n audit.

e43efc6

vercel bot deployed to staging May 5, 2019 04:06 View deployment

connorjclark mentioned this pull request May 5, 2019

misc(proto): assert libprotoc version #8863

Merged

brendankenny mentioned this pull request May 5, 2019

misc(proto): require protobuf 3.7.1, add stricter audit details test #8867

Merged

brendankenny reviewed May 5, 2019

View reviewed changes

paulirish requested changes May 5, 2019

View reviewed changes

paulirish added the P0 label May 5, 2019

renamed files, updated a lot of things. testing next.

5239b85

vercel bot deployed to staging May 5, 2019 23:56 View deployment

brendankenny reviewed May 6, 2019

View reviewed changes

tests

bbc3374

vercel bot deployed to staging May 6, 2019 00:10 View deployment

rename test var

4f11781

vercel bot deployed to staging May 6, 2019 00:13 View deployment

updated assets

66fd11c

vercel bot deployed to staging May 6, 2019 01:27 View deployment

paulirish reviewed May 6, 2019

View reviewed changes

paulirish approved these changes May 6, 2019

View reviewed changes

null -> {}

a45afbc

vercel bot deployed to staging May 6, 2019 04:39 View deployment

brendankenny reviewed May 6, 2019

View reviewed changes

brendankenny requested changes May 6, 2019

View reviewed changes

exterkamp changed the title ~~audit: add iOS pwa icon audit~~ audit: add apple-touch-icon audit May 6, 2019

Remove URL checks. Update pwa expectations, don't know how they passe…

52e0528

…d before? Removed href explanation. Updated en-US.

vercel bot deployed to staging May 6, 2019 17:32 View deployment

exterkamp added the waiting4reviewer label May 6, 2019

paulirish mentioned this pull request May 6, 2019

[meta] 2019 I/O Burndown #8331

Closed

67 tasks

description whitespace

fe0e3fe

vercel bot deployed to staging May 6, 2019 19:42 View deployment

sample json

35542b7

vercel bot deployed to staging May 6, 2019 20:08 View deployment

brendankenny removed the waiting4reviewer label May 6, 2019

brendankenny reviewed May 6, 2019

View reviewed changes

more tests. updates

698e0c0

vercel bot deployed to staging May 6, 2019 22:27 View deployment

exterkamp added the waiting4reviewer label May 6, 2019

brendankenny removed the waiting4reviewer label May 6, 2019

brendankenny approved these changes May 7, 2019

View reviewed changes

brendankenny merged commit acfa777 into master May 7, 2019

brendankenny deleted the ios-pwa branch May 7, 2019 00:31

paulirish pushed a commit that referenced this pull request May 28, 2019

new_audit: add apple-touch-icon PWA audit (#8857)

a4763e4

exterkamp mentioned this pull request Aug 1, 2019

I18N: Missing translations for audits #9492

Closed

audit: add apple-touch-icon audit #8857

audit: add apple-touch-icon audit #8857

Conversation

exterkamp commented May 5, 2019

Choose a reason for hiding this comment

brendankenny May 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark May 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterkamp commented May 5, 2019

paulirish commented May 5, 2019

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish May 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterkamp commented May 6, 2019

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

brendankenny May 6, 2019 • edited Loading

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented May 6, 2019

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny May 5, 2019 •

edited

Loading

connorjclark May 5, 2019 •

edited

Loading

paulirish May 5, 2019 •

edited

Loading

brendankenny May 6, 2019 •

edited

Loading

brendankenny May 6, 2019 •

edited

Loading