1041845 - Apply image-rendering: -moz-crisp-edges to favicons throughout the UI

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

There are still many favicons that are only 16x16 in the longtail of websites so we should use image-rendering: -moz-crisp-edges wherever we show favicons to make them crisper.

Comment 1

[Bernardo Rittmeyer [:rittme]]

Some favicons already have image-rendering: -moz-crisp-edges applied, but only for higher resolutions, using @media (min-resolution: 2dppx). I think we should apply it to every resolution, since assuming that we are scaling at 2dppx is now wrong. This also don't take into account other high resolution devices, that have less than 2dppx, for example Surface Pro, that has 1.5 ddpx.

Does anyone have a reason not to do it for all dppx?

Comment 2

[Matthew N. [:MattN]]

MDN says image-rendering "applies to any images appearing on the element properties, but has no effect on non-scaled images.". If it actually has no effect when there isn't scaling then it seems like we should apply it to favicons regardless of the resolution.

Comment 3

[Justin Dolske [:Dolske]]

Yes, we should just do it for all DPI.

We might want to revisit in the future to see if we can do things better. For example, 16x16 at 125% is 20x20 -- that's unlikely to scale well with any method, and might look better by just adding 2px of padding. And I think I mentioned in the original bug that ideally we'd using normal scaling in certain cases (when working with larger icons, or when downscaling from a big photo), but that hasn't been an issue so far on Retina. So we can defer overengineering it. :)

Comment 4

[Dão Gottwald [:dao]]

