Closed
Bug 1388379
Opened 7 years ago
Closed 7 years ago
Activity Stream Top Sites: Update default icons (and pages?)
Categories
(Firefox for Android Graveyard :: General, enhancement, P1)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: sebastian, Assigned: mcomella)
References
Details
(Whiteboard: [MobileAS] 1.28)
Attachments
(8 files, 2 obsolete files)
58.42 KB,
image/png
|
Details | |
269.60 KB,
image/png
|
Details | |
603.41 KB,
application/zip
|
Details | |
124.92 KB,
image/png
|
Details | |
96.30 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
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?
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Bryan, Aaron: Can you provide us with new icons? What did we do for iOS here?
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
Updated•7 years ago
|
Flags: needinfo?(abenson)
Comment 3•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
For precedent, bug 1216987 is when we changed to Alexa top 5 for suggestedsites.
See Also: → 1216987
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(michael.l.comella)
Assignee | ||
Updated•7 years ago
|
Iteration: --- → 1.28
Flags: needinfo?(michael.l.comella)
Whiteboard: [MobileAS] → [MobileAS] 1.28
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Comment 11•7 years ago
|
||
(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)
Reporter | ||
Comment 12•7 years ago
|
||
(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)
Assignee | ||
Comment 13•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
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)
Assignee | ||
Comment 15•7 years ago
|
||
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)
Updated•7 years ago
|
Iteration: 1.28 → 1.29
Comment 16•7 years ago
|
||
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
Assignee | ||
Comment 17•7 years ago
|
||
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)
Comment 18•7 years ago
|
||
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•7 years ago
|
||
I exported the icons in 6 different resolutions.
Attachment #8898453 -
Attachment is obsolete: true
Flags: needinfo?(bbell)
Assignee | ||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
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•7 years ago
|
||
Added Webmaker, Mozilla, and Support icons.
Attachment #8902000 -
Attachment is obsolete: true
Flags: needinfo?(bbell)
Assignee | ||
Comment 23•7 years ago
|
||
Assignee | ||
Comment 24•7 years ago
|
||
Note: this is on a tablet before we made tablet changes, bug 1352342.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
(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)
Assignee | ||
Comment 29•7 years ago
|
||
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.
Assignee | ||
Comment 30•7 years ago
|
||
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.
Assignee | ||
Comment 31•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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 33•7 years ago
|
||
mozreview-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 34•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
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
![]() |
||
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f9d5ed89257 https://hg.mozilla.org/mozilla-central/rev/af95b6471444 https://hg.mozilla.org/mozilla-central/rev/57e21bf28387
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Assignee | ||
Updated•7 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•