975535 - [VsD Refresh] Update App Icons in Homescreen & Settings/App Permissions

Status: RESOLVED FIXED

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

Description

[Peter La]

This is a reduced scope change for homescreen that replaces what was previously planned for homescreen in 1.4.

It amounts to just swapping in the new app icon design + a couple of very minor css tweaks.

The scope of this change is exactly the following:

1) Swap in new app icon bitmaps for homescreen (image replacements)
2) Swap in new app icon bitmaps for settings > app permissions (image replacements)
3) Adjust the drop shadow css for app icons in homescreen
4) Adjust the drop shadow css for app labels in homescreen (this may not be necessary, will post in the spec our decision here)

Final set of icons + spec to be provided EOD Tuesday, February 25.

Comment 1

[Peter La]

Attachment: Icons! (Homescreen App Icons & Settings App Permissions)

Full icon sets for homescreen applications and settings > app permissions exported @1x, 1.5x, and 2x resolutions.

Comment 2

[Peter La]

Attachment: App Icon Refresh Spec

Please follow this spec for:

1) App icon drop shadow
2) App icon text label drop shadow

To reiterate, there are no layout changes, and no changes to any other UI elements except for the app icons on homescreen and settings > app permissions, plus the 2 drop shadow changes (app icon & text label).

Please contact me if you have any questions, thanks!
Peter

Comment 3

[Gregor Wagner [:gwagner]]

Sam, can you work on this now?

Comment 4

[Peter La]

FYI - Sam/Gregor

I forgot to upload a 2.25x size for Madai.  I will do that today ASAP.

Sam - for the app permissions icons, I wasn't sure what the current naming scheme is in the build, so I just created one - feel free to change these to line up with how they are stored in the build.

Comment 5

[Jason Smith [:jsmith]]

VD refresh issues do not block the release. Renom.

Comment 6

[Peter La]

Attachment: Icons! (Updated with 2.25x Size)

Added 2.25x size icons to the set for both Homescreen App icons and Settings > App Permissions icons.

Comment 7

[Sam Joch [:samjoch]]

Attachment: PR

Peter, can test if that fit your needs?

Should i include all 2.25x because that means i will to update all the app manifest files also?

Comment 8

[Sam Joch [:samjoch]]

I have updated the PR with 2.25x icons.

Comment 9

[Peter La]

Hi Sam,

I have reviewed the patch on both an Inari and on my Nexus 4 device, and the icons as well as drop shadows on the icons and labels look good.

There are just a couple of issues:

1) The marketplace icon is using the old one.  I think this is because it is actually defined in a text file as a string of hex values, and doesn't use the png image.  I forget which file it is in, but it's there.  For v1, I remember having to convert the png to a string of values and paste it into this text file.  You will likely also have to define the new sizes in this file.

2) In Settings > App Permissions, the 'Homescreen' app and 'System' app don't have the updated icons that I provided.

3) In Settings > App Permissions, can you remove the drop shadow from the icons here?

I will post some screenshots to illustrate the issues.

Thanks,
Peter

Comment 10

[Peter La]

Attachment: Review Image - Homescreen

This screenshot shows the old Marketplace icon used on the homescreen.

Comment 11

[Peter La]

Attachment: Review Image - Settings > App Permissions

This screenshot shows the issues in App Permissions.

Comment 12

[Sam Joch [:samjoch]]

Ok the PR is updated. I've updated System & Homescreen icons, i've removed shadow on 'Settings->App Permissions' and i've replaced marketplace icons inside application.zip in 'external-apps/marketplace.firefox.com/'.

Comment 13

[Peter La]

Comment on attachment 8383546 [details] [review]
PR

Hi Sam,

I just reviewed the patch on both a low-end (Inari) and a high-end device (Nexus 4).  Everything looks perfect on the Inari!  On the Nexus 4 everything is perfect except for 2 icons in App Permissions.  For some reason, it looks like it is grabbing a lower-res icon for 'Homescreen' and 'System' apps as they look a bit fuzzy compared to the others.  Did you put all 4 sizes of these icons into the build?  I will attach a screenshot.

This bug isn't super serious, but if there is a simple solution, it would be good to fix now rather than later.

Peter.

Comment 14

[Peter La]

Attachment: Screenshot showing fuzzy icons in App Permissions

This is the screenshot that shows only 'Homescreen' and 'System' icons being fuzzy.  The others look perfect.

