Closed Bug 1389343 Opened 2 years ago Closed 2 years ago

Missing/tiny icons for history, bookmark items

Categories

(Firefox for Android :: Awesomescreen, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
fennec + ---
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: Grisha, Assigned: cnevinchen)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [FNC][SPT58.2][MVP])

Attachments

(4 files)

Attached image history.png
Not sure if this is photon related, but I haven't seen this behaviour before very recently.
Cropped icon is Bug 1388554.
tracking-fennec: --- → ?
Adding AS just in case this is our regression.
Whiteboard: [mobileAS]
(In reply to Michael Comella (:mcomella) from comment #2)
> Adding AS just in case this is our regression.

Actually, Sebastian - do you think this is related to AS (perhaps your favicon changes - did those land yet)? Please re-add [mobileAS] if you think so.
Flags: needinfo?(s.kaspari)
Whiteboard: [mobileAS]
[triage@0816] leave AS team to triage it.
Hi Jing Wei
Could this be our regression?

This should be important
tracking-fennec: ? → +
Flags: needinfo?(wehuang)
Flags: needinfo?(topwu.tw)
Priority: -- → P1
We should prioritize the investigation in Sprint 57.3.
Flags: needinfo?(wehuang)
Is this something you still see in Nightly?
Flags: needinfo?(s.kaspari)
(In reply to :Grisha Kruglov from comment #0) 
> Not sure if this is photon related, but I haven't seen this behaviour before
> very recently.

I have seen this before and it's not a regression. The difference here is that no accent color could be extracted - therefore you are seeing the icon without a background color added to it. Another example is the bugzilla favicon. It's also tiny but with the green background it appears larger in the list.
Assignee: nobody → cnevinchen
Whiteboard: [FNC][SPT57.3][BL]
This bug is not regression. It's a special case for this url: www.calvertjournal.com.
We can't get the color from the bitmap of the favicon.
And since the default color is 0 (no color, the favicon will look tiny and small since there's no background)

Hi Carol
Please see the attachment.
Could you give us some design suggestion about this?
I've also attached a test sample to see if we change the color to gray background (#3C3C3C)
Flags: needinfo?(topwu.tw)
Flags: needinfo?(chuang)
ni Carol per comment#9. ni Jingwei for review the patch?
Flags: needinfo?(topwu.tw)
I need UX approve my solution before code review.
Flags: needinfo?(wehuang)
thanks and got it, no one was ni-ed before so I ni Carol for it.
Flags: needinfo?(wehuang)
Hi Nevin! 
Let's use one of our Photon color #B1B1B3 for the bitmap of the favicon bg color! thanks!
Flags: needinfo?(chuang) → needinfo?(cnevinchen)
Whiteboard: [FNC][SPT57.3][BL]
Attached image FFB1B1B3.png
Hi Carol
Please verify if this works.
I'll land it after your review
Flags: needinfo?(cnevinchen) → needinfo?(chuang)
Hi Nevin,
it looks good to me! thanks!
Flags: needinfo?(chuang) → needinfo?(cnevinchen)
Flags: needinfo?(cnevinchen)
Attachment #8904121 - Flags: review?(max)
Flags: needinfo?(topwu.tw)
Attachment #8904121 - Flags: review?(max) → review?(topwu.tw)
Comment on attachment 8904121 [details]
Bug 1389343 - Make default background grey so it can occupied the entire favicon.

https://reviewboard.mozilla.org/r/175902/#review192114
Attachment #8904121 - Flags: review?(topwu.tw) → review+
Comment on attachment 8904121 [details]
Bug 1389343 - Make default background grey so it can occupied the entire favicon.

Approval Request Comment
[Feature/Bug causing the regression]: For some small favicon, we show white background thus makes it looks smaller. We should show a darker color so it's more obvious. 
[User impact if declined]: User will see small favicon and feel confused.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]: no
[Why is the change risky/not risky?]: just changing the default color of favicon background
[String changes made/needed]: no
Attachment #8904121 - Flags: approval-mozilla-beta?
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/174a7ea87cc5
Make default background grey so it can occupied the entire favicon. r=jwu
https://hg.mozilla.org/mozilla-central/rev/174a7ea87cc5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8904121 [details]
Bug 1389343 - Make default background grey so it can occupied the entire favicon.

Photon related, beta57+
Attachment #8904121 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on both Nightly (58.0a1 - 2017-10-08)and latest Beta 57.0b7.
This issue was verified on a Sony Xperia Z5 (Android 6.0.1) and on a Pixel C (Android 7.1.1)
Whiteboard: [FNC][SPT58.2][MVP]
As per comment #24
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.