1388379 - Activity Stream Top Sites: Update default icons (and pages?)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

A fresh install of Firefox will show 5 top sites: Facebook, YouTube, Amazon, Wikipedia and Twitter.

The icons of those sites where made for our old top sites implementation and look really weird in the new layout (see attached screenshot). We should update them.

In addition to that 5 icons looks a bit weird. I wonder whether we should change that number?

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: default_top_sites.png

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Bryan, Aaron: Can you provide us with new icons? What did we do for iOS here?

Comment 3

[Chenxia Liu [:liuche]]

Maria to ping about where these icons would be sourced from.

Keeping in P2 list because we think it won't be that much engineering work once we have the icons.

Comment 4

[bbell]

Attachment: Icons.zip

icons

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

For a refresher on how suggested sites works, we create `suggestedsites.json` for each locale via generate_suggestedsites.py [1]. Here is the sample output on my locale build [2]. To fix this bug, we'll have to update the US assets in the tree, like [3]. Notes/questions:
- Current icons hug the icon content and fill the background color from the JSON - do we want to continue this pattern (e.g. for space reasons)?
- This output shows that we'll also need new icons for restricted profiles (webmaker, mozilla support, and sumo).
- How do we coordinate updating the suggested sites across locales?
- The icons from bbell are only one size, 360x360, so I wonder which dpi these are supposed to be in. The photon icons go all the way down to mdpi so I assume that we don't want to only use a higher dpi (e.g. bug 959203).

[1]: https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/generate_suggestedsites.py#6
[2]: https://gist.github.com/mcomella/d2710d2d58976dc69bd07272e0609882
[3]: https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile%2Fandroid+path%3Asuggestedsites+path%3Apng&redirect=false

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

For precedent, bug 1216987 is when we changed to Alexa top 5 for suggestedsites.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

So generate_suggestedsites.py will read a locales/.../chrome/region.properties file to load in the top sites [1]. The old icons hugged the content (e.g. [2], you should download locally because icon is white-on-white) so we centered the icons and used a color from region.properties to fill the background.

Sebastian, the icons provided in comment 4 have the inner content with proper padding and the background color provided - do you think there's any reason to keep the old conventions? 

One reason I can think of is that it'd be a pain to get all the locales to the new format.

[1]: https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/mobile/locales/en-US/chrome/region.properties#42
[2]: https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/mobile/android/app/src/photon/res/drawable-xxhdpi/suggestedsites_twitter.png

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #7)
> Sebastian, the icons provided in comment 4 have the inner content with
> proper padding and the background color provided - do you think there's any
> reason to keep the old conventions? 
> 
> One reason I can think of is that it'd be a pain to get all the locales to
> the new format.

Another reason could be that it more gracefully scales without artifacting - we can keep the raw icon the same size but scale the amount of padding/background based on the screen size.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

If we want to minimize artifacting from resizing in the top sites tiles, we'll need these icons in several different sizes:
- 135x135 px (hdpi: 1.5 x baseline)
- 180x180 px (xhdpi: 2 x baseline)
- 270x270 px (xxhdpi: 3 x baseline)

(I received icons at 300x300 px) This based off the assumption that the largest size we'll want to show the top sites tiles is 90x90 dp (which is the current values in the code but notably, they were set without tablet mocks: bug 1352342).

If we want to have a different largest size for tablets, we could either 1) have separate assets for tablet or 2) always downscale for phones (I'd prefer downscaling, if there are no obvious flaws, because we'll probably already downscaling anyway - see caveat).

Caveat: in practice, setting a best size image might not make much of a difference because our top sites tiles are always dynamically resized, with possible artifacting, if we cannot fit them at the max size.

NI self to follow-up with Bryan/Aaron once we figure out if it's worth keeping the old format or not (also, mention that we'll need the restricted assets as well).

---

Source of 90dp:

The largest tile size we currently support is 90dp (resource is accessed at [1] and eventually passed to TopSitesPageAdapter.onCreateViewHolder, which sets the TopSitesCard size with it [2]).

---

Note also: if we determine we want to keep the new style icons with the backgrounds, the fix for this bug is a simple asset swap.

[1]: https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java#76
[2]: https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topsites/TopSitesPageAdapter.java#102

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Flod, do you know how suggested sites icons get updated on Firefox for Android, or who I might follow-up with?

We're updating the formatting of the icons in this bug for the Activity Stream UI rewrite so we'll need to get our other locales to do a similar reformatting of their images. fwiw, the values associated with the icons are currently defined in region.properties [1].

[1]: https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/mobile/locales/en-US/chrome/region.properties#42

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Michael Comella (:mcomella) from comment #10)
> Flod, do you know how suggested sites icons get updated on Firefox for
> Android, or who I might follow-up with?

We never localized those, so we're shipping with English defaults (and, at this point, I'm really glad we did…).

The process simply doesn't scale, given the amount of work requested and who's going to see it (only fresh installs). If we want to do that (customize top sites), I'd suggest a centralized approach like we do for search (and fallback to en-US by default).
https://dxr.mozilla.org/mozilla-central/source/mobile/locales/search

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #7)
> Sebastian, the icons provided in comment 4 have the inner content with
> proper padding and the background color provided - do you think there's any
> reason to keep the old conventions? 

I would be okay with changing the convention. But I just remembered that afaik distributions can define their own suggested sites. We should at least make sure that we do not totally break them - although I'm not sure if we can make them "look good" with the new design.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Mike, wrt comment 12, would it be difficult/impossible to get our distributions to update their suggestedsites (default top sites) icons for 57? Would it trouble our distributors too much? If they're unable to update the icons, how bad would it be if we shipped anyway (which would misformat their default icons, kind of like how our default top sites are currently ugly)?

I can talk to UX about keeping the old convention but this pattern makes it possible for suggested sites to define their own padding.

Comment 14

[Mike Kaply [:mkaply]]

It shouldn't be a big deal to get folks to update their existing topsites assuming we have a spec for how their new tiles should look (currently we have them produce three different sizes).

Unfortunately we won't be able to do anything to update stuff that's already in the field.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Aaron, Bryan: according to the code, the maximum size our top sites tiles can be are 90 dp (analysis in comment 9). However, they're dynamically sized so they can be smaller than that.

Will we eventually want bigger images for tablet? If so, I think we should avoid the churn and include the larger asset here (especially because our distributions need to be told to update each time the asset format changes).

Also, a single Android asset is typically provided at multiple DPIs – can you provide the assets at the appropriate sizes (so I don't create artifacts resizing myself)? Suggested sites are currently using hdpi, xhdpi, xxhdpi which are 1.5 x baseline, 2 x baseline, and 3 x baseline respectively. For example:

with our 90 dp value (which is 90 px at the baseline), we'd want:
- 135x135 px (hdpi: 1.5 x baseline)
- 180x180 px (xhdpi: 2 x baseline)
- 270x270 px (xxhdpi: 3 x baseline)

Traditionally, you'd provide a separate set of assets for each of phone and tablet, so if we want a larger image for tablet top sites, we should provide a separate set of assets at these dpi.

Note: since our tiles are dynamically sized, having separate assets matters less because we might already be resizing them.

---

Note: I thought about using only xhdpi & xxhdpi assets but I figured I should follow the existing convention and we'd move to xhdpi/xxhdpi in one larger step, which is bug 959203.

Comment 16

[Chenxia Liu [:liuche]]

Regarding tablet icon size and whether we need others, here's a link to a screenshot of tablets currently (with some default icons): https://bug1352342.bmoattachments.org/attachment.cgi?id=8901925

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Spoke with bbell: he will provide the assets for phones in the three sizes at 90 dp (comment 15).

After thinking it over, I don't think we need to provide the tablet assets now - as per mkaply's comment 14, "It shouldn't be a big deal to get folks to update their existing topsites" - so let's just update the tablet assets if we need to add them in bug 1352342.

mcomella (NI) to follow-up for:
- A spec for distributions to update their assets
- Assets for restricted profiles, if still relevant

Comment 18

[Mike Kaply [:mkaply]]

For reference, here's what we tell partners:

https://wiki.mozilla.org/Mobile/Distribution_Files#Suggested_sites

We probably need a way to maintain the old assets if at all possible (since not everyone will be on 57 immediately)

Comment 19

[bbell]

Attachment: Top-sites.zip

I exported the icons in 6 different resolutions.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot: top sites in restricted profile

Bryan, it looks like we'll need assets for restricted profiles too: webmaker.org, support.mozilla.org, and mozilla.org. I've attached what this currently looks like in the new UI.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

fwiw, we only ship these in xhdpi at the moment, probably because they're infrequently used and we're trying to save space, so I'm fine shipping just the one size.

Comment 22

[bbell]

Attachment: Top-sites.zip

Added Webmaker, Mozilla, and Support icons.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot: post patch

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot post-patch restricted profile

Note: this is on a tablet before we made tablet changes, bug 1352342.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1388379: Rm xxxhdpi/suggestedsites_twitter.

This is the only suggested site in xxxhdpi and is thus inconsistent.

Review commit: https://reviewboard.mozilla.org/r/173470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173470/

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1388379: Use new suggestedsites assets for default profile.

Review commit: https://reviewboard.mozilla.org/r/173472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173472/

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1388379: Use new suggestedsites assets for restricted profile.

Review commit: https://reviewboard.mozilla.org/r/173474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173474/

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #17)
> mcomella (NI) to follow-up for:
> - A spec for distributions to update their assets

Moved to follow-up bug 1394641.

Also clearing NI for Maria because we've already been able to source the assets.

Comment 29

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Still TODO:
- rm unused properties of region.properties; update code not to use them
- Make sure code doesn't break without bg color (e.g. will we ever try to center the top sites favicon for being too small? What if we don't have the right BG color?)
- Update tests for ^
- Verify if l10n needs to be notified of these changes.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

