1247109 - Synced Tabs sidebar has blurry favicons

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

The favicons shown in the synced tabs panel appear slightly blurry - it's almost as if they are being scaled up or down a pixel or 2.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

I could see this while on Vidyo with Ryan, but can't reproduce it now - I took some screenshots of the bookmarks and syncedtabs sidebar and no matter how far I blow them up, I can't see a different. FWIW, the sidebar is using 16x16 for the icon, which is the same size as the default favicon.

Ryan, can you please try and repro, and if you can, attach a screenshot or some other evidence?

Comment 2

[Ryan Feeley [:rfeeley]]

Attachment: blur.png

Doubled the image (nearest neighbour, so no additional blurring), but look at the globe in Synced Tabs. They are similiar in size, but the Synced Tabs one is blurry. All the favicons there are blurry.

Comment 3

[Ryan Feeley [:rfeeley]]

Attachment: blur2.png

Here's another one. Synced Tabs on left, bookmarks on right. Is Gecko applying some interpolation?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0005-Bug-1247109-apply-moz-crisp-edges-style-to-favicons-.patch

A small patch to add "image-rendering: -moz-crisp-edges;" to favicons in hidpi. I've noticed some @media declarations for hidpi have started using 1.1 rather than 2, but I couldn't find an example of -moz-crisp-edges using 1.1 so I stuck with 2 - let me know if 1.1 is a better choice.

In IRC, Ryan said he applied that style using devtools and it fixed the issue for him.

The patch:

+/* Apply crisp rendering for favicons at exactly 2dppx resolution */
+@media (resolution: 2dppx) {
+  .item-icon-container {
+    image-rendering: -moz-crisp-edges;
+  }
+}
+

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=71b2885ed3cb (incase Ryan wants to test further)

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Greener try (failures were due to unrelated patches in the same push) at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8456a64466c1

Comment 6

[:Gijs (out for now; he/him)]

Comment on attachment 8720640 [details] [diff] [review]
0005-Bug-1247109-apply-moz-crisp-edges-style-to-favicons-.patch

Review of attachment 8720640 [details] [diff] [review]:
-----------------------------------------------------------------

All the other cases of this are in content/, so that it works with other themes. This probably should be, too, but you'll need a more specific selector than ".item-icon-container". :-)

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8720640 [details] [diff] [review]
> 0005-Bug-1247109-apply-moz-crisp-edges-style-to-favicons-.patch
> 
> Review of attachment 8720640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All the other cases of this are in content/, so that it works with other
> themes. This probably should be, too, but you'll need a more specific
> selector than ".item-icon-container". :-)

I'm not sure what you are suggesting here. I could move this rule (with a more descriptive selector) into content, but the rest of the panel probably isn't going to work with themes. ie, just before the new ".item-icon-container" rule is an existing ".item-icon-container" rule - having *just* a rule with the -moz-crisp-edges rule in content/ but all other key rules for the look-and-feel of the sidebar remaining in sidebar.css doesn't make any sense to me.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Mark Hammond [:markh] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > Comment on attachment 8720640 [details] [diff] [review]
> > 0005-Bug-1247109-apply-moz-crisp-edges-style-to-favicons-.patch
> > 
> > Review of attachment 8720640 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > All the other cases of this are in content/, so that it works with other
> > themes. This probably should be, too, but you'll need a more specific
> > selector than ".item-icon-container". :-)
> 
> I'm not sure what you are suggesting here. I could move this rule (with a
> more descriptive selector) into content, but the rest of the panel probably
> isn't going to work with themes. ie, just before the new
> ".item-icon-container" rule is an existing ".item-icon-container" rule -
> having *just* a rule with the -moz-crisp-edges rule in content/ but all
> other key rules for the look-and-feel of the sidebar remaining in
> sidebar.css doesn't make any sense to me.

Well, .item-icon-container will always have the favicon, because that's set from JS. There is no reason I can think of why other themes don't want that thing set (and every reason to think they might forget).

More pragmatically, every single other browser/ and toolkit/ use of crisp-edges that I could see in the tree already is in content/ . That includes the other sidebars and the bookmarks toolbar. Putting it in content (like browser/base/content/browser.css like the bookmark toolbar one and the panel menu subview ones) would be the most consistent thing to do.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

hmm - I can't get any styles from browser.css to be applied. sidebar.xhtml doesn't explicitly reference browser.css, but even when I do, I can't get the styles to apply. eg, as an experiment I added:

