1147702 - Use a @2x Toolbar.png on Windows for basic hidpi support

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Justin Dolske [:Dolske]]

A general fix for using hidpi icons on Windows (bug 1023511 & deps) has still not been prioritized, but as a short-term / quick improvement we would like to land a 200% version of Toolbar.png to incrementally improve the main toolbar icons. This is similar to what we did in bug 946987 for the initial Australis landing (and, uhh, still have not prioritized fixing bug 995733 to do that better, but I digress).

We'll need 2x versions of Toolbar.png, Toolbar-aero.png, Toolbar-inverted.png, and Toolbar-XP.png from /browser/themes/windows/, and the related CSS fixes to use the right image and region when appropriate.

[Looking at the DISPLAY_SCALING_MSWIN telemetry, we could arguably not bother with Toolbar-XP.png, as the hidpi usage on XP is extremely tiny, but I'm assuming it would be more of a hassle to _not_ fix it.]

NI shorlander for the image assets, but we could start working on the patch by just using an upscaled version.

Comment 1

[Mark Mayo [:mmayo]]

Thank you!

Comment 2

[Dão Gottwald [:dao]]

shorlander, according to telemetry, we have (if I recall correctly) roughly ten times more users with 1.25x than with 1.5x, and again a lot fewer users with 2x, so it might actually make sense to start with providing 1.25x assets and upscaling them for the larger scaling factors, or start with 1.5x, downscale for 1.25x and upscale for 2x. Either way I think we probably shouldn't start with optimizing for 2x if that means leaving 1.25x and 1.5x behind. That said, downscaling is better than upscaling, so maybe starting with 2x doesn't leave behind 1.25x and 1.5x that much...

Comment 3

[Blake Winton (:bwinton) (:☕️)]

As a data point, to hopefully help decide which size of images to create, here's a picture of the downscaled 2x bookmark menu icon in the various sizes: https://dl.dropboxusercontent.com/u/2301433/Firefox/Downsizing2x.png

(The incorrect height of the icon is probably due to an error in the code I wrote, not necessarily a problem with downscaling.  And on a personal note, I think downsizing the 2x looks just fine for 1.25x and 1.5x, so that's the approach I'ld recommend.)

Comment 4

[Sebastian Zartner [:sebo]]

For reference, I requested to replace all PNG images inside the DevTools by SVG counterparts in bug 1068939, so you don't have to care about resizing anymore.

And I think the same should be considered for this issue.

Sebastian

Comment 5

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #2)
> shorlander, according to telemetry, we have (if I recall correctly) roughly
> ten times more users with 1.25x than with 1.5x, and again a lot fewer users
> with 2x, so it might actually make sense to start with providing 1.25x
> assets and upscaling them for the larger scaling factors, or start with
> 1.5x, downscale for 1.25x and upscale for 2x. Either way I think we probably
> shouldn't start with optimizing for 2x if that means leaving 1.25x and 1.5x
> behind. That said, downscaling is better than upscaling, so maybe starting
> with 2x doesn't leave behind 1.25x and 1.5x that much...

After doing some experimentation I think the downscaling looks good but not perfect at all default scaling stops. The downscaling approach will give us decent coverage at all scaling factors. If it turns out to look too off we can reevaluate.

Ideally this is a temporary measure until we can implement some properly scaling assets (i.e. SVG).

Comment 6

[Stephen Horlander [:shorlander]]

Attachment: hidpi-icons-main-browser-window.patch - 01

Adds 2x version of most of the icons visible in the main browser window. I might have missed a few.

Comment 7

[Dão Gottwald [:dao]]

Attachment: WIP

This is proving hairy because I need to size the icons explicitly, which is more difficult with our toolbarbutton style on Windows than on OS X.

Also, I wanted to share code between OS X and Windows, and along the way I cleaned up some things and fixed some stuff that seemed broken. Unfortunately I can't test on OS X at the moment. If somebody wants to give this a quick try and tell me if something obvious is broken in the navigation toolbar, that would be appreciated.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

They seem okay to me.

https://dl.dropboxusercontent.com/u/2301433/Firefox/DaoWIP.png
https://dl.dropboxusercontent.com/u/2301433/Firefox/[e-mail]

Comment 9

[Dão Gottwald [:dao]]

Thanks!

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch

This takes care of Toolbar*.png and syncProgress-toolbar*.png. Other images will be updated in other bugs.

I still need to test this on Windows after my last minor modifications to the patch, but I think this should mostly be ready for review.

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch

updated to tip

Comment 12

[Matthew N. [:MattN]]

Attachment: attachment 8588077 mozscreenshots 10.9 HiDPI results

(I'm travelling today so have had limited connectivity)

I ran mozscreenshots with the following arguments on OS X 10.9 HiDPI and so far it didn't detect any regressions with a comparison to before your patch.

Buttons WindowSize TabsInTitlebar Toolbars LightweightThemes AppMenu --pref=browser.startup.homepage_override.mstone:40.0a1 --pref=browser.startup.homepage:data:text/plain,mozscreenshots --pref=datareporting.policy.dataSubmissionPolicyAcceptedVersion:2

I'm going to repeat this on Windows 8 now. Afterwards I will review the CSS changes.

Comment 13

[Fusion Future]

It can be fixed by using usercss (Stylus) and some @2x pngs.

Comment 14

[Fusion Future]

Attachment: Some 2x pngs and HiDPI.css included.

Comment 15

[Fusion Future]

Attachment: Example by using hidpi.css

Comment 16

[Fusion Future]

Comment on attachment 8588864 [details]
Example by using hidpi.css

The download indicator icon is still blurry but it seems I can't find a proper way...

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8588077 [details] [diff] [review]
patch

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

Sorry for the delay. I've been travelling an on PTO. It's also hard to review the patch since there are many things changing without any context.

r=me assuming the changes below are intentional. In the future it would be nicer to review if the different changes were in separate patches.

::: browser/base/content/browser.css
@@ -891,5 @@
> -toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
> -  max-width: 18px;
> -}
> -toolbarpaletteitem[place="palette"] > toolbarbutton.badged-button > .toolbarbutton-badge-container > .toolbarbutton-icon {
> -  max-width: 32px;

I'm assuming you tested a badged button

::: browser/themes/shared/toolbarbuttons.inc.css
@@ +365,5 @@
> +  #zoom-controls[cui-areatype="toolbar"] > #zoom-out-button {
> +    -moz-image-region: rect(0, 1116px, 36px, 1080px);
> +  }
> +
> +  #zoom-controls[cui-areatype="toolbar"] > #zoom-in-button {

The above zoom and clipboard controls have changed layout on Windows @1x as a result of this change. Is that intentional?

http://grab.by/GgyA

::: browser/themes/windows/browser.css
@@ +756,5 @@
>  }
>  
> +#nav-bar #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
> +  /* horizontal padding + border + actual icon width */
> +  width: 31px;

