Closed Bug 1021496 Opened 6 years ago Closed 5 years ago
(vertical-homescreen) Implement bookmark app icon design from Classic Homescreen (favicons)
262.51 KB, image/jpeg
398.96 KB, image/png
18.97 KB, application/zip
46 bytes, text/x-github-pull-request
|Details | Review|
46 bytes, text/x-github-pull-request
|Details | Review|
267.03 KB, image/png
Implement the favicon bookmark design from Classic Homescreen (using Nearest Neighbour scaling). See attached spec.
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.
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.
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.
QA Whiteboard: [VH-FL-blocking-] → [VH-FL-blocking-][VH-FC-blocking-]
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?
feature-b2g: --- → 2.0
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.
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.
(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.
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.
This is a straight up regression from V1 and is a must fix.
(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.
\o/ Thank you so much. :)
Marked as 2.0? blocker as bad UX.
blocking-b2g: --- → 2.0?
Hey Diego, Thank you for helping us on homescreen. Would you be able to help us out and take a look at this bug?
Diego mentioned he started looking into this in todays standup. Assigning to him.
Assignee: nobody → dmarcos
(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!
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.
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.
Attachment #8440259 - Flags: feedback?(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.
Attachment #8440259 - Flags: feedback?(kgrandon) → feedback+
Updated PR to prevent anti aliasing of the favicon
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 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.  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.
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?
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.
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 on attachment 8440424 [details] [review] Hijacked pull request And also add James as a reviewer because he may be interested in this.
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
Attachment #8440424 - Flags: review?(crdlc) → review+
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 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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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!
Attachment #8440424 - Flags: approval-gaia-v2.0?(bbajaj)
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.
blocking-b2g: 2.0? → ---
Attachment #8440424 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
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.
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
You need to log in before you can comment on or make changes to this bug.