Closed Bug 1401851 Opened 2 years ago Closed 2 years ago

Yahoo favicon / rich-icon is black

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: Dolske, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

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?).
we should never store icons with a mask attribute :(
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.
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee: nobody → mak77
Status: NEW → ASSIGNED
[Tracking Requested - why for this release]: Recent regression due to bug 1352459
Depends on: 1401894
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.
Attachment #8910646 - Flags: review?(dolske) → review?(najiang)
"since it touches image-rendering: crisp"
Blocks: 1402250
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 on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

https://reviewboard.mozilla.org/r/182084/#review187896
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!
Flags: needinfo?(edilee)
Comment on attachment 8910646 [details]
Bug 1401851 - Skip masked favicons in ContentLinkHandler until we support them.

https://reviewboard.mozilla.org/r/182086/#review187898
Attachment #8910646 - Flags: review?(najiang) → review+
Oops, nvm, just tried again, it worked.
Flags: needinfo?(edilee)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/176e16629030
Skip masked favicons in ContentLinkHandler until we support them. r=nanj
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
Attachment #8910646 - Flags: review+
anyone can review, and we're supposed to ask review to non-peers to grow new peers :)
https://hg.mozilla.org/mozilla-central/rev/176e16629030
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8910646 - Flags: approval-mozilla-beta?
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
Attachment #8910646 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.