Is it intentional that the size of the bookmark menu button changed width on Windows @1x as a result of this patch? Are you correcting an image that wasn't shown at the correct proportions.

http://grab.by/GgyA

Comment 18

[Fusion Future]

Attachment: [2x.zip]Some 2x pngs and HiDPI.css included.

Before final patch, someone can use it to easily improve the blurry of the icons.

IMPORTANT:Before applying this stylesheet, please manually replace "file:///D:/Program%20Files/Iceweasel_X86/2x/" with the directory you place these files.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #17)
> > -toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
> > -  max-width: 18px;
> > -}
> > -toolbarpaletteitem[place="palette"] > toolbarbutton.badged-button > .toolbarbutton-badge-container > .toolbarbutton-icon {
> > -  max-width: 32px;
> 
> I'm assuming you tested a badged button

Yes, PanelUI-menu-button is a badged button.

> ::: browser/themes/shared/toolbarbuttons.inc.css
> @@ +365,5 @@
> > +  #zoom-controls[cui-areatype="toolbar"] > #zoom-out-button {
> > +    -moz-image-region: rect(0, 1116px, 36px, 1080px);
> > +  }
> > +
> > +  #zoom-controls[cui-areatype="toolbar"] > #zoom-in-button {
> 
> The above zoom and clipboard controls have changed layout on Windows @1x as
> a result of this change. Is that intentional?
> 
> http://grab.by/GgyA

The cited styles are for cui-areatype="toolbar", not the panel, and don't apply to @1x. I'll check if some other change could have had that effect.

> ::: browser/themes/windows/browser.css
> @@ +756,5 @@
> >  }
> >  
> > +#nav-bar #bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
> > +  /* horizontal padding + border + actual icon width */
> > +  width: 31px;
> 
> Is it intentional that the size of the bookmark menu button changed width on
> Windows @1x as a result of this patch? Are you correcting an image that
> wasn't shown at the correct proportions.
> 
> http://grab.by/GgyA

I made the bookmarks menu button as wide as other toolbar buttons.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8588077 [details] [diff] [review]
patch

> > The above zoom and clipboard controls have changed layout on Windows @1x as
> > a result of this change. Is that intentional?
> > 
> > http://grab.by/GgyA
> 
> The cited styles are for cui-areatype="toolbar", not the panel, and don't
> apply to @1x. I'll check if some other change could have had that effect.

It's the second selector of this rule:

>+toolbarbutton[cui-areatype="toolbar"] > :-moz-any(@nestedButtons@) > .toolbarbutton-icon,
>+:-moz-any(@primaryToolbarButtons@):-moz-any([cui-areatype="toolbar"],:not([cui-areatype])) > .toolbarbutton-icon,
>+:-moz-any(@primaryToolbarButtons@):-moz-any([cui-areatype="toolbar"],:not([cui-areatype])) > .toolbarbutton-badge-container > .toolbarbutton-icon,
>+:-moz-any(@primaryToolbarButtons@):-moz-any([cui-areatype="toolbar"],:not([cui-areatype])) > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+#bookmarks-menu-button[cui-areatype="toolbar"] > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {
>+  width: 18px;
>+}

I'll add :not(:-moz-any(@nestedButtons@)) to that...

Comment 21

[Dão Gottwald [:dao]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87f15ce8266d

Comment 23

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Deprioritizing on our list for Manual QA verification, since it's @2x assets mostly verified by Dão and Blake during implementation.

Comment 24

[Fusion Future]

Attachment: [20150410]Download indicator icon is still blurry.

After clicking the download icon, it becomes blurry.

And some addons' icons are stretched but some are not.

Comment 25

[Fusion Future]

Comment on attachment 8591167 [details]
[20150410]Download indicator icon is still blurry.

I think the problem comes from here (browser.css) and I make some changes.

#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;
}