1153306 - The Loop/Hello icon is blurry in HiDPI mode on Windows

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P3)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Go to about:config and set layout.css.devPixelsPerPx to 2.0
Look at Hello icon on the toolbar

ER:
Just as crisp as the other icons

AR:
Blurry

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Shell, can we get this prioritized for Firefox 40? Bug 1147702 is going to ship with Firefox 40.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

In total, we'll want:
/browser/themes/windows/loop/[e-mail]
/browser/themes/windows/loop/[e-mail]
/browser/themes/windows/loop/[e-mail]

Comment 3

[Fusion Future]

The download indicator button is also blurry.
It just needs a small fix on browser.css, because [e-mail] already exists.

So will any mozillaian guys see it?


#downloads-indicator-icon {
  background: -moz-image-rect(url("chrome://browser/skin/[e-mail]"),
                              0, 396, 36, 360) center no-repeat;
  background-size: 18px;
  min-width: 18px;
  min-height: 18px;
}
#downloads-button[attention] > #downloads-indicator-anchor > #downloads-indicator-icon {
  background-image: -moz-image-rect(url("chrome://browser/skin/[e-mail]"), 36, 396, 72, 360);
  background-size: 18px;
}

#downloads-button:not([counter]) > #downloads-indicator-anchor > #downloads-indicator-progress-area > #downloads-indicator-counter {
  background: -moz-image-rect(url("chrome://browser/skin/[e-mail]"),
                              0, 396, 36, 360) center no-repeat;
  background-size: 12px;
}

Comment 4

[:shell escalante]

Hi Michael - can we get the assets that jared lists in Comment 2

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Fushan Wen from comment #3)

Just wanted to note that this was fixed in bug 1153529.

We are still waiting on the Hello graphics from Michael though. I'll send him an email.

Comment 6

[Bogdan Maris, Desktop Test Engineering]

I see that if Hello icon is moved to customization additional tools and features the icon is blurry as well without changing 'layout.css.devPixelsPerPx' that has value '-1' by default. I`m testing on Surface Pro 2 and Windows 7 64-bit 125 dpi. 

Happens only in Nightly and it`s a regression. 

m-i:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=223975f99ca3&tochange=8225a3b75df6 

