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

web manifest does not support combination of values for icon purpose #3791

Closed
tmb-github opened this issue May 31, 2020 · 7 comments · Fixed by #3841
Closed

web manifest does not support combination of values for icon purpose #3791

tmb-github opened this issue May 31, 2020 · 7 comments · Fixed by #3841
Assignees
Labels
hint-category:pwa msft-consider Issues being considered for contributions from Microsoft type:bug
Milestone

Comments

@tmb-github
Copy link

tmb-github commented May 31, 2020

The commit that added support for maskable is c5eb40f#diff-07d179b4d8516fb968d006563b37bfe3

Unfortunately, as @amazingcaio it does not allow for combinations:

I don't think this issue is fixed. "purpose" can have multiple valid values, so "any maskable" and "maskable any", and (I suppose) even "any maskable badge" should also be accepted. I think the real issue here is, because the possible options are hardcoded you'd have to have all 15 different possibilities (cos you have to treat "any maskable badge" and "ask badge maskable" as different, for instance). I reckon having to type 15 different possible options is fine, but it gets more complicated as more values are added to the web manifest spec. If one more value is added, for instance, there would be 60 different possible combinations. So changing the code so you don't have to hardcode every option and disregard the order of the values (so "any maskable" and "maskable any" would be considered the same) is the best solution.

Need to find a way to enable this in the schema and our types.

Note: Original issue text by @tmb-github is available in the history

@molant molant transferred this issue from webhintio/webhintio.github.io Jun 1, 2020
@molant
Copy link
Member

molant commented Jun 1, 2020

Looks like we will have to update the manifest, can't remember if we update them automatically on release or if we just copied from schema store, but more reason to do #3413 to automate this.

@tmb-github do you know if Firefox supports it as well?

@tmb-github
Copy link
Author

@molant Sadly, I don't. I just learned about this last week with the most recent version of Chrome Canary.

@tmb-github
Copy link
Author

@molant It turns out Firefox Preview does support maskable icons...see these articles:
https://css-tricks.com/maskable-icons-android-adaptive-icons-for-your-pwa/
https://tigeroakes.com/projects/mozilla-firefox/

@molant
Copy link
Member

molant commented Jun 8, 2020

I'm looking at the commit history of our schema and maskable was added back in January 🤔

Looking at the original issue it looks like @amazingcaio disagreed with the resolution and correctly points out that we don't support combinations which seems to be your case (any maskable). Not sure why I didn't see the comment earlier 😓

Maybe there's something we can do with json schema to enable this (and find something similar in TypeScript).

I'll update the title and description of the issue.

@hxlnt is this something your team might want to get considered?

@molant molant changed the title Chrome Lighthouse 6.0.0 now audits for maskable icon, which webhint rejects web manifest does not support combination of values for icon purpose Jun 8, 2020
@tmb-github
Copy link
Author

@molant I can no longer see my original message (it was apparently clipped when the topic was transferred). FWIW, the article referenced by the Lighthouse audit is: https://web.dev/maskable-icon-audit/ which suggests that users "Set the value of purpose to maskable or any maskable. See Values." And "Values" links to https://developer.mozilla.org/en-US/docs/Web/Manifest/icons#Values, which only lists 3 possible purposes. Are there more purposes than those listed in this spec?

@molant
Copy link
Member

molant commented Jun 8, 2020

@tmb-github you can access the original one by pressing on "edited by @molant":

image

The current issue is that you have any maskable and we only support any or maskable, not both at the same time 🤦‍♂️

@hxlnt hxlnt added the agenda label Jun 8, 2020
@Malvoz
Copy link
Member

Malvoz commented Jun 10, 2020

Just a heads-up, when w3c/manifest#833 is merged we'll have to support monochrome too.

@hxlnt hxlnt added the msft-consider Issues being considered for contributions from Microsoft label Jun 12, 2020
@hxlnt hxlnt added this to the 2006-2 milestone Jun 12, 2020
sarvaje pushed a commit to sarvaje/hint that referenced this issue Jun 24, 2020
sarvaje pushed a commit to sarvaje/hint that referenced this issue Jun 24, 2020
sarvaje pushed a commit to sarvaje/hint that referenced this issue Jun 24, 2020
sarvaje added a commit to sarvaje/hint that referenced this issue Jun 24, 2020
@hxlnt hxlnt modified the milestones: 2006-2, 2007-1 Jun 26, 2020
sarvaje added a commit to sarvaje/hint that referenced this issue Jun 29, 2020
@hxlnt hxlnt removed the agenda label Jul 10, 2020
@hxlnt hxlnt modified the milestones: 2006-2, 2007-2 Jul 10, 2020
antross pushed a commit that referenced this issue Jul 27, 2020
Fix #3791
Close #3841

Co-authored-by: Antón Molleda <[email protected]>
Co-authored-by: Caio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment