Closed Bug 975535 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v1.3 wontfix, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 2.0+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: pla, Assigned: sjochimek)

References

Details

(Whiteboard: ux-tracking, visual design, visual-tracking, bokken [systemsfe])

Attachments

(11 files, 2 obsolete files)

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.
Target Milestone: --- → 1.4 S2 (28feb)
Full icon sets for homescreen applications and settings > app permissions exported @1x, 1.5x, and 2x resolutions.
Attached image 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
Sam, can you work on this now?
Assignee: nobody → sjochimek
blocking-b2g: --- → 1.4+
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.
VD refresh issues do not block the release. Renom.
blocking-b2g: 1.4+ → 1.4?
Added 2.25x size icons to the set for both Homescreen App icons and Settings > App Permissions icons.
Attachment #8381676 - Attachment is obsolete: true
Attached file 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?
Flags: needinfo?(pla)
Target Milestone: 1.4 S2 (28feb) → 1.4 S3 (14mar)
blocking-b2g: 1.4? → backlog
I have updated the PR with 2.25x icons.
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
Flags: needinfo?(pla)
This screenshot shows the old Marketplace icon used on the homescreen.
This screenshot shows the issues in App Permissions.
Flags: needinfo?(sjochimek)
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/'.
Flags: needinfo?(sjochimek)
Attachment #8383546 - Flags: feedback?(pla)
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.
Attachment #8383546 - Flags: feedback?(pla) → feedback-
This is the screenshot that shows only 'Homescreen' and 'System' icons being fuzzy.  The others look perfect.
Comment on attachment 8383546 [details] [review]
PR

Looks good now!
Attachment #8383546 - Flags: feedback- → feedback+
Attachment #8383546 - Flags: review?(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
Attachment #8383546 - Flags: review?(crdlc) → review-
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
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
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.

(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
Peter, is it possible to update those collection icons as well ?
Flags: needinfo?(pla)
Target Milestone: 1.4 S3 (14mar) → 1.4 S4 (28mar)
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.
Flags: needinfo?(sjochimek)
Flags: needinfo?(ran)
Flags: needinfo?(pla)
Flags: needinfo?(crdlc)
Flags: needinfo?(amirn)
Here is the link: https://github.com/mozilla-b2g/gaia/tree/master/apps/homescreen/collections
Flags: needinfo?(sjochimek)
Flags: needinfo?(ran)
Flags: needinfo?(crdlc)
Flags: needinfo?(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.
Flags: needinfo?(pla)
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
Flags: needinfo?(pla)
Attached file E.me follwups patch (obsolete) —
updated the E.me patch.

Sam, can you please update the PR?

Thanks.
Attachment #8392964 - Flags: review?(ran)
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 icon@xx.png in separate folders.

Flagging you for needinfo as a notification only.

Let me know if you need anything else,
Peter.
Flags: needinfo?(sjochimek)
the pr is updated and travis is green.
Flags: needinfo?(sjochimek)
Attachment #8383546 - Flags: review- → review?(crdlc)
Comment on attachment 8383546 [details] [review]
PR

LGTM, Great work! Thanks a lot mate
Attachment #8383546 - Flags: review?(crdlc) → review+
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.
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.
Flags: needinfo?(sjochimek)
Hopefully Cristian will have an idea :)
Flags: needinfo?(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
Flags: needinfo?(crdlc)
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.
Flags: needinfo?(sjochimek) → needinfo?(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.
Flags: needinfo?(amirn)
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?
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 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
Attachment #8383546 - Flags: review+ → review?(crdlc)
(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.
> 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?
(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.
And why do we need to increase the size of the canvas?
Flags: needinfo?(sjochimek)
I needed to increase it cause the shadow was increased too.
Flags: needinfo?(sjochimek)
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?
and why not is increased the padding for iconable bookmarks?
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.
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.
Patryk and/or Peter, please respond to comment #49 ASAP.
Flags: needinfo?(pla)
Flags: needinfo?(padamczyk)
@Gregor - my last comment to Sam should resolve all the problems - waiting on Sam.
Flags: needinfo?(pla)
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 :)
Flags: needinfo?(padamczyk)
Attachment #8392964 - Attachment is obsolete: true
Attachment #8392964 - Flags: review?(ran)
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 on attachment 8383546 [details] [review]
PR

Good job!! 10x
Attachment #8383546 - Flags: review?(crdlc) → review+
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.
the pr is updated.
Flags: needinfo?(ran)
Attachment #8383546 - Flags: review?(ran) → review+
Flags: needinfo?(ran)
merged https://github.com/mozilla-b2g/gaia/commit/641aff9d2f54778bf429ac6597d2e79e70f0bfd4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
(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
Depends on: 989876
due to Anthony's comment, reopening this for Sam's review.
Status: RESOLVED → REOPENED
Flags: needinfo?(sjochimek)
Resolution: FIXED → ---
Blocks: 989731
Duplicate of this bug: 981940
Attached file 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]:
Attachment #8404783 - Flags: approval-gaia-v1.4?
(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 ?
Attachment #8404783 - Flags: approval-gaia-v1.4? → approval-gaia-v1.4+
Comment on attachment 8404783 [details] [review]
PR for v1.4

removing approval for 1.4 as this is needed for 2.0
Attachment #8404783 - Flags: approval-gaia-v1.4+ → approval-gaia-v1.4-
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
(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.
Sorry if this wasn't clear, but this visual refresh (already vastly de-scoped) is required.
We forgot to upstream the marketplace icon changes to the fireplace Repo. :cvan is helping out.
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?
Flags: needinfo?(pla)
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
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 :)
Attachment #8410660 - Flags: review?(felash)
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 ? :)
Attachment #8410660 - Flags: review?(felash) → review+
(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.
blocking-b2g: backlog → 2.0?
blocking-b2g: 2.0? → 2.0+
Clearing the ni? for Sam because he's no longer on this.
Flags: needinfo?(sjochimek)
Flagging Candice for bug reassignment.
Flags: needinfo?(cserran)
Flags: needinfo?(cserran)
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.
Flags: needinfo?(mhenretty)
We got them in bug 1025299.
Flags: needinfo?(mhenretty)
Flags: needinfo?(pla)
You need to log in before you can comment on or make changes to this bug.