(In reply to Justin Dolske [:Dolske] from comment #3)
> Yes, we should just do it for all DPI.

I'm not convinced, see below.

> We might want to revisit in the future to see if we can do things better.
> For example, 16x16 at 125% is 20x20 -- that's unlikely to scale well with
> any method

But probably better with interpolation rather than one in four pixel rows being duplicated, which might make icons look distorted?

Comment 5

[Justin Dolske [:Dolske]]

Screenshots welcome, but I'd expect both to look not-great, and would prefer to err on the side of simplicity for now.

Comment 6

[Bernardo Rittmeyer [:rittme]]

Attachment: favicon for image-resolution testing

Comment 7

[Bernardo Rittmeyer [:rittme]]

Attachment: favicon for image-resolution testing

Comment 8

[Bernardo Rittmeyer [:rittme]]

Attachment: favicon for image-resolution testing

Comment 9

[Bernardo Rittmeyer [:rittme]]

Attachment: page to compare favicon rendering methods

This should allow some visual comparison.

Comment 10

[Bernardo Rittmeyer [:rittme]]

Attachment: Rev 1 - Resized favicons are now rendered using -moz-crisp-edges throughout the UI

Here is a list of the different favicons touched by this patch:

.All tabs dropdown
.Applications pane list
.Awesomebar dropdown
.Back/forward navigation popup menu
.Bookmark toolbar (moved from theme to content)
.Panel menu bookmarks/history subviews
.Places windows (history/bookmarks)
.Preferences Home Page “Use bookmark…"
.Search engine icons (moved from theme to content)
.Sidebars (bookmark/history)
.Social API chatbox
.Social API sidebar
.Social API toolbar button
.Tabs (moved from theme to content)
.Tabs groups

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8468800 [details] [diff] [review]
Rev 1 - Resized favicons are now rendered using -moz-crisp-edges throughout the UI

Please do this in theme CSS files (i.e. in browser/themes/).

Comment 12

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #11)
> Please do this in theme CSS files (i.e. in browser/themes/).

I had advised the opposite since this is about the content (favicon) rendering and not something that themes should need to be changing. i.e. this is more of a correctness change and not about styling.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #12)
> I had advised the opposite since this is about the content (favicon)
> rendering and not something that themes should need to be changing. i.e.
> this is more of a correctness change and not about styling.

This seems reasonable to me - Dao, what do you think?

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8468800 [details] [diff] [review]
Rev 1 - Resized favicons are now rendered using -moz-crisp-edges throughout the UI

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

::: browser/base/content/browser.css
@@ +21,5 @@
>  searchbar {
>    -moz-binding: url("chrome://browser/content/search/search.xml#searchbar");
>  }
>  
> +

Nit: extra new line

@@ +853,5 @@
>  
> +.social-sidebar-favico,
> +.social-status-button,
> +.chat-status-icon
> + {

Braces go on the same lines as the last rule

Comment 15

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > Please do this in theme CSS files (i.e. in browser/themes/).
> 
> I had advised the opposite since this is about the content (favicon)
> rendering

I'm not sure what this means. As far as I can tell this is a stylistic change purely motivated by aesthetics.

This patch isn't consistent either, as it patches browser/themes/shared/customizableui/panelUIOverlay.inc.css which isn't a content stylesheet.

> and not something that themes should need to be changing. i.e.
> this is more of a correctness change and not about styling.

Nearest-neighbor isn't more correct than interpolation. As a theme author I wouldn't copy this code, as I personally think it's doing more harm than good. Looking at the 1.25x rows in attachment 8466360 [details], it appears to me that nearest-neighbor makes all three icons look distorted, although in the special case of the facebook icon it doesn't look as unpleasant, since the vertical stroke of the "f" being thicker than the horizontal strokes doesn't matter much for the icon.

Comment 16

[Dão Gottwald [:dao]]

There's a second, unrelated issue. image-rendering:-moz-crisp-edges affects down-scaled icons and images used as their own favicons in image documents. That effect seems clearly undesirable.

Comment 17

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #15)
> This patch isn't consistent either, as it patches
> browser/themes/shared/customizableui/panelUIOverlay.inc.css which isn't a
> content stylesheet.

That's because there isn't a content stylesheet for the panel UI subviews. Making one for one ruleset didn't seem like a good idea and it seemed like PanelUI wasn't really following the content vs. skin pattern that well.

> Nearest-neighbor isn't more correct than interpolation.

It seemed like their was agreement that it's clearly better for pixel art like favicons when we added it for HiDPI OS X @ 2dppx. I agree it's more subjective for 1.25/1.5 but it still doesn't seem like something that we should leave up to themes to decide as it's not about theme images, it's about images that are content in the browser (i.e. provided by webpages) and I personally wouldn't expect themes to affect how images from a website are rendered.

> As a theme author I
> wouldn't copy this code, as I personally think it's doing more harm than
> good. Looking at the 1.25x rows in attachment 8466360 [details], it appears
> to me that nearest-neighbor makes all three icons look distorted, although
> in the special case of the facebook icon it doesn't look as unpleasant,
> since the vertical stroke of the "f" being thicker than the horizontal
> strokes doesn't matter much for the icon.

Do you think it does more harm than good for 2dppx? If not, would you be fine with wrapping these in media queries for 2dppx?

Comment 18

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #17)
> (In reply to Dão Gottwald [:dao] from comment #15)
> > This patch isn't consistent either, as it patches
> > browser/themes/shared/customizableui/panelUIOverlay.inc.css which isn't a
> > content stylesheet.
> 
> That's because there isn't a content stylesheet for the panel UI subviews.
> Making one for one ruleset didn't seem like a good idea and it seemed like
> PanelUI wasn't really following the content vs. skin pattern that well.

This can be in browser.css. It's unclear to my why panelUIOverlay.css exists in the first place. It's included as an ordinary stylesheet in browser.xul, not scoped or used in other documents or anything.

> Do you think it does more harm than good for 2dppx? If not, would you be
> fine with wrapping these in media queries for 2dppx?

For 2dppx it's a much easier sell since we wouldn't duplicate pixel rows and columns unevenly. Comment 16 would still be an issue there, although to a lesser extent, as it would only affect icons larger than 32*32 pixels...

Comment 19

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #16)
> There's a second, unrelated issue. image-rendering:-moz-crisp-edges affects
> down-scaled icons and images used as their own favicons in image documents.
> That effect seems clearly undesirable.

This is already a problem for HiDPI tab favicons on OS X so this bug wouldn't be making it worse for the image document case since I think they only get favicons on tabs but not for bookmarks or other UI not referring to the live page and the tabs already had image-rendering:-moz-crisp-edges applied.

I'm not as concerned about the downscaling case since I suspect it's much less common than exact matches or upscaling (especially if we use @sizes and get smarter about which icon to use). We also resize larger icons to 16x16 if their filesize is larger than 1KB at the moment for the non-tab case.

It sounds like what you want is to only change image-rendering for 2ddpx and that change in image-rendering should only apply to upscaling, not downscaling. Is this correct? AFAICT, we don't have an image-rendering value that does that so do the above two argument convince you that doing this for 2dppx for now is an acceptable compromise.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #19)
> (In reply to Dão Gottwald [:dao] from comment #16)
> > There's a second, unrelated issue. image-rendering:-moz-crisp-edges affects
> > down-scaled icons and images used as their own favicons in image documents.
> > That effect seems clearly undesirable.
> 
> This is already a problem for HiDPI tab favicons on OS X so this bug
> wouldn't be making it worse for the image document case since I think they
> only get favicons on tabs

Correct, except that this problem will now leak to Windows...

> It sounds like what you want is to only change image-rendering for 2ddpx and
> that change in image-rendering should only apply to upscaling, not
> downscaling. Is this correct? AFAICT, we don't have an image-rendering value
> that does that

Right, we don't have such a value. (If I were asked what I wanted despite our limitations, it would be interpolation with unsharp masking or something even fancier like http://research.microsoft.com/en-us/um/people/kopf/pixelart/.)

> so do the above two argument convince you that doing this for
> 2dppx for now is an acceptable compromise.

Sure, I didn't mean to say we shouldn't do it for 2dppx, although we could improve this further by setting an attribute on tab favicons when they're images from image documents and avoid setting image-rendering:-moz-crisp-edges in that case.

Comment 21

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #20)
> (In reply to Matthew N. [:MattN] from comment #19)
> > (In reply to Dão Gottwald [:dao] from comment #16)
> > > There's a second, unrelated issue. image-rendering:-moz-crisp-edges affects
> > > down-scaled icons and images used as their own favicons in image documents.
> > > That effect seems clearly undesirable.
> > 
> > This is already a problem for HiDPI tab favicons on OS X so this bug
> > wouldn't be making it worse for the image document case since I think they
> > only get favicons on tabs
> 
> Correct, except that this problem will now leak to Windows...

Right, but we have telemetry data that shows 2dppx on Windows is rare.

> > It sounds like what you want is to only change image-rendering for 2ddpx and
> > that change in image-rendering should only apply to upscaling, not
> > downscaling. Is this correct? AFAICT, we don't have an image-rendering value
> > that does that
> 
> Right, we don't have such a value. (If I were asked what I wanted despite
> our limitations, it would be interpolation with unsharp masking or something
> even fancier like
> http://research.microsoft.com/en-us/um/people/kopf/pixelart/.)

Can you file the graphics/imagelib bug so maybe we can get this in the future?

> > so do the above two argument convince you that doing this for
> > 2dppx for now is an acceptable compromise.
> 
> Sure, I didn't mean to say we shouldn't do it for 2dppx, although we could
> improve this further by setting an attribute on tab favicons when they're
> images from image documents and avoid setting
> image-rendering:-moz-crisp-edges in that case.

OK, so Bernardo will put the properties into 2dppx media queries in the same locations as his current patch. I think we can deal with the imagedocument case in a follow-up since it also affects OS X currently.

Does all this sound fine?

Comment 22

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #20)
> > Right, we don't have such a value. (If I were asked what I wanted despite
> > our limitations, it would be interpolation with unsharp masking or something
> > even fancier like
> > http://research.microsoft.com/en-us/um/people/kopf/pixelart/.)
> 
> Can you file the graphics/imagelib bug so maybe we can get this in the
> future?

What exactly?

> > Sure, I didn't mean to say we shouldn't do it for 2dppx, although we could
> > improve this further by setting an attribute on tab favicons when they're
> > images from image documents and avoid setting
> > image-rendering:-moz-crisp-edges in that case.
> 
> OK, so Bernardo will put the properties into 2dppx media queries in the same
> locations as his current patch. I think we can deal with the imagedocument
> case in a follow-up since it also affects OS X currently.
> 
> Does all this sound fine?

Yes, except for the theme/content confusion. At this point I don't care much where it's set, but it should be consistent.

Comment 23

[Matthew N. [:MattN]]

(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to Matthew N. [:MattN] from comment #21)
> > (In reply to Dão Gottwald [:dao] from comment #20)
> > > Right, we don't have such a value. (If I were asked what I wanted despite
> > > our limitations, it would be interpolation with unsharp masking or something
> > > even fancier like
> > > http://research.microsoft.com/en-us/um/people/kopf/pixelart/.)
> > 
> > Can you file the graphics/imagelib bug so maybe we can get this in the
> > future?
> 
> What exactly?

Asking for a new image-rendering property value that implements the behaviour you would like. I suppose it could also be filed as a CSS bug of ours.

> > > Sure, I didn't mean to say we shouldn't do it for 2dppx, although we could
> > > improve this further by setting an attribute on tab favicons when they're
> > > images from image documents and avoid setting
> > > image-rendering:-moz-crisp-edges in that case.
> > 
> > OK, so Bernardo will put the properties into 2dppx media queries in the same
> > locations as his current patch. I think we can deal with the imagedocument
> > case in a follow-up since it also affects OS X currently.
> > 
> > Does all this sound fine?
> 
> Yes, except for the theme/content confusion. At this point I don't care much
> where it's set, but it should be consistent.

OK, he'll address the panelUIOverlay.css issue and move that property to a content stylesheet.

Comment 24

[Bernardo Rittmeyer [:rittme]]

Attachment: Rev 2 - Resized favicons are now rendered using -moz-crisp-edges throughout the UI, when resolution is exaclty 2dppx

I moved the panelUIOverlay.css ruleset to browser.css.

I also used media queries to apply crisp edges rendering only to favicons at exactly 2dppx resolution.

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8470370 [details] [diff] [review]
Rev 2 - Resized favicons are now rendered using -moz-crisp-edges throughout the UI, when resolution is exaclty 2dppx

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

::: browser/base/content/browser.css
@@ +319,5 @@
>  #search-container {
>    min-width: 25ch;
>  }
>  
> +/* Crisp rendering should only be applied to exactly 2dppx resolution */

I kinda wonder if we should add "for favicons" to make it easier for people to understand what the blocks are doing: "Crisp rendering for favicons should…"

An alternative phrasing is:
"Apply crisp rendering for favicons at exactly 2dppx resolution".

Comment 26

[Bernardo Rittmeyer [:rittme]]

Attachment: rev 3 - Resized favicons are now rendered using -moz-crisp-edges throughout the UI, when resolution is exaclty 2dppx. r=MattN

Fixed the comments.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

Tried to verify this using latest Nightly (build ID: 20140820030202) on Windows 7 64bit, Mac OS X 10.9 and a Surface Pro 2 device running Windows 8.1 64bit (hiDPI) by comparing the favicons mentioned in comment 10 with the ones from latest Firefox Aurora (build ID: 20140820004001) but I could not spot any difference.

Is there something I'm missing here?

Comment 30

[Bernardo Rittmeyer [:rittme]]

Attachment: rendering comparison

The differences are really subtle and are only applicable to retina screens. 
You probably have to zoom in (without the smooth images feature turned on) to see something different. 
I'm attaching this image showing a comparison of two zoomed in favicons from the back and forward menu. The left one, 'auto', is how it was rendered in an older nightly. The right one, 'crips', is how it's rendered now. 
I hope this helps.

Comment 31

[Cornel Ionce [:noni] [Hubs QA]]

Unfortunately I don't have access to a retina screen device.
Juan, would you mind having a look at this?

Comment 32

[juan becerra [:juanb]]

I've verified this on the latest nightly using a 13" Macbook with Retina display. I used Facebook and Google icons to check the items in the list in comment #10 with the exception of the Social API part, because I wasn't able to enable it on Facebook (their page promoting this no longer exists).