[Vertical Homescreen] Mozilla Homepage icon doesn't have background in smart collection icon

RESOLVED WONTFIX

Status

Firefox OS
Gaia::Everything.me
P2
normal
RESOLVED WONTFIX
4 years ago
2 years ago

People

(Reporter: amylee, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [2.0-VH-bug-bash][systemsfe][priority])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8449672 [details]
screenshot.png

Steps to reproduce:

1. Save the mozilla homepage to homescreen
2. Drag icon from homescreen to a smart collection. 
3. The mozilla icon is missing a background in the smart collection icon.

See screenshot
Does the icon look correct in the collection itself?
Blocks: 1015336
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?]
Keywords: qawanted
(Reporter)

Comment 2

4 years ago
(In reply to Jason Smith [:jsmith] from comment #1)
> Does the icon look correct in the collection itself?

It's hard to tell since the icon doesn't show up in the smart collections when I drag it in. See Bug 1033465.

Comment 3

4 years ago
My icons:

1) Show up as minis inside of/on top of the smart collection icon. I can see, for example, the tiny little Trimet icon (for PDX public transit) on/in the "Portland" smart collection icon. But...

2) Once I click to enter the "Portland" smart collection, the actual Trimet icon is not there.
(In reply to Amy Lee [:amylee] from comment #2)
> (In reply to Jason Smith [:jsmith] from comment #1)
> > Does the icon look correct in the collection itself?
> 
> It's hard to tell since the icon doesn't show up in the smart collections
> when I drag it in. See Bug 1033465.

Hi Amy,
   I was able to get the mozilla homepage to show up in the Entertainment Collection but that was the only collection I could get it to appear in. Even tried a creating a custom collection with no luck. However, YES the correct Mozilla homepage icon showed up with the background however in the Entertainment collection.
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Contact: croesch
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage?] → [VH-FL-blocking-][VH-FC-blocking?]
Flags: needinfo?(jmitchell)
Actually it does show up, but it looks extremely muted. You can see the 'M' in the attachment, the mozilla favicon is pretty muted, so not sure what we can do here. We'd probably have to give the smart collection some more visual treatment, perhaps darkening the background.

Can we have verification if the icon is any better in 1.4? I am fairly sure that we just copied the icon logic over, so I don't think it would be any different.
Blocks: 1017954
No longer blocks: 1015336
Adding QA Wanted to provide a comparison screenshot between 1.4 & 2.0.
Keywords: qawanted
(Reporter)

Comment 7

4 years ago
Created attachment 8450202 [details]
screen.png

Hi Kevin, 

Just to clarify, the icon should match the larger homescreen version (white circle background and no crop on the M). I've attached a screenshot for reference. Also, I'm sure it's probably been brought up before but can we do anything about the image quality of the icon? It's very pixelated. Thanks
Flags: needinfo?(kgrandon)
(In reply to Amy Lee [:amylee] from comment #7)
> Created attachment 8450202 [details]
> screen.png
> 
> Just to clarify, the icon should match the larger homescreen version (white
> circle background and no crop on the M). I've attached a screenshot for
> reference. Also, I'm sure it's probably been brought up before but can we do
> anything about the image quality of the icon? It's very pixelated. Thanks

I'd need to look into the code, but we'd need to see if we can provide a larger sized favicon on the main mozilla site as well to reduce pixelation.

The white background is specific homescreen treatment. If you open the mozilla site in a browser, you will see the favicon in the tab. For the inner icons in the circle we're rounding them, but not applying any other treatment. It would probably be possible to give icons a background as well, so transparent favicons (like this) would work better.

I'm going to ni? Amir from E.me to see what his thoughts are as he should be returning from PTO soon :)
Flags: needinfo?(kgrandon) → needinfo?(amirn)
Created attachment 8450374 [details]
2.0 and 1.4 comparison

Added comparison screenshot for Flame 2.0 and Flame 1.4. Flame 1.4 icon looks a lot better than what is seen in 2.0.
Flags: needinfo?(jmitchell)

Updated

4 years ago
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage?]
Keywords: qawanted
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage?] → [VH-FL-blocking-][VH-FC-blocking?]
Flags: needinfo?(jmitchell)
Josh - The whiteboard isn't right here. If there are no QA requests left & the existing request was good enough, [QAnalyst-Triage+] should be used.
Flags: needinfo?(jmitchell)
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
This looks ugly. UX - Is this a blocker from your perspective?
Blocks: 1015336
No longer blocks: 1017954
Flags: needinfo?(firefoxos-ux-bugzilla)

