802792 - [App Icons] Are being shrunken down by a few pixels.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect, P3)

Description

[Patryk Adamczyk [:patryk] UX]

Hey so I think creating the shrinking rule we discussed is not going to work for our native icons. The "breaking out of the frame" concepts forces them of them to be off center, so close cropping them won't work as you'd need to offset a few spec icons. 

Can we add a flag in the code, if an icon name contains a certain string they don't be affected by the shrinking? 
ie. gallery~*.png
if the file name contains "~*" don't shrink?

Comment 1

[Patryk Adamczyk [:patryk] UX]

(In reply to Patryk Adamczyk [:patryk] UX from comment #0)
> Hey so I think creating the shrinking rule we discussed is not going to work
> for our native icons. The "breaking out of the frame" concepts forces them
> of them to be off center, so close cropping them won't work as you'd need to
> offset a few spec icons. 
> 
> Can we add a flag in the code, if an icon name contains a certain string
> they don't be affected by the shrinking? 
> ie. gallery~*.png
> if the file name contains "~*" don't shrink?

er... "The "breaking out of the frame" concept forces many of them to be off center"

Comment 2

[Ben Francis [:benfrancis]]

I really don't think we should be adding special hacks into our code to let our icons be bigger than everyone else's! That's not very Mozilla.

I understand that square icons of the same size as our circular icons may appear larger, but if we add a special case for our own icons that disadvantages third party circular icons!

Is there really no way we can find a set of resizing rules which can be consistently applied in all cases? If our icons are going to look slightly smaller because they're asymmetrical and need extra padding then so be it, we should be designing our icons within the same constraints as everyone else!

Is it not enough to issue icon guidelines to the marketplace saying that square icons should have extra padding to make them look consistent next to circular icons of the same size?

Comment 3

[Patryk Adamczyk [:patryk] UX]

This scaling rule would only apply to icons that don't follow the rules. The alternative would be to do what apple does by forcing a shape crop. Which we wanted to avoid to give 3rd party devs some freedom.
So the rules should be:

If an icon is less than 52px, scale to 52px
If an icon is more than 57px, scale to 54px
Don't scale the icons which are loaded on the device.

Most icons will be rectangular in shape. And we'll give ppl assets to use to create circular icons which are 57x57.

Comment 4

[Ben Francis [:benfrancis]]

I discussed this with Vivien in IRC who feels there's no good engineering reason to add a special case into our code for our own icons, to solve a UX problem. I think I agree with him.

As Vivien would have to approve this patch for it to land I suggest we look for another solution.

How does it look if we crop our icons to the least padding possible without them being off-centre, then resize all oversized icons to the same size? It will be possible for someone to create a larger looking square icon by using all the available pixels if we don't enforce guidelines for padding square icons, but it seems like the right thing to do from an engineering point of view.

Having added lots of favicons to the homescreen from the browser which have no padding based on the currently implemented resizing rules, it doesn't look as bad as I thought it would.

Comment 5

[Patryk Adamczyk [:patryk] UX]

Can you attach or send me a screenshot 

So I think there are 4 issues. We can probably agree on first 2, and the 3rd is where we are disagreeing on, so perhaps that has be another bug.

1. If an icon is too small, less than 52px. Scale it to 52px. 
2. If its a bookmark / favicon scale it to 32px and add the generic gray plate.

3. If the icon is bigger than 57px... 

So we have a few icons, email comes to mind, where when centered, the top of the icon occupies the full 64x64 canvas. As an FYI the icons are just not centered within the plate, the base of the icon sits on the same vertical axis as the rest of the circular icons, so they look uniform.

We don't want our icons to be visually bigger than the other app icons, we just want them to be balance.

Comment 6

[Ben Francis [:benfrancis]]

(In reply to Patryk Adamczyk [:patryk] UX from comment #5)
> We don't want our icons to be visually bigger than the other app icons, we
> just want them to be balance.

I completely understand and it's a tricky problem, but in our view it's a visual design problem which needs a visual design or marketplace policy solution, not an engineering one.

I know I'm being a massive pain and it's unfortunate but these design constraints have always existed, it's just that they've become more visible now that we've actually started resizing icons to a uniform size rather than just displaying whatever is provided (which is clearly not a workable solution).

We shouldn't put hacks in the code which have the side effect of disadvantaging third party brands. People will notice and it will reflect badly on us.

Screenshot coming up when I can stop my phone from crashing! :)

+clee +jcarpenter

Comment 7

[Patryk Adamczyk [:patryk] UX]

Hey Ben, so we did alot of tests, and created a spec. We did a work around the "flag" issue. We've modified the canvas size to 60x60 px from 64x64px

So here are the rules.
1. If an icon is too small, less than 60px. Scale it to 60px. 
2. If its a bookmark / favicon scale it to 32px and add the generic gray plate.
3. If the icon is bigger than 60px... scale it to 60px.

In our icon guides that we're in the process of publishing, we gave out recommendations on how icons should be created, but every icon will have a 60 x 60 px canvas size.

Comment 8

[Jason Smith [:jsmith]]

I don't understand the difference between this bug and bug 808152. Can someone clarify?

Comment 9

[Peter La]

@jsmith

This bug refers to the code that resize icons that are either too large or too small.  Currently the code shrinks the icons too much.  I have a fix ready to be checked in for tomorrow, so I'll take this issue.

The other bug (bug 808152) is about resizing the actual icon images, and is a visual design fix.  These two bugs will work in conjunction to line up the icon size and resizing behaviour with the new app icon spec.

Comment 10

[Peter La]

Attachment: https://github.com/mozilla-b2g/gaia/pull/6340

https://github.com/mozilla-b2g/gaia/pull/6340

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 680268 [details]
https://github.com/mozilla-b2g/gaia/pull/6340

Are you trying to merge two bug patches in one pull request?

Please correct your git branches, submit another pull request, then ask for review again.

Comment 12

[Ben Francis [:benfrancis]]

Ha, I just commented on the other bug asking that these be landed together as they are inter-dependent.

Either way the first commit needs amending with Tim's requested change.

Comment 13

[Peter La]

Attachment: Pull Request 6365

@timdream @benfrancis

I deleted my original branches, please ignore the old requests.
This should be clean now - 1 branch for image updates, 1 branch for .js file updates.

This change now changes both page.js and appmanager.js to resize to 60.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 680729 [details]
Pull Request 6365

Redirect the review to home screen app owner; Cristian, could you make sure that if those two changes is enough for changing default icon sizes? Thanks.

Comment 15

[Cristian Rodriguez (:crdlc)]

there are some comment on github 

https://github.com/mozilla-b2g/gaia/pull/6365#issuecomment-10320532

thanks

Comment 16

[Cristian Rodriguez (:crdlc)]

I've tested the pr and I have two comments, if you can implement them I can immediately put R+ ;)

thanks friends

Comment 17

[Peter La]

I've redone this change by syncing to latest version of master, creating one new branch (appicon_updateandresize) and making all changes to it (including the ones crdlc asked for + additional ones I found necessary), and submitted a new pull request to replace the previous 2.

Please review and approve pull request https://github.com/mozilla-b2g/gaia/pull/6537.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/b9d1d6f9b01661db17a9d932d614fd9a1d79a720