Synced Tabs sidebar has blurry favicons

VERIFIED FIXED in Firefox 47

Status

()

defect
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
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?
Flags: needinfo?(rfeeley)
Posted image 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.
Flags: needinfo?(rfeeley)
Posted image blur2.png
Here's another one. Synced Tabs on left, bookmarks on right. Is Gecko applying some interpolation?
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)
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8720640 - Flags: review?(gijskruitbosch+bugs)
Greener try (failures were due to unrelated patches in the same push) at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8456a64466c1
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". :-)
Attachment #8720640 - Flags: review?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
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?
(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. :|
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... :-\
(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 :)
Attachment #8720640 - Attachment is obsolete: true
Attachment #8721870 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8721870 - Flags: review?(gijskruitbosch+bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b594319b2865
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Ryan, can you please verify this is fixed? I guess it will be in tomorrow's nightly...
Flags: needinfo?(rfeeley)
It's fixed!
Flags: needinfo?(rfeeley)
\o/
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.