1021496 - (vertical-homescreen) Implement bookmark app icon design from Classic Homescreen (favicons)

Status: VERIFIED FIXED

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

Description

[Peter La]

Attachment: FaviconsSpec.jpg

Implement the favicon bookmark design from Classic Homescreen (using Nearest Neighbour scaling).

See attached spec.

Comment 1

[Kevin Grandon :kgrandon]

We're not going to have anywhere near that quality of favicon, so I think we're going to have a much bigger background. I think we should count on only having a 16x16 favicon.

Comment 2

[Peter La]

Kevin,

So it's should scale the favicon up using nearest neighbour resampling, like it does in 1.4 and earlier.  This will give kind of an 8-bit pixelated look, but it's better than fuzziness.

The sizes spec'd are all multiples of 16.

Comment 3

[Jason Smith [:jsmith]]

Moving to VH next since we won't stop ship of the release with this issue. If it's implemented in time, then we'll get approval.

Comment 4

[Stephany Wilkes]

This should block 2.0 because it supports bookmarks, which is an important feature, and it looks terrible and some users can't see it well. Wouldn't this also perhaps count as a 1.4 regression if it doesn't carry over properly?

Comment 5

[Stephany Wilkes]

To qualify my earlier comment: If bookmarks look broken, that’s pretty bad particularly because Haida is about blurring the distinction between apps and webpages on the Homescreen. So it makes the pre-Haida distinction obvious, which is the opposite of one of Haida's goals.

Comment 6

[Jason Smith [:jsmith]]

I'm not sure I understand proof of the impact of here. What's the user impact here if we don't fix this? We need a screenshot showing why the existing design doesn't work.

This is asking for a new feature request, which needs approval from product.

Comment 7

[Peter La]

(In reply to Jason Smith [:jsmith] from comment #6)
> I'm not sure I understand proof of the impact of here. What's the user
> impact here if we don't fix this? We need a screenshot showing why the
> existing design doesn't work.
> 
> This is asking for a new feature request, which needs approval from product.

Hi Jason, I'm attaching a screenshot to show you how it looks currently.  It's not pretty.

Comment 8

[Peter La]

Attachment: Screenshot.png

Note the icons for The Verge, Gamespot, and StarCraft II websites.  These should be crisp and placed on a circular background as shown in the spec attached to this bug.

Comment 9

[Peter La]

This is a straight up regression from V1 and is a must fix.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Peter La from comment #9)
> This is a straight up regression from V1 and is a must fix.

Okay - that screenshot looks horrible. Agreed on the decision now after seeing the screenshot.

Comment 11

[Peter La]

\o/ Thank you so much. :)

Comment 12

[Johan Lorenzo [:jlorenzo]]

Marked as 2.0? blocker as bad UX.

Comment 13

[Candice Serran (:cserran)]

Hey Diego,

Thank you for helping us on homescreen. Would you be able to help us out and take a look at this bug?

Comment 14

[Hema Koka [:hema]]

Diego mentioned he started looking into this in todays standup. Assigning to him.

Comment 15

[Candice Serran (:cserran)]