Comment 12

4 years ago
It is not appealing but I don't think I'd block. Flagging Peter for final call though.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(pdolanjski)
Do you guys have a spec somewhere for smart icon generation? I haven't seen a spec and it would be good to have a reference to it here.

Updated

4 years ago
Blocks: 1017954
No longer blocks: 1015336
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?][QAnalyst-Triage+] → [VH-FL-blocking-][VH-FC-blocking-][QAnalyst-Triage+]
(In reply to Kevin Grandon :kgrandon from comment #13)
> Do you guys have a spec somewhere for smart icon generation? I haven't seen
> a spec and it would be good to have a reference to it here.

Patryk?

Unless this is a widespread issue, I wouldn't block.
Flags: needinfo?(pdolanjski) → needinfo?(padamczyk)
blocking-b2g: --- → backlog
Whiteboard: [2.0-VH-bug-bash][systemsfe] → [2.0-VH-bug-bash][systemsfe][priority]
Hung, should be able to provide a spec, see comment 13
And not this wouldn't be a block, just embarrassing, we should get this fixed for the next release for sure.
Flags: needinfo?(padamczyk) → needinfo?(hnguyen)
(In reply to Kevin Grandon :kgrandon from comment #8)

We use the spec from bug 965711 for Smart Collection icons.
It does not cover favicons though, so we will need UX to decide on that.

Bug 1021496 can be related to the pixelation issues?
Flags: needinfo?(amirn)

Updated

4 years ago
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking-][QAnalyst-Triage+] → [VH-FL-blocking-][VH-FC-blocking-][QAnalyst-Triage+][lead-review+]

Comment 17

4 years ago
Created attachment 8452583 [details]
___SmartCollectionsSpec20140708.png

Adding updated spec. 

After speaking with Amir, we agreed that there should be a default background in place so transparent favicon will always be visible. 

This only addresses the legibility issue however. 

For the distortion issue, I'm not sure what we can do since favicon are only 32x32px and need to be scaled up when used in this case.
Flags: needinfo?(hnguyen)
(In reply to Hung Nguyen from comment #17)
> For the distortion issue, I'm not sure what we can do since favicon are only
> 32x32px and need to be scaled up when used in this case.

Maybe we can follow the spec in bug 1021496 and disable anti-aliasing (see https://github.com/mozilla-b2g/gaia/pull/20524/files)

Updated

4 years ago
Assignee: nobody → amirn
The favicon rendering logic already implemented here:
https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/grid_icon_renderer.js#L107-L141

I would like to avoid duplicating it, and re-use the already rendered bookmark icon from the homescreen, but it is a blob which only the homescreen can have a reference to.

Kevin, can you suggest an alternative solution?
Flags: needinfo?(kgrandon)
(In reply to Amir Nissim (:amirn) from comment #19)
> The favicon rendering logic already implemented here:
> https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/
> grid_icon_renderer.js#L107-L141
> 
> I would like to avoid duplicating it, and re-use the already rendered
> bookmark icon from the homescreen, but it is a blob which only the
> homescreen can have a reference to.
> 
> Kevin, can you suggest an alternative solution?

I don't think we'll have luck accessing rendered icons from homescreen, but since these are pinned, can't we just get them from the grid instance in the collection? The logic should be the same (both apps should use GridIconRenderer)
Flags: needinfo?(kgrandon)
Created attachment 8456167 [details] [review]
Pull Request [WIP]

This patch fails when dropping the Mozilla bookmark into the Collection, but works for any icon rendering that is done afterwards.

I think it is because in add_to_collection.js the icon was never rendered.

Kevin, can you take a quick look at this please?
Attachment #8456167 - Flags: feedback?(kgrandon)
Comment on attachment 8456167 [details] [review]
Pull Request [WIP]

I didn't test, but seems like it should work. Left a comment on github.
Attachment #8456167 - Flags: feedback?(kgrandon) → feedback+
Created attachment 8461505 [details] [review]
[WIP] trying to prepare decorated blobs in HomeIcons init

At first I though this bug will be an easy fix, but after trying several methods I still can't find a reasonable solution.

The only thing needed is to use the decoratedIconBlob of the bookmark instead of the 'icon' property (which is the url for the not decorated favicon).
But the problem is that it's not ready when dropping a bookmark ontop a collection because it was not added to any grid, and so not rendered.

This patch is an attempt at rendering the decorated blobs in the HomeIcons init process, but I can't get it working (see comment in the patch).
Also, I'm not sure this is the best solution as it is quite complex.

I'm going to unassign myself for now. Feel free to take it.
Attachment #8456167 - Attachment is obsolete: true
Attachment #8461505 - Flags: feedback?(kgrandon)
Attachment #8461505 - Flags: feedback?(crdlc)

Updated

4 years ago
Assignee: amirn → nobody
Comment on attachment 8461505 [details] [review]
[WIP] trying to prepare decorated blobs in HomeIcons init

Hmm, it does seem fairly complex. Not sure if I will have time in the next few weeks to pick it up, but if I find some I will take it.

Have we tried calling GridIconRenderer directly in order to generate what we need? All we should need to do there is create an <img> element, and call the right methods to generate blobs. It may be easier.
Attachment #8461505 - Flags: feedback?(kgrandon)
(In reply to Kevin Grandon :kgrandon from comment #24)
> Have we tried calling GridIconRenderer directly in order to generate what we
> need? All we should need to do there is create an <img> element, and call
> the right methods to generate blobs. It may be easier.

I tried calling GridIconRenderer.prototype.favicon(img) but it failed instantly, in:
https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/grid_icon_renderer.js#L111

it was missing the _icon property
Ah right, it looks like it assigns a reference in the constructor, so you'd need to call it for each icon: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/js/grid_icon_renderer.js#L15

I also saw that the _maxSize getter will reference the grid layout, so it appears we might need a full grid instantiated =/
Comment on attachment 8461505 [details] [review]
[WIP] trying to prepare decorated blobs in HomeIcons init

Kevin's feedback is enough thought
Attachment #8461505 - Flags: feedback?(crdlc)
(Reporter)

Updated

4 years ago
Blocks: 1069288
Priority: P1 → P2

Comment 28

4 years ago
The smart collections is an important element of the UX. The missing background makes it harder to understand for first time users as it is almost impossible to see what is in the collection.
blocking-b2g: backlog → 2.1?
Whiteboard: [2.0-VH-bug-bash][systemsfe][priority] → [2.0-VH-bug-bash][systemsfe][priority][Tako_Blocker]
This is something that we would like to fix, but unfortunately doesn't fit the definition of a blocker as we've shipped 2.0 with it. We would consider a patch for uplift though.
blocking-b2g: 2.1? → ---
hi Kevin,

Partner has raised this concern for 2.1, so when you mentioned about a patch to uplift, are you referring to 2.1 or 2.2?
Flags: needinfo?(kgrandon)
We have no patches written for this yet, so nothing for uplift. At the time I was talking about uplfting something to v2.1 if we were to write a patch - it seems it might be too late for that though?

For this issue, we'd still need to write a patch.
Flags: needinfo?(kgrandon)
hi Kevin,

thanks for your reply. i just want to understand the complexity/feasibility of making this patch and is there any potential risk to land it to 2.1? partner takes this as an UX blocker according to their criteria.

thank you
Flags: needinfo?(kgrandon)
The icon generation logic in the collections app is quite difficult to get right. There is probably a bit of risk to do this.
Flags: needinfo?(kgrandon)

Updated

4 years ago
Blocks: 1088026

Updated

4 years ago
Whiteboard: [2.0-VH-bug-bash][systemsfe][priority][Tako_Blocker] → [2.0-VH-bug-bash][systemsfe][priority]
Mass update: Resolve/Wontfix all existing collections bugs as this feature is now removed.

Please re-open or file a new bug if you feel that this bug still exists in master.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.