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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: mcomella)

Tracking

unspecified
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [MobileAS] 1.28)

Attachments

(8 attachments, 2 obsolete attachments)

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?
Bryan, Aaron: Can you provide us with new icons? What did we do for iOS here?
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
Flags: needinfo?(abenson)
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.
Flags: needinfo?(mpopova)
Posted file Icons.zip (obsolete) —
icons
Flags: needinfo?(bbell)
Assignee: nobody → michael.l.comella
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
For precedent, bug 1216987 is when we changed to Alexa top 5 for suggestedsites.
See Also: → 1216987
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
Flags: needinfo?(s.kaspari)
(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.
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
Flags: needinfo?(michael.l.comella)
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
Flags: needinfo?(michael.l.comella) → needinfo?(francesco.lodolo)
Flags: needinfo?(michael.l.comella)
Iteration: --- → 1.28
Flags: needinfo?(michael.l.comella)
Whiteboard: [MobileAS] → [MobileAS] 1.28
Priority: P2 → P1
(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
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(s.kaspari)
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.
Flags: needinfo?(mozilla)
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.
Flags: needinfo?(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.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
Iteration: 1.28 → 1.29
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
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
Flags: needinfo?(abenson) → needinfo?(michael.l.comella)
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)
Posted file Top-sites.zip (obsolete) —
I exported the icons in 6 different resolutions.
Attachment #8898453 - Attachment is obsolete: true
Flags: needinfo?(bbell)
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.
Flags: needinfo?(bbell)
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.
Posted file Top-sites.zip
Added Webmaker, Mozilla, and Support icons.
Attachment #8902000 - Attachment is obsolete: true
Flags: needinfo?(bbell)
Note: this is on a tablet before we made tablet changes, bug 1352342.
(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.
Flags: needinfo?(mpopova)
Flags: needinfo?(michael.l.comella)
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.
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.
(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 on attachment 8902065 [details]
Bug 1388379: Rm xxxhdpi/suggestedsites_twitter.

https://reviewboard.mozilla.org/r/173470/#review179284
Attachment #8902065 - Flags: review?(liuche) → review+
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?
Attachment #8902066 - Flags: review?(liuche) → review+
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.
Attachment #8902067 - Flags: review?(liuche) → review+
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.
Pushed by michael.l.comella@gmail.com:
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
Blocks: 1394641
No longer depends on: 1394641
You need to log in before you can comment on or make changes to this bug.