core(pwa): revamp and move short_name_length audit #4860

paulirish · 2018-03-23T23:12:17Z

Two separate, but related changes in here:

webapp-install-banner audit doesn't check for a defined short_name any more. It's never technically been required, so we shouldn't assert it.
Just straight up remove short-name-length. The length of 12 characters was picked out of the air. And having a whole audit to verify str.length <= 12 is kinda annoying.
- If we didn't remove it, I'd move it to PWA, set weight to 0, and flip it to notApplicable: true if no manifest is present. But honestly I don't think it's worth keeping it around. Open to arguments tho.

Fixes #2698. See also #23 (comment)
Fixes #686 (comment)
Fixes #4860

Doesn't fix #348 because it makes a different conclusion.

patrickhulce

From the checklist perspective, I appreciated a helpful reminder to provide an alternative to long names. I'd propose turning it informative and marking not applicable when no manifest rather than killing it off entirely.

If others never found it useful I can live with it gone though :)

paulirish · 2018-03-23T23:23:40Z

I'd propose turning it informative and marking not applicable when no manifest rather than killing it off entirely.

I did have this at first, fwiw.

@brendankenny thoughts?

brendankenny

yeah, I like keeping it personally.

How did its requirement in wepapp-install-banner make it through the last round of culling?

paulirish · 2018-04-25T20:11:56Z

Welcome back!

Now it's in PWA section
Weight of 0 always, since it isn't included in PWA checklist requirements
Failure of no manifest is communicated

screenshot where there's no manifest at all

screenshot where there's a failing manifest

If you hypothetically have a manifest name that's under 12 characters we will still fail you. (We prefer you just add a short_name with the same value).

patrickhulce

wfm 👍

aside: is it just me or could multicheckaudit and byteefficiencyaudit use a bit of a refresh cleanup post I/O :)

patrickhulce · 2018-04-25T21:59:34Z

got some lint failures though

paulirish · 2018-04-25T22:01:57Z

aside: is it just me or could multicheckaudit and byteefficiencyaudit use a bit of a refresh cleanup post I/O :)

yUPPPPPPPPP

brendankenny · 2018-04-25T22:06:47Z

lighthouse-core/audits/manifest-short-name-length.js

-      }
+      const shortNameCheckIds = ['hasShortName', 'shortNameLength'];
+      manifestValues.allChecks.filter(item => shortNameCheckIds.includes(item.id)).forEach(item => {
+        if (!item.passing) failures.push(item.failureText);


won't this have the other problem where if you don't have a short name you'll get both 'Manifest does not have `short_name`' and 'Manifest `short_name` will be truncated when displayed on the homescreen'

yes good call. thx.

fixes #2698 see #23 (comment)

paulirish · 2018-04-26T17:24:25Z

has no manifest

has manifest, has no shortname:

has manifest, shortname length is <= 12

has manifest, shortname length is > 12

glittle · 2018-04-26T19:03:38Z

As for the limit of 12, does this consider that Android word-wraps the text and shows on two lines? So, two words, both 12 long, would be fine.

paulirish · 2018-04-26T19:42:33Z

does this consider that Android word-wraps the text and shows on two lines?

It doesn't, but I don't see this on my phone. Android 8.1.0 on a Pixel 2. I've never seen the text below an icon get word-wrapping. :/

glittle · 2018-04-26T19:52:38Z

Hmm... it looks like on the desktop it does not, but in a folder it does! I keep almost everything in folders, so didn't notice. (Sorry these screen captures are so big!) Probably dependent on the home screen app being used too.

JoelEinbinder · 2018-06-07T22:11:33Z

lighthouse-core/gather/computed/manifest-values.js

-        validate: manifest => !!manifest.value.short_name.value &&
+        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


I think that this should fail if there is no short_name and the name is longer than SUGGESTED_SHORTNAME_LENGTH. Or another audit like hasSomeShortName should fail. I wouldn't know that short_name exists without this audit.

paulirish · 2018-06-27T21:55:14Z

@patrickhulce @brendankenny ptal

patrickhulce

LGTM!

brendankenny · 2018-06-27T23:29:06Z

should update the PR description (looks like you ended up doing the opposite)

brendankenny

shrtnm

brendankenny · 2018-06-27T23:30:00Z

lighthouse-core/audits/manifest-short-name-length.js

+    const failures = [];
+    /** @type {Array<string>} */
+    const warnings = [];
+
    return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {


brendankenny · 2018-06-27T23:45:41Z

lighthouse-core/audits/manifest-short-name-length.js

-      return {
-        rawValue: !!isShortEnough && isShortEnough.passing,
-      };
+      return result;


there's no warnings and only one possible failure, so why not drop mulitcheckAudit and do like

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 shortNameCheck = manifestValues.allChecks.find(i => i.id === 'hasShortName'); const shortNameLengthCheck = 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, }; } if (shortNameLengthCheck && !shortNameLengthCheck.passing) { return { rawValue: false, explanation: `Failure: ${shortNameLengthCheck.failureText}.`, }; } return { rawValue: true, };

brendankenny · 2018-06-27T23:46:44Z

lighthouse-core/test/audits/manifest-short-name-length-test.js

@@ -30,29 +30,30 @@ describe('Manifest: short_name_length audit', () => {
    artifacts.Manifest = null;

    return ManifestShortNameLengthAudit.audit(artifacts).then(result => {
-      assert.strictEqual(result.rawValue, false);
-      assert.strictEqual(result.explanation, undefined);


update test description (doesn't fail)

brendankenny · 2018-06-27T23:46:59Z

lighthouse-core/test/audits/manifest-short-name-length-test.js

    });
  });

  it('fails when an empty manifest is present', () => {
    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.');


brendankenny · 2018-06-27T23:47:57Z

lighthouse-core/test/audits/manifest-short-name-length-test.js

    });
  });

