1389343 - Missing/tiny icons for history, bookmark items

Status: VERIFIED FIXED

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

Description

[:Grisha Kruglov]

Attachment: history.png

Not sure if this is photon related, but I haven't seen this behaviour before very recently.

Comment 1

[:Grisha Kruglov]

Cropped icon is Bug 1388554.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Adding AS just in case this is our regression.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 4

[Wesly Huang (EPM)]

[triage@0816] leave AS team to triage it.

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Hi Jing Wei
Could this be our regression?

This should be important

Comment 6

[Wesly Huang (EPM)]

We should prioritize the investigation in Sprint 57.3.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Is this something you still see in Nightly?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Attachment: change to gray background test

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)

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1389343 - Make default background grey so it can occupied the entire favicon.

Review commit: https://reviewboard.mozilla.org/r/175902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/175902/

Comment 11

[Wesly Huang (EPM)]

ni Carol per comment#9. ni Jingwei for review the patch?

Comment 12

[Nevin Chen(Not active on Bugzilla)]

I need UX approve my solution before code review.

Comment 13

[Wesly Huang (EPM)]

thanks and got it, no one was ni-ed before so I ni Carol for it.

Comment 14

[Carol Huang [:Carol]]

Hi Nevin! 
Let's use one of our Photon color #B1B1B3 for the bitmap of the favicon bg color! thanks!

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8904121 [details]
Bug 1389343 - Make default background grey so it can occupied the entire favicon.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175902/diff/1-2/

Comment 16

[Nevin Chen(Not active on Bugzilla)]

Attachment: FFB1B1B3.png

Hi Carol
Please verify if this works.
I'll land it after your review

Comment 17

[Carol Huang [:Carol]]

Hi Nevin,
it looks good to me! thanks!

Comment 18

[Jing-wei Wu [:jwu]]

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

Comment 19

[Nevin Chen(Not active on Bugzilla)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/174a7ea87cc5
Make default background grey so it can occupied the entire favicon. r=jwu

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/174a7ea87cc5

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8904121 [details]
Bug 1389343 - Make default background grey so it can occupied the entire favicon.

Photon related, beta57+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5a9fd7c02662

Comment 24

[u549602]

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)

Comment 25

[Ioana Chiorean]

As per comment #24