Is this bug covering this behavior as well or I can go ahead and log a new issue?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #6)
> Happens only in Nightly and it`s a regression. 

I'm not sure how this is a regression. It may be more noticeable now that the other icons are crisp in the Customize Mode, but this should have been looking blurry since the beginning of Hello. 

> Is this bug covering this behavior as well or I can go ahead and log a new
> issue?

This bug is covering this behavior.

Comment 8

[Stephen Horlander [:shorlander]]

Attachment: loop-icons.zip

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 10

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

Comment on attachment 8616947 [details] [diff] [review]
Patch

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

Please revert the changes to the non-2x versions, as they seem to all have grown in size with no difference in the actual images. It probably wouldn't hurt chucking the 2x files through an image minimizer as well, though I've not checked for certain. See bug 1062969 for earlier work. (We really should be automizing this... though I'm not sure how that would work...)

::: browser/themes/shared/menupanel.inc.css
@@ +383,5 @@
> +
> +  #loop-button[cui-areatype="menu-panel"] > .toolbarbutton-badge-container > .toolbarbutton-icon,
> +  toolbarpaletteitem[place="palette"] > #loop-button > .toolbarbutton-badge-container > .toolbarbutton-icon {
> +    width: 32px;
> +    height: 32px;

This is new. Over in bug 1165679 we discussed fixing the rule in panelUIOverlay.css, and then (I think it was) Dão (who) added a plain width/height in it in another bug. But all the badge-containers are in there, and not the icons, and I bet that changing that will cause "issues" because of the margin that we also set.

I don't really understand why this is still necessary considering the container is already set to 32px. I'm assuming you tested it and found it necessary?

I think we should tidy these rules up a bit because right now we set width all over the place and the rules make little sense to me anymore. Can you file a followup bug for this?

::: browser/themes/windows/jar.mn
@@ +635,5 @@
>  % override chrome://browser/skin/[e-mail]                       chrome://browser/skin/[e-mail]                         os=WINNT osversion=6.1
>  % override chrome://browser/skin/loop/menuPanel.png                   chrome://browser/skin/loop/menuPanel-aero.png                     os=WINNT osversion=6
>  % override chrome://browser/skin/loop/menuPanel.png                   chrome://browser/skin/loop/menuPanel-aero.png                     os=WINNT osversion=6.1
> +% override chrome://browser/skin/loop/[e-mail]                chrome://browser/skin/loop/[e-mail]                     os=WINNT osversion=6
> +% override chrome://browser/skin/loop/[e-mail]                chrome://browser/skin/loop/[e-mail]                     os=WINNT osversion=6.1

You need to add similar overrides for the toolbar version...

@@ +641,5 @@
>  % override chrome://browser/skin/Toolbar.png                          chrome://browser/skin/Toolbar-aero.png                            os=WINNT osversion=6
>  % override chrome://browser/skin/Toolbar.png                          chrome://browser/skin/Toolbar-aero.png                            os=WINNT osversion=6.1
>  % override chrome://browser/skin/Toolbar.png                          chrome://browser/skin/Toolbar-XP.png                              os=WINNT osversion<6
>  % override chrome://browser/skin/loop/toolbar.png                     chrome://browser/skin/loop/toolbar-aero.png                       os=WINNT osversion=6
>  % override chrome://browser/skin/loop/toolbar.png                     chrome://browser/skin/loop/toolbar-aero.png                       os=WINNT osversion=6.1

... here, I think? Both for aero and XP (!)

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #10)
> Please revert the changes to the non-2x versions, as they seem to all have
> grown in size with no difference in the actual images. It probably wouldn't
> hurt chucking the 2x files through an image minimizer as well, though I've
> not checked for certain.

I checked with PNGCrush and all of the @2x images were already optimized. I've reverted the 1x images.

> ::: browser/themes/shared/menupanel.inc.css
> @@ +383,5 @@
> > +
> > +  #loop-button[cui-areatype="menu-panel"] > .toolbarbutton-badge-container > .toolbarbutton-icon,
> > +  toolbarpaletteitem[place="palette"] > #loop-button > .toolbarbutton-badge-container > .toolbarbutton-icon {
> > +    width: 32px;
> > +    height: 32px;
> 
> I don't really understand why this is still necessary considering the
> container is already set to 32px. I'm assuming you tested it and found it
> necessary?

Without this rule, the icon has a box model with width:64px, height:32px when in the menupanel. The width and height seem to be coming from the -moz-image-region. I can't figure out why the height is reduced but the width not. 
 
> I think we should tidy these rules up a bit because right now we set width
> all over the place and the rules make little sense to me anymore. Can you
> file a followup bug for this?

Filed bug 1173401.

> ::: browser/themes/windows/jar.mn
> You need to add similar overrides for the t

Good catch. Thanks!

Comment 12

[Pulsebot]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/646bc6779d35

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for 40

Approval Request Comment
[Feature/regressing bug #]: Windows 10 HiDPI
[User impact if declined]: HiDPI icons on Windows are blurry
[Describe test coverage new/current, TreeHerder]: on mozilla-central for much of 41-nightly
[Risks and why]: none expected 
[String/UUID change made/needed]: none

https://hg.mozilla.org/try/pushloghtml?changeset=71084a9edf1e

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8627208 [details] [diff] [review]
Patch for 40

On m-c for 3 weeks already. Visual fix in support of Windows 10. Beta+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/13af5e0adac4

Comment 17

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

Confirming the fix on Windows 10 64-bit using:
- latest Aurora, build ID: 20150706004011
- Firefox 40 beta 2, build ID: 20150706172413.