Comment 15

[Peter La]

Comment on attachment 8383546 [details] [review]
PR

Looks good now!

Comment 16

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8383546 [details] [review]
PR

Hi,

   There are some important regressions:

1) Collection icons are not updated:

http://i.imgur.com/SY4MYWq.png

Please comment this issue with Ran and address the changes needed

2) Local apps inside collections are decapitated

http://i.imgur.com/DOebxIL.png

3) Go to dock, press and hold one icon and release this one without moving the finger. You can see an unexpected movement. It seems that you modify a margin-top without touching this rule in dragdrop.css

.draggable > div { 

4) Do the same for icons in the grid. You can see that labels don't behave correctly. It seems that you modify the margin for labels but nothing was done in draggable icons

Thanks a lot

Comment 17

[Ran Ben Aharon [:ranbena] EverythingMe]

Amir, we have a copy of the wallpaper icon here https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything.me/images/. Please PR samjoch.

Also, if we still do replicate the icon styling (shadow, size), we should update it as well. https://github.com/mozilla-b2g/gaia/pull/16732/files#diff-f009c0c3b8c39e5f5834393af6690a94R45

Comment 18

[Amir Nissim (:amirn)]

Sam, please see PR https://github.com/mozilla-b2g/gaia/pull/17126 which includes E.me followups.

It fixes #2 reported by Cristian and issues reported by Ran in Comment 17.

I created symlinks from homescreen to wallpaper to avoid image assets duplication.
Is this an acceptable solution?

commit:
https://github.com/EverythingMe/gaia/commit/5c1fc4bb5f853b5ab00a099a1cb915a85e8cd1d6

Comment 19

[Amir Nissim (:amirn)]

Comment on attachment 8383546 [details] [review]
PR

r? Ran for the required changes in E.me code:
https://github.com/samjoch/gaia/commit/a3095fa8ff6ab3d1e95c4fcfab1a8638799a731e

Comment 20

[Sam Joch [:samjoch]]

Cristian,

1. Collections icons: Those icons are not in the scope of this bug.
2. Draggable issues: I have updated the patch.

It seems that we still have issues in the e.me part.

Comment 21

[Cristian Rodriguez (:crdlc)]

(In reply to Sam Joch [:samjoch] from comment #20)
> Cristian,
> 
> 1. Collections icons: Those icons are not in the scope of this bug.

I know that collection icon design is out of this bug but I mean that I won't put r+ if we have a dialer icon on the grid different than the social collection icon shows (legacy one) for example

> 2. Draggable issues: I have updated the patch.

OK, thanks a lot

> 
> It seems that we still have issues in the e.me part.

I will review when this is addressed

Comment 22

[Sam Joch [:samjoch]]

Peter, is it possible to update those collection icons as well ?

Comment 23

[Peter La]

Hi Sam,

Yes I will update those collections ASAP.

Can someone on this bug point me to _ALL_ of those smart collection icon sets (the overlapping ones)?  I need to know exactly what to replace.  Also, I'm assuming I'll need to provide 1x, 1.5x, 2x, and 2.25x versions -- let me know otherwise.

Peter.

Comment 24

[Sam Joch [:samjoch]]

Here is the link: https://github.com/mozilla-b2g/gaia/tree/master/apps/homescreen/collections

Comment 25

[Amir Nissim (:amirn)]

Peter, will it be correct to assume the new icons:
1. have no padding (the old ones had 2px padding right?)
2. have no embedded shadow

is there any guideline for the new icons I can take a look at?

Thanks.

Comment 26

[Peter La]

Hi Amir,

The answer is 'yes' for both queestions.

You can see the spec for the new icons in the first attachment to this bug called 'App Icon Refresh Spec'.

Cheers,
Peter

Comment 27

[Amir Nissim (:amirn)]

Attachment: E.me follwups patch

updated the E.me patch.

Sam, can you please update the PR?

Thanks.

Comment 28

[Amir Nissim (:amirn)]

A reference to the updated patch:
https://github.com/EverythingMe/gaia/commit/44a48d357cb24cd88b547a5ed2427c97969e18dc

Comment 29

[Peter La]

Attachment: SmartCollectionsDefaultIconSets.zip

Hi Sam,

Attached are the default sets of Smart Collections icons (3 icons stacked), rendered in all sizes (1x, 1.5x, 2x, 2.25x).  The key size here for Madai in 1.4 is the 2.25x size, which is new - gaia doesn't have files for that size yet, but it has the other 3 sizes.

You will need to rename these files slightly as it was just easier for me to prefix them with the collection name.  In gaia the files are stored simply as [e-mail] in separate folders.

Flagging you for needinfo as a notification only.

Let me know if you need anything else,
Peter.

Comment 30

[Sam Joch [:samjoch]]

the pr is updated and travis is green.

Comment 31

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8383546 [details] [review]
PR

LGTM, Great work! Thanks a lot mate

Comment 32

[Ran Ben Aharon [:ranbena] EverythingMe]

Amir, your last commit fixed most bugs and it's looking good.
But now that we know that the Collection icon visual refresh is meant for 1.5, we should address the following:
The main PR made installed apps render smaller in the Collection icon. It's apparent in the default Music icon.

Comment 33

[Amir Nissim (:amirn)]

Attachment: screenshot of issue described in comment 32

Sam/Cristian - 
I can't get my head around this bug.

As far as I can tell it is not caused by E.me code since I didn't change anything in the scope of Collections icons.

Can you think of anything that might cause it?

I also noticed that pinned webapps' icons are rendered in the correct size:
1) open Social
2) long tap Facebook and choose 'Add to top of collection'
3) rearrange the apps above the line so that 'Facebook' is the first app
4) close Social
5) 'Social' icon is rendered with Facebook icon in the correct size.

Comment 34

[Amir Nissim (:amirn)]

Hopefully Cristian will have an idea :)

Comment 35

[Cristian Rodriguez (:crdlc)]

I don't have any idea sadly, icons on the homescreen haven't been changed for a long time ago. Maybe this change in the PR could be related to this issue:

https://github.com/mozilla-b2g/gaia/pull/16732/files#r10421112

Comment 36

[Sam Joch [:samjoch]]

I have an idea about the problem.
E.me use the renderedIcon icon of the app, that include all the transformation applied to it (Shadow).
E.me need to use the original blob icon.
(cf. https://github.com/samjoch/gaia/compare/bug-975535-vsd-refresh-update-app-icons-in-homescreen-settings-app-permissions-fixe-eme#diff-2a9a46da9e305ebaeca891eaaf0fd6bcL182)

I have a branch that work on the browser but not on the device.
Amir, Can you check it: https://github.com/samjoch/gaia/compare/bug-975535-vsd-refresh-update-app-icons-in-homescreen-settings-app-permissions-fixe-eme

I feel like this a not the scope of this bug, as well.

Comment 37

[Amir Nissim (:amirn)]

We used renderedIcon since the original icon is not exposed by the homescreen, and we wanted the icons to show exactly as they do on the homescreen.

It seems this is no longer practical since the rendered size is now a lot bigger (was 64px vs. 60px before this change and now 80px vs. 60px).

Exposing the original icon sounds right, but will require more changes on our side.
I think it will also introduce more risk in the homescreen app since there are several scenarios in which the icon reference should be updated. I am not familiar enough with this code, but Cristian surely is.

Comment 38

[Cristian Rodriguez (:crdlc)]

It is just for 1.5 (2.0) not for 1.4 so what's the problem? ev.me icon sizes? If this is the only issue you could land it and fill a follow up. 

(In reply to Amir Nissim (Everything.me) from comment #37)
> We used renderedIcon since the original icon is not exposed by the
> homescreen, and we wanted the icons to show exactly as they do on the
> homescreen.
> 
> It seems this is no longer practical since the rendered size is now a lot
> bigger (was 64px vs. 60px before this change and now 80px vs. 60px).
> 
> Exposing the original icon sounds right, but will require more changes on
> our side.
> I think it will also introduce more risk in the homescreen app since there
> are several scenarios in which the icon reference should be updated. I am
> not familiar enough with this code, but Cristian surely is.

No more blobs will be stored because as you know we have memory issues thought. I haven't gone deep on this code but this is called "Visual refresh" and adding more code in homescreen won't make sense. I mean if a visual refresh is to add some shadow to icons, borders, colors or whatever, this new look&feel should be displayed in smart collections as well thought. What is my misunderstanding?

Comment 39

[Amir Nissim (:amirn)]

I have no objections landing if it is fine with everyone.

(In reply to Cristian Rodriguez (:crdlc) from comment #38)
The problem is in the Collections icon (see screenshot https://bugzilla.mozilla.org/attachment.cgi?id=8394298) - the app icons in it are rendered too small because we now have 80x80 icons from the homescreen and not 60x60.

Comment 40

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8383546 [details] [review]
PR

and where is that code? is not possible to increase the size in collection icons as well? Frozen my review until I understand this issue

Comment 41

[Amir Nissim (:amirn)]

(In reply to Cristian Rodriguez (:crdlc) from comment #40)
> Comment on attachment 8383546 [details] [review]
> PR
> 
> and where is that code? is not possible to increase the size in collection
> icons as well? Frozen my review until I understand this issue

The problem with resizing is that the OS icons are 80px with 20px padding (10px top and bottom) while web apps icons do not have any padding.

The code is in the IconGroup module: https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything.me/js/helpers/IconManager.js#L99

Note: this module will be deprecated when #965711 lands.

Comment 42

[Cristian Rodriguez (:crdlc)]

> The problem with resizing is that the OS icons are 80px with 20px padding (10px top and bottom) while
> web apps icons do not have any padding

And could I add this padding to web apps as well?

Comment 43

[Amir Nissim (:amirn)]

(In reply to Cristian Rodriguez (:crdlc) from comment #42)
> 
> And could I add this padding to web apps as well?

You could, but it would be bad practice IMO which could result in visual artifacts for some icons.

Comment 44

[Cristian Rodriguez (:crdlc)]

And why do we need to increase the size of the canvas?

Comment 45

[Sam Joch [:samjoch]]

I needed to increase it cause the shadow was increased too.

Comment 46

[Cristian Rodriguez (:crdlc)]

No icon-able bookmarks are created here [1] as well

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/page.js#L329

Or are bookmamrks from Ev.me iconable?

Comment 47

[Cristian Rodriguez (:crdlc)]

and why not is increased the padding for iconable bookmarks?

Comment 48

[Peter La]

Hi Sam,

Since this drop shadow size change is causing so many problems, I propose we go back to the old drop shadow size but just reduce the opacity from 65% to 5%.

You won't really notice the drop shadow anymore on most wallpapers - but this 5% will make sure all the icons are still visible in the worst case scenarios.

This should solve all these layout problems and ramifications with E.Me.  The increased dropshadow size is not worth doing if it means we don't land the icon swap.

Thanks!
Peter.

Comment 49

[Gregor Wagner [:gwagner]]

Whats the status here? Do we solutions to all the problems we found here?
This was supposed to go into 1.4 and depending on the risk we will consider uplifting.

Comment 50

[Stephany Wilkes]

Patryk and/or Peter, please respond to comment #49 ASAP.

Comment 51

[Peter La]

@Gregor - my last comment to Sam should resolve all the problems - waiting on Sam.

Comment 52

[Gregor Wagner [:gwagner]]

Thanks Peter.
That's a confusing bug for me so I just want to get a summary of things we have to figure out here.
I see some open questions from Cristian. Sam, Cristian can you sync via IRC? I think that's faster than bugzilla :)

Comment 53

[Sam Joch [:samjoch]]

Comment on attachment 8383546 [details] [review]
PR

i removed all the js part to keep the shadow the same size.
Amir updated the e.me part.

The pr is updated and travis is green, the patch should be ready now.

Comment 54

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8383546 [details] [review]
PR

Good job!! 10x

Comment 55

[Amir Nissim (:amirn)]

Sam, can you please include this patch instead:
https://github.com/EverythingMe/gaia/commit/db233c3360c9a0eb090328baa4c24911f61c1756

It only contains the wallpaper files. The other regressions we found are not caused by this bug and we will file a new bug for fixing it.

Thanks.

Comment 56

[Sam Joch [:samjoch]]

the pr is updated.

Comment 57

[Sam Joch [:samjoch]]

merged https://github.com/mozilla-b2g/gaia/commit/641aff9d2f54778bf429ac6597d2e79e70f0bfd4

Comment 58

[Gabriele Svelto [:gsvelto] (PTO)]

I've noticed that the new icons in the PR have not been properly recompressed before landing. I'll open a follow up for that as it saves a significant amount of space.

Comment 59

[Amir Nissim (:amirn)]

(In reply to Amir Nissim (Everything.me) from comment #55)
> Sam, can you please include this patch instead:
> https://github.com/EverythingMe/gaia/commit/
> db233c3360c9a0eb090328baa4c24911f61c1756
> 
> It only contains the wallpaper files. The other regressions we found are not
> caused by this bug and we will file a new bug for fixing it.
> 
> Thanks.

followups: bug 989731, bug 989983

Comment 60

[jachen]

due to Anthony's comment, reopening this for Sam's review.

Comment 62

[Sam Joch [:samjoch]]

Attachment: PR for v1.4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): a vsd refreshed
[User impact] if declined: user will see old icons
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky): Mostly assets updates or color changes, no logic is change here.
[String changes made]:

Comment 63

[bhavana bajaj [:bajaj]]

(In reply to Sam Joch [:samjoch] from comment #62)
> Created attachment 8404783 [details] [review]
> PR for v1.4
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): a vsd refreshed
> [User impact] if declined: user will see old icons
> [Testing completed]:
> [Risk to taking this patch] (and alternatives if risky): Mostly assets
> updates or color changes, no logic is change here.
> [String changes made]:
Sam, regarding testing completed, can you confirm the changes looked good on 2.0 to ensure correctness ?

Comment 64

[Preeti Raghunath(:Preeti)]

Comment on attachment 8404783 [details] [review]
PR for v1.4

removing approval for 1.4 as this is needed for 2.0

Comment 65

[Jason Smith [:jsmith]]

This is already landed patch, so this should be closed out. There's followups already on file to track the remaining work that needed to be fixed.

Comment 66

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #64)
> Comment on attachment 8404783 [details] [review]
> PR for v1.4
> 
> removing approval for 1.4 as this is needed for 2.0

No, that's not right. There was a product commitment towards partners that they strongly desired the refresh for 1.4. Because of 1.4 scope cuts, this was cut from the blocker list, but it was still strongly desired for the release to help meet the product commitment originally desired.

Comment 67

[Stephany Wilkes]

Sorry if this wasn't clear, but this visual refresh (already vastly de-scoped) is required.

Comment 68

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Upstream Icon changes to Marketplace

We forgot to upstream the marketplace icon changes to the fireplace Repo. :cvan is helping out.

Comment 69

[Michael Henretty [:mikehenrty][:mhenretty]]

Peter, I noticed when adding the Marketplace Icons to the marketplace repo that we are updating the following icon sizes:
60x60
90x90
120x120

but not:
128x128
256x256

Is this on purpose? Are those icons unused? I also noticed in the zip file you attached that we have a 135x135 icon we aren't adding to the marketplace. Is this all expected?

Comment 70

[Michael Henretty [:mikehenrty][:mhenretty]]

Ok, merged to v1.4 using the updated marketplace package. Thanks for the help :cvan!

v1.4: https://github.com/mozilla-b2g/gaia/commit/38997b9e876e38b20c0b1c09cf33a45aa44b2acf

Comment 71

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR, update marketplace with new icons

Julien, the earlier marketplace icon changes got overriden with the latest marketplace update. This pull request makes sure we are using the latest upstream version of marketplace which contains the icons. Please review :)

Comment 72

[Julien Wajsberg [:julienw]]

Comment on attachment 8410660 [details] [review]
Github PR, update marketplace with new icons

yep, looks fine, r=me :)

Does that mean that people on 1.3 updating the marketplace app will get the new icon, then ? :)

Comment 73

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] from comment #72)
> Does that mean that people on 1.3 updating the marketplace app will get the
> new icon, then ? :)

That's a good question. My guess would be yes, but not sure. In any case, I don't think this is too bad since the new icons are not so different that they will look out of place with the old ones.

Comment 74

[Michael Henretty [:mikehenrty][:mhenretty]]

Updated Marketplace Icons are back on master:

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

Comment 75

[Stephany Wilkes]

Clearing the ni? for Sam because he's no longer on this.

Comment 76

[Stephany Wilkes]

Flagging Candice for bug reassignment.

Comment 77

[Wil Clouser [:clouserw]]

comment 69 seems to have gone unanswered.  The Marketplace requires only a 128x128 icon to be listed, but it looks like it wasn't updated here.  That means the icon listing in the marketplace is incorrect.  Can we get 128x128 and 256x256 icons here please?  Thanks.

Comment 78

[Wil Clouser [:clouserw]]

We got them in bug 1025299.