From distributions follow-up bug:

(In reply to Mike Kaply [:mkaply] from bug 1394641 comment #2)
> The real problem is that distributions copy their files into the profile and
> they are used from that location.
> 
> So any existing users will always be using the old versions of files (even
> in activity stream).

It sounds like we can't actually remove the old code to display these distributions then; we should also write a test to make sure no one regresses this.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #30)
> It sounds like we can't actually remove the old code to display these
> distributions then; we should also write a test to make sure no one
> regresses this.

I'll handle updating the code (including removing/modifying legacy use cases) in the follow-up bug 1394641 because how distributions interacts with top sites icons greatly affects the code.

Comment 32

[Chenxia Liu [:liuche]]

Comment on attachment 8902065 [details]
Bug 1388379: Rm xxxhdpi/suggestedsites_twitter.

https://reviewboard.mozilla.org/r/173470/#review179284

Comment 33

[Chenxia Liu [:liuche]]

Comment on attachment 8902066 [details]
Bug 1388379: Use new suggestedsites assets for default profile.

https://reviewboard.mozilla.org/r/173472/#review179286

Swapping out new icons - looks great!

::: commit-message-f7b42:1
(Diff revision 1)
> +Bug 1388379: Use new suggestedsites assets for default profile. r=liuche

Just checking, did you pngcrush these?

Comment 34

[Chenxia Liu [:liuche]]

Comment on attachment 8902067 [details]
Bug 1388379: Use new suggestedsites assets for restricted profile.

https://reviewboard.mozilla.org/r/173474/#review179288

lgtm, same caveat of pngcrushing.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8902066 [details]
Bug 1388379: Use new suggestedsites assets for default profile.

https://reviewboard.mozilla.org/r/173472/#review179286

> Just checking, did you pngcrush these?

Yep! But I'll verify.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f9d5ed89257
Rm xxxhdpi/suggestedsites_twitter. r=liuche
https://hg.mozilla.org/integration/autoland/rev/af95b6471444
Use new suggestedsites assets for default profile. r=liuche
https://hg.mozilla.org/integration/autoland/rev/57e21bf28387
Use new suggestedsites assets for restricted profile. r=liuche

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4f9d5ed89257
https://hg.mozilla.org/mozilla-central/rev/af95b6471444
https://hg.mozilla.org/mozilla-central/rev/57e21bf28387