(In reply to Hema Koka [:hema] from comment #14)
> Diego mentioned he started looking into this in todays standup. Assigning to
> him.

Awesome...thanks Diego and Hema!

Comment 16

[Kevin Grandon :kgrandon]

Attachment: Assets for bookmark circle

Comment 17

[Kevin Grandon :kgrandon]

Any progress here? If not no worries, but let's unassign it so someone can pick this up over the weekend if you can't get to it. Thanks.

Comment 18

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

The patch overrides _decorateIcon for bookmarks. It paints the icon circle as a background of the favicon that now seats in the middle. I feel I have not spend enough time polishing and testing the patch to mark if for review. I can jump  on this again on Monday but feel free to take it from here if you want to get it done any earlier.

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8440259 [details] [review]
Pull Request

Nice work. I will take a look at the patch today. I wouldn't worry too much about testing as long as you think it looks good. I'll also probably try to create some simple reftests in the next week or two. The bookmark icon would be a good candidate for those.

Comment 20

[Diego Marcos [:dmarcos]]

Updated PR to prevent anti aliasing of the favicon

Comment 21

[Kevin Grandon :kgrandon]

Diego - this is awesome! I think it's looking really good. The only thing that is a bit tricky is that currently the search and collections app leverage the bookmark item type to process images. This means that if you open a smart collection with your patch, they aren't taking up the full icon space like they should.

One thing that both of these places do though is pass the 'clipIcon' attribute[1] into the object, so perhaps we could detect on that and pass-through to the parent. Also I think we're looking for this for Monday morning, so perhaps I will get to your patch and take it over if I don't see you on IRC before then.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/items/grid_item.js#L188

We do need tests for this stuff which would've caught it on travis. I'm looking into creating some reftests for these use-cases to bullet proof them for the future.

Comment 22

[Diego Marcos [:dmarcos]]

I cannot find an example where Bookmark.prototype._decorateIcon gets called except when bookmarking from the browser. Searching on the homescreen or creating smart collections don't seem fall through the bookmark item. Do you have any example so I can understand?

Comment 23

[Kevin Grandon :kgrandon]

Yeah, it's currently these places:

https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/js/view_apps.js#L52
https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/providers/webresults.js#L60

I guess there are three different ways to render an Icon now, it may make sense to pass in some "icon strategy" and offload the rendering into a different class. The three ways are:

- Clipped, for web results/e.me
- Centered, for homescreen bookmarks
- Default, for apps and marketplace results.

One thing that makes this a bit tricky is that we've been abusing the Bookmark class for more than we should probably. We could use this as an opportunity to clean that up, or continue abusing the detail property.

Comment 24

[Kevin Grandon :kgrandon]

Attachment: Hijacked pull request

Thanks for the great work on this Diego - really appreciate you taking the time to look at this over the weekend. I hope you don't mind me taking over and making some adjustments to your work. (Normally I would love for you to finish, but we are in a huge rush and need to land asap. If you see something wrong with this, or it needs work tomorrow I'm more than happy for you to take it.)

Flagging a few people for review here, please take a look if you have time.

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8440424 [details] [review]
Hijacked pull request

And also add James as a reviewer because he may be interested in this.

Comment 26

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8440424 [details] [review]
Hijacked pull request

LGTM and great work as usual. Just a comment, could we add the strategies as constants:

GridIconRenderer.TYPE.STANDARD, CLIP, etc...

if we have visibility of that object in the renderer definitions of course, otherwise forget my comment

Comment 27

[Amir Nissim (:amirn)]

Comment on attachment 8440424 [details] [review]
Hijacked pull request

Looks good, things I noticed:

1. I suggest adding error handling to the rendering functions (add reject/catch to the promises). Maybe there should be a fallback to a default icon in case of a failure instead of rendering nothing.

2. Bookmark icons saved from Collections are not rounded (open collection, long tap webresult, save to homecreen).

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8440424 [details] [review]
Hijacked pull request

I've gotten feedback from others, so I feel comfortable going with Cristian's review for now.

Comment 29

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/4ef3aaf1552509688b361d488ece2f7d44150639

Comment 30

[Kevin Grandon :kgrandon]

Comment on attachment 8440424 [details] [review]
Hijacked pull request

This is needed for the vertical homescreen. We've done our best at testing this patch, and believe it should be safe for uplift. Thanks!

Comment 31

[Stephany Wilkes]

Removing b2g blocking nom from Amy only because this is already fixed, marked for uplift, and noted as feature-b2g. This one is my fault in not clarifying flag use.

Comment 32

[Kevin Grandon :kgrandon]

A one-liner follow-up, was landed as well: https://github.com/mozilla-b2g/gaia/commit/13d6253c9b000c501c364072888fd51d90373f87

Please uplift that if possible, it's for testing only and NBOTB, but will make our lives easier.

Comment 33

[Kevin Grandon :kgrandon]

Uplifted:
https://github.com/mozilla-b2g/gaia/commit/546d06c5128d6d7f5113512e2b0707256469d8e0
https://github.com/mozilla-b2g/gaia/commit/459e69fa584e8384004b5783f169e10eb36f3c4a

Comment 34

[Lancy(Leave from Mozilla)]

Attachment: Verify_1021496.png

This issue has been verified successfully on Flame 2.0& 2.1.
See attachment: Verify_1021496.png
Reproducing rate: 0/10

Flame v2.0 version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/29222e215db8
Build-ID        20141203000201
Version         32.0

Flame v2.1 version:
Gaia-Rev        dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebce587d2194
Build-ID        20141203001205
Version         34.0