-  it('fails when a manifest contains no short_name and too long name', () => {


brendankenny · 2018-06-27T23:48:54Z

lighthouse-core/test/audits/manifest-short-name-length-test.js

@@ -66,6 +67,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, false);
+      assert.ok(result.explanation.includes('without truncation'), result.explanation);
+      assert.ok(!result.explanation.includes('Manifest does not have `short_name`'));


delete this line as it's not a possibility any longer? Want to assert not not applicable, though?

brendankenny

LGTM!

patrickhulce · 2018-06-28T17:53:51Z

noyainrain · 2019-10-10T10:59:05Z

Thanks for fixing this @paulirish 😊

I know it's been over a year, just a quick question related to this change. From the description of the PR, the original goal was to remove the hasShortName check:

1. webapp-install-banner audit doesn't check for a defined `short_name` any more. It's never technically been required, so we shouldn't assert it.

If I understand the discussion correctly, it was then agreed to make it informative instead:

I'd propose turning it informative and marking not applicable when no manifest rather than killing it off entirely.

But in the end, the check still is mandatory:

      // 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',

Is there a particular reason for keeping it required in the installable-manifest audit, instead of making it informative somehow?

(Problem background: Most PWA directories use the result of lighthouse pwa-installable to inform the user if a PWA is installable. For a manifest with a short name and no (redundant) shortname, a PWA is incorrectly labeled as non-installable.)

paulirish requested review from brendankenny and patrickhulce as code owners March 23, 2018 23:12

devtools-bot added the waiting4reviewer label Mar 23, 2018

patrickhulce reviewed Mar 23, 2018

View reviewed changes

brendankenny reviewed Mar 23, 2018

View reviewed changes

patrickhulce added needs-discussion and removed waiting4reviewer labels Mar 26, 2018

devtools-bot added the waiting4reviewer label Mar 26, 2018

paulirish added waiting4committer and removed waiting4reviewer labels Apr 5, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Apr 5, 2018

paulirish added waiting4committer and removed waiting4reviewer labels Apr 12, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Apr 12, 2018

paulirish force-pushed the short-name-mania branch from 137629c to 35b821f Compare April 25, 2018 20:09

paulirish changed the title ~~core(pwa): don't require short_name. remove short_name_length audit~~ core(pwa): revamp and move short_name_length audit Apr 25, 2018

patrickhulce approved these changes Apr 25, 2018

View reviewed changes

patrickhulce removed the needs-discussion label Apr 25, 2018

brendankenny reviewed Apr 25, 2018

View reviewed changes

paulirish added 5 commits April 26, 2018 10:01

justify why we require both name and short_name.

046847f

fixes #2698 see #23 (comment)

show shortname audit in PWA category, 0 weight

aeda793

audit title brevity

4e6f65a

lint.

8a9b0b1

don't compliain twice if there's no short_name

f422128

paulirish force-pushed the short-name-mania branch from 001c900 to f422128 Compare April 26, 2018 17:13

test expectations.

96a232e

paulirish mentioned this pull request Apr 26, 2018

Failed shortname audit with no manifest file #5042

Closed

paulirish removed the waiting4reviewer label Apr 30, 2018

patrickhulce mentioned this pull request May 18, 2018

No short_name but name is already short #5259

Closed

JoelEinbinder mentioned this pull request Jun 7, 2018

fix: short_name shouldn't be required if the name is short #5449

Closed

JoelEinbinder reviewed Jun 7, 2018

View reviewed changes

paulirish added the 3.0 label Jun 27, 2018

paulirish added 3 commits June 27, 2018 13:38

Merge branch 'master' into short-name-mania

c8d18fc

not applicable status.

1078f69

golden lhr.

b7fe9ff

patrickhulce approved these changes Jun 27, 2018

View reviewed changes

brendankenny requested changes Jun 27, 2018

View reviewed changes

feedback

2f7fe37

brendankenny approved these changes Jun 28, 2018

View reviewed changes

patrickhulce merged commit 3474c39 into master Jun 28, 2018

patrickhulce deleted the short-name-mania branch June 28, 2018 18:03

brendankenny mentioned this pull request Sep 21, 2018

Wrong best practices warning about manifest short_name when testing non-pwa websites #5937

Closed

paulirish added 3.0 and removed 3.0 labels Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(pwa): revamp and move short_name_length audit #4860

core(pwa): revamp and move short_name_length audit #4860

paulirish commented Mar 23, 2018 •

edited

Loading

patrickhulce left a comment •

edited

Loading

paulirish commented Mar 23, 2018

brendankenny left a comment

paulirish commented Apr 25, 2018 •

edited

Loading

patrickhulce left a comment

patrickhulce commented Apr 25, 2018

paulirish commented Apr 25, 2018

brendankenny Apr 25, 2018

paulirish Apr 26, 2018

paulirish commented Apr 26, 2018

glittle commented Apr 26, 2018

paulirish commented Apr 26, 2018

glittle commented Apr 26, 2018 •

edited

Loading

JoelEinbinder Jun 7, 2018 •

edited by paulirish

Loading

paulirish commented Jun 27, 2018

patrickhulce left a comment

brendankenny commented Jun 27, 2018

brendankenny left a comment

brendankenny Jun 27, 2018 •

edited

Loading

brendankenny Jun 27, 2018

brendankenny Jun 27, 2018

brendankenny Jun 27, 2018

brendankenny Jun 27, 2018

brendankenny Jun 27, 2018

brendankenny left a comment

patrickhulce commented Jun 28, 2018

noyainrain commented Oct 10, 2019 •

edited

Loading

core(pwa): revamp and move short_name_length audit #4860

core(pwa): revamp and move short_name_length audit #4860

Conversation

paulirish commented Mar 23, 2018 • edited Loading

patrickhulce left a comment • edited Loading

Choose a reason for hiding this comment

paulirish commented Mar 23, 2018

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Apr 25, 2018 • edited Loading

screenshot where there's no manifest at all

screenshot where there's a failing manifest

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce commented Apr 25, 2018

paulirish commented Apr 25, 2018

brendankenny Apr 25, 2018

Choose a reason for hiding this comment

paulirish Apr 26, 2018

Choose a reason for hiding this comment

paulirish commented Apr 26, 2018

has no manifest

has manifest, has no shortname:

has manifest, shortname length is <= 12

has manifest, shortname length is > 12

glittle commented Apr 26, 2018

paulirish commented Apr 26, 2018

glittle commented Apr 26, 2018 • edited Loading

JoelEinbinder Jun 7, 2018 • edited by paulirish Loading

Choose a reason for hiding this comment

paulirish commented Jun 27, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny commented Jun 27, 2018

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

brendankenny Jun 27, 2018

Choose a reason for hiding this comment

brendankenny Jun 27, 2018

Choose a reason for hiding this comment

brendankenny Jun 27, 2018

Choose a reason for hiding this comment

brendankenny Jun 27, 2018

Choose a reason for hiding this comment

brendankenny Jun 27, 2018

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented Jun 28, 2018

noyainrain commented Oct 10, 2019 • edited Loading

paulirish commented Mar 23, 2018 •

edited

Loading

patrickhulce left a comment •

edited

Loading

paulirish commented Apr 25, 2018 •

edited

Loading

glittle commented Apr 26, 2018 •

edited

Loading

JoelEinbinder Jun 7, 2018 •

edited by paulirish

Loading

brendankenny Jun 27, 2018 •

edited

Loading

noyainrain commented Oct 10, 2019 •

edited

Loading