1401851 - Yahoo favicon / rich-icon is black

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Justin Dolske [:Dolske]]

With bug 1352459, the Yahoo! favicon is now black instead of purple!, which seems mildly surprising given their love for the color.

The page markup has:

<link rel="icon" sizes="any" mask href="https://s.yimg.com/os/mit/media/p/common/images/favicon_new-7483e38.svg">
<meta name="theme-color" content="#400090">
<link rel="shortcut icon" href="https://s.yimg.com/rz/l/favicon.ico" />

Devtools confirms that we're using favicon_new-7483e38.svg as the tab's favicon.

This previously came up in bug 366324 (bug 1174548 is probably a better-distilled version)... Apple's poorly-spec'd "mask" icons need the theme-color <meta> attribute applied or else they'll be black-and-white.

Previously this wasn't a problem: Correctly-coded sites put the "mask" icon first, which we ignored in favor of the following favicon.ico (i.e., last <link rel=icon> wins).

Given bug 1401777, I presume this is showing up because we're picking it as the highest-resolution icon available. But even with a fix for that, we're still going to need to add support for theme-color for where such icons get used in activity stream (or perhaps in some cases where we intentionally end up preferring the SVG over the favicon?).

Comment 1

[Marco Bonardo [:mak]]

we should never store icons with a mask attribute :(

Comment 2

[Marco Bonardo [:mak]]

This is not a bug in Places, it's ContentLinkHandler that should not even try to store that icon... But there's no component for that.

Comment 3

[Marco Bonardo [:mak]]

Attachment: Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

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

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

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

Comment 5

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: Recent regression due to bug 1352459

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

I'm switching the request to Nan, the other bug is more involving since it touches crip we have from some time, but this one has no bad consequences.

Comment 7

[Marco Bonardo [:mak]]

"since it touches image-rendering: crisp"

Comment 8

[Nan Jiang [:nanj]]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

https://reviewboard.mozilla.org/r/182084/#review187892

LGTM. Thank you! r+

Comment 9

[Nan Jiang [:nanj]]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

https://reviewboard.mozilla.org/r/182084/#review187896

Comment 10

[Nan Jiang [:nanj]]

Looks like I am not the "suitable reviewer" yet (level 3 I guess) :P, although the patch looks good to me.

Mardak, could you place the r+ to reviewboard for me, please? Thanks!

Comment 11

[Nan Jiang [:nanj]]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

https://reviewboard.mozilla.org/r/182086/#review187898

Comment 12

[Nan Jiang [:nanj]]

Oops, nvm, just tried again, it worked.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/176e16629030
Skip masked favicons in ContentLinkHandler until we support them. r=nanj

Comment 14

[Ed Lee :Mardak]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

(In reply to Nan Jiang [:nanj] from comment #10)
> Looks like I am not the "suitable reviewer" yet (level 3 I guess) :P,
> although the patch looks good to me.
> Mardak, could you place the r+ to reviewboard for me, please? Thanks!
Looks like mozreview doesn't care about actual reviewer status or not. Adding r=Mardak

Comment 15

[Marco Bonardo [:mak]]

anyone can review, and we're supposed to ask review to non-peers to grow new peers :)

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/176e16629030

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1352459
[User impact if declined]: wrong favicons
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: trivial condition change
[String changes made/needed]: none

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

Fix a recent regression + impacting a partner, taking it.
Should be in 57b3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0b02b146664

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Marco Bonardo [::mak] from comment #17)
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no
> [Why is the change risky/not risky?]: trivial condition change

Setting qe-verify- based on Marco's assessment on manual testing needs.