--- a/browser/base/content/browser.css
+++ b/browser/base/content/browser.css
@@ -10,6 +10,10 @@
   --panelui-subview-transition-duration: 150ms;
 }

+  .item-icon-container {
+    font-weight: bold;
+  }
+
...
--- a/browser/components/syncedtabs/sidebar.xhtml
+++ b/browser/components/syncedtabs/sidebar.xhtml
@@ -20,6 +20,7 @@
     <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/skin/syncedtabs/sideb
ar.css"/>
     <link rel="stylesheet" type="text/css" media="all" href="chrome://global/skin/"/>
     <link rel="stylesheet" type="text/css" media="all" href="chrome://global/skin/textbox.css"/>
+    <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/content/browser.css"/
>

and devtools tells me that style wasn't applied - browser.css isn't listed as contributing *any* styles to *any* elements in the sidebar. Gijs, do you know what that would be?

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Mark Hammond [:markh] from comment #9)
> hmm - I can't get any styles from browser.css to be applied. sidebar.xhtml
> doesn't explicitly reference browser.css, but even when I do, I can't get
> the styles to apply. eg, as an experiment I added:
> 
> --- a/browser/base/content/browser.css
> +++ b/browser/base/content/browser.css
> @@ -10,6 +10,10 @@
>    --panelui-subview-transition-duration: 150ms;
>  }
> 
> +  .item-icon-container {
> +    font-weight: bold;
> +  }
> +
> ...
> --- a/browser/components/syncedtabs/sidebar.xhtml
> +++ b/browser/components/syncedtabs/sidebar.xhtml
> @@ -20,6 +20,7 @@
>      <link rel="stylesheet" type="text/css" media="all"
> href="chrome://browser/skin/syncedtabs/sideb
> ar.css"/>
>      <link rel="stylesheet" type="text/css" media="all"
> href="chrome://global/skin/"/>
>      <link rel="stylesheet" type="text/css" media="all"
> href="chrome://global/skin/textbox.css"/>
> +    <link rel="stylesheet" type="text/css" media="all"
> href="chrome://browser/content/browser.css"/
> >
> 
> and devtools tells me that style wasn't applied - browser.css isn't listed
> as contributing *any* styles to *any* elements in the sidebar. Gijs, do you
> know what that would be?

Welcome to explicit XML namespacing in CSS, probably the most annoying/useless "feature" ever!

I just checked, and while .item-icon-container doesn't work, html|*.item-icon-container does. So you'll need to use html| in appropriate doses to make it work. I *think* just putting it on the first item in whatever descendant/child selector combination you're going for should work, but don't quote me on that. :|

Comment 11

[:Gijs (out for now; he/him)]

It might be quickest to use the browser toolbox to experiment with this - open it up, then use the frame button (top right, leftmost button) to select only the sidebar frame, then open the style editor for browser.css and poke until you have something that looks reasonable and works...

Sorry this is such a pain... :-\

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0004-Bug-1247109-apply-moz-crisp-edges-style-to-favicons-.patch

(In reply to :Gijs Kruitbosch from comment #10)
> Welcome to explicit XML namespacing in CSS, probably the most
> annoying/useless "feature" ever!
> 
> I just checked, and while .item-icon-container doesn't work,
> html|*.item-icon-container does.

Ah - thanks! I did play a little with the "html|" but neglected to add the "*".

> So you'll need to use html| in appropriate
> doses to make it work. I *think* just putting it on the first item in
> whatever descendant/child selector combination you're going for should work,
> but don't quote me on that. :|

It doesn't - I needed to add it on each selector :(

The selector I chose is "html|*.tabs-container html|*.item-tabs-list html|*.item-icon-container" which isn't great as it doesn't self-document the fact it is the Synced Tabs sidebar. Adding "#sidebar" at the start also didn't work - I'm guessing as it didn't like matching from XUL down into the HTML document. So I added a comment :)

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 8721870 [details] [diff] [review]
0004-Bug-1247109-apply-moz-crisp-edges-style-to-favicons-.patch

Review of attachment 8721870 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take it. Thanks!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b594319b2865

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b594319b2865

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Ryan, can you please verify this is fixed? I guess it will be in tomorrow's nightly...

Comment 17

[Ryan Feeley [:rfeeley]]

It's fixed!

Comment 18

[Mark Hammond [:markh] [:mhammond]]

\o/