Closed
Bug 1401851
Opened 7 years ago
Closed 7 years ago
Yahoo favicon / rich-icon is black
Categories
(Toolkit :: Places, defect, P1)
Toolkit
Places
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)
(deleted),
text/x-review-board-request
|
nanj
:
review+
Mardak
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
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?).
Assignee | ||
Comment 1•7 years ago
|
||
we should never store icons with a mask attribute :(
Assignee | ||
Comment 2•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
[Tracking Requested - why for this release]: Recent regression due to bug 1352459
tracking-firefox57:
--- → ?
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
"since it touches image-rendering: crisp"
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox-esr52:
--- → unaffected
Keywords: regression
Comment 8•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-review |
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+
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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+
Assignee | ||
Comment 15•7 years ago
|
||
anyone can review, and we're supposed to ask review to non-peers to grow new peers :)
Comment 16•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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+
Comment 19•7 years ago
|
||
bugherder uplift |
Comment 20•7 years ago
|
||
(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.
Description
•