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.

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

audit: Can prompt recurring visitor to install to homescreen #23

Closed
paulirish opened this issue Mar 17, 2016 · 12 comments
Closed

audit: Can prompt recurring visitor to install to homescreen #23

paulirish opened this issue Mar 17, 2016 · 12 comments
Assignees

Comments

@paulirish
Copy link
Member

Manual evaluation: manifest has short_name, 144x144 png icon

input:
audit:
  • has a registered SW
  • valid start_url
  • valid name
  • valid short_name
  • icon of size >= 144x144 and png (either type image/png or filename ending in .png)

update: ^ Copied from brendan's comment below.

See also "PWA validator" bookmarklet - #17

docs: install-to-homescreen on /web

when:
  • page loaded and in idle state
@paulirish
Copy link
Member Author

from the docs: "Your icon declarations should include a mime type of image/png"

@brendankenny is going to verify in the source

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/banners/app_banner_data_fetcher.cc&q=%22bool%20AppBannerDataFetcher::IsManifestValidForWebApp%22&sq=package:chromium&type=cs


also found in //src/chrome/android/java/res/values/dimens.xml:

 <dimen name="webapp_home_screen_icon_size">48dp</dimen>

@brendankenny
Copy link
Member

currently required in manifest:

  • valid start_url
  • valid name
  • valid short_name
  • icon of size >= 144x144 and png (either type image/png or filename ending in .png)

@paulirish
Copy link
Member Author

"valid". for url it's valid by chromium's gURL standards, right? and then the two names it checks for exists and not empty?

@brendankenny
Copy link
Member

for valid URL, it does appear that's correct. For the name checks, you may technically be able to get away with empty strings (it checks they aren't null), but I'm not sure. The manifest parse step may just covert empty strings to null.

@paulirish
Copy link
Member Author

k cool​

@paulirish
Copy link
Member Author

Nearly done, but we need to check for 144px icons instead of 192px. (But having 192 icons would allow the test to pass)

brendan said:

. The change shouldn't be long in the icons-192 audit. Split on 'x' (which is required to be lowercase after parsing), parseInt on each, then check that both width and height are >= 144

@paulirish paulirish reopened this Mar 30, 2016
@paulirish paulirish modified the milestones: PWA MVP, Lighthouse MVP Mar 30, 2016
@paulirish
Copy link
Member Author

@brendan i have this alsmost complete in a branch on my fork. 1% battery. see you guys.

@paulirish paulirish self-assigned this Apr 4, 2016
@brendankenny
Copy link
Member

To add to this, on a macbook pro, I get a "256px square icon is required" in Chrome. The 256px appears to come from it not taking the emulated screen into account, but we may need to investigate the "square" part of that.

This seems to discard non-square icons:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/manifest/manifest_icon_selector.cc&rcl=1459883706&l=43

but this explicitly tests non-square as OK:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/banners/app_banner_data_fetcher_unittest.cc&sq=package:chromium&type=cs&q=%22Non-square%20is%20okay%22%20file:%5Esrc/chrome/browser/banners/

so not sure how these two worlds interact within Chrome.

@paulirish
Copy link
Member Author

Fixed by the icons PR: #116

@paulirish
Copy link
Member Author

paulirish commented Jan 4, 2017

Just verifying the check here:

source: https://cs.chromium.org/chromium/src/chrome/browser/installable/installable_manager.cc?type=cs&q=IsManifestValidForWebApp&sq=package:chromium&l=304

validity requires ALL of these:

  1. manifest is not empty
  2. manifest has valid start url
  3. manifest has a valid name and valid shortname
  4. manifest display property is either standalone or fullscreen
  5. manifest contains icon that's a png and size >= 144px
  6. SW is registered, and it owns this page and the manifest's start url
  7. Site engagement score of 2 or higher

Our audit is a little softer in its tests so far. It doesn't look at standalone/fullscreen and doesn't consider SW controlling the starturl. Also it checks for shortname || name when it should be shortname && name. Also obviously we don't look at site engagement.

@brendankenny
Copy link
Member

manifest has a valid name and valid shortname

this one has been confusing because of the difference in context of use of the short_name. See #348, for instance. We should add a clear comment on this when we fix this :)

It doesn't look at standalone/fullscreen

IMO we should get rid of the manifest display audit and rename it chrome-a2hs-display.js or whatever to make it clear what it is and test only for standalone/fullscreen

@paulirish
Copy link
Member Author

Looked again at the current implementation (installable_manager.cc and app_banner_manager.cc):

  1. is secure
    • is either localhost or has a valid SSL Certificate
  2. has a service worker
    • it must control the current url and the manifest's start_url
  3. valid_manifest
    • has a valid starturl
    • has either a name or shortname (that isn't an empty string)
    • display is standalone/fullscreen/minimal-ui
  4. has a satisfactory icon
    • must be a png
    • must be square
    • must be at least 72px, but chrome much prefers 144px or larger.
  5. has sufficient engagement
    • currently requires a 2. see at chrome://site-engagement/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants