YouTube icon doesn't look good in Firefox

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dolske, Assigned: mak)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

Reporter

Description

2 years ago
Posted image Screenshot
The YouTube favicon is not looking good in Firefox. It was already kind of wonky in Firefox 55, and is worse now in Nightly. (With the fix for bug 1401777, but I think it's been looking that way for a while.)

Screenshot attached shows Release 55.0.3, current Nightly, and Chrome Canary.

Markup seems to be:

<link rel="shortcut icon" href="/yts/img/favicon-vfl8qSV2F.ico" type="image/x-icon" >
<link rel="icon" href="/yts/img/favicon_32-vflOogEID.png" sizes="32x32" >
<link rel="icon" href="/yts/img/favicon_48-vflVjB_Qk.png" sizes="48x48" >
<link rel="icon" href="/yts/img/favicon_96-vflW9Ec0w.png" sizes="96x96" >
<link rel="icon" href="/yts/img/favicon_144-vfliLAfaB.png" sizes="144x144" >

In Nightly, devtools shows that we're using favicon-vfl8qSV2F.ico, which contains just a 16x16 icon. Looks "correct" for what it is, but isn't the best choice of available icons for us to use.

In 55, devtools shows that we're using avicon_144-vfliLAfaB.png. That's the 144x144 icon, and it looks poor because it's downscaled with -moz-crisp-edges.

Hard to tell what Chrome is using, but since it looks properly scaled it's probably the specified 32x32 icon.


What should Nightly be using? Ideally the 32x32 (since I'm on a Retina display), but it's a little surprising we're not trying to use the 144x144 like 55 did (even though that would still look bad) -- not sure if that's a bug or known limitation.
Assignee

Comment 1

2 years ago
(In reply to Justin Dolske [:Dolske] from comment #0)
> In Nightly, devtools shows that we're using favicon-vfl8qSV2F.ico, which
> contains just a 16x16 icon. Looks "correct" for what it is, but isn't the
> best choice of available icons for us to use.

It's very uncommon that they provide an ico with just a 16x16 resolution ;(

> What should Nightly be using? Ideally the 32x32 (since I'm on a Retina
> display), but it's a little surprising we're not trying to use the 144x144
> like 55 did (even though that would still look bad) -- not sure if that's a
> bug or known limitation.

the 144x144 is considered a Rich icon and thus we don't use it for the tabs, since it would be heavily downscaled.
55 just stores the last favicon defined in the page, nothing more than that. It works by luck.

We are not done yet with picking icons in ContentLinkHandler, Nan did a first pass, but we need to check the sizes attribute yet and always store a 32px icon if it's available in any form.

We'll try to fix the main favicon problems in 58.
Priority: -- → P1
Whiteboard: [fxsearch]
Assignee

Updated

2 years ago
Blocks: 1363620
Assignee

Updated

2 years ago
Blocks: 1409969
Assignee

Updated

2 years ago
No longer blocks: 1409969
Assignee

Updated

2 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8924973 [details]
Bug 1403829 - Prefer fetching properly sized favicons from pages.

https://reviewboard.mozilla.org/r/196230/#review201714

::: browser/modules/ContentLinkHandler.jsm:152
(Diff revision 1)
>    // Rich icons are either apple-touch or fluid icons, or the ones of the
>    // dimension 96x96 or greater
>    let largestRichIcon;
>  
> +  function isICO(icon) {
> +    return icon.type == "image/x-icon" || icon.type == "image/vnd.microsoft.icon";

nit: this could be declared outside of `faviconTimeoutCallback`

::: browser/modules/ContentLinkHandler.jsm:162
(Diff revision 1)
> -      icon.type === "image/vnd.microsoft.icon") {
> -      preferredIcon = icon;
> +        preferredIcon = icon;
> -      continue;
> +      } else if (icon.width == preferredWidth && preferredIcon.type != "image/svg+xml") {
> +        preferredIcon = icon;
> +      } else if (isICO(icon) && (preferredIcon.type == null || isICO(preferredIcon))) {
> +        preferredIcon = icon;

Something about these conditions made it hard to follow, but after staring at it for 30 minutes, I can't think of anything particularly better. Maybe have the comment for `preferredIcon` closer to this logic?

I think it's just the asymmetry of each of the conditions:
- always pick svg
- prefer width but don't replace svg
- use ico only if currently ico or no preferred yet (implicitly meaning don't replace a non-svg/non-ico which must have been preferred because it was the desired width)

But something also that it could be very easy to accidentally overwrite an actually preferred icon in the future, so good thing with all the tests.
Attachment #8924973 - Flags: review?(edilee) → review+
Comment hidden (mozreview-request)

Comment 5

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/fef50221fd78
Prefer fetching properly sized favicons from pages. r=Mardak

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fef50221fd78
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.