Closed Bug 1147702 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Theme, defect)

x86
Windows 8.1
defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: Dolske, Assigned: dao)

References

Details

Attachments

(5 files, 4 obsolete files)

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.
Flags: needinfo?(shorlander)
Flags: qe-verify+
Flags: firefox-backlog+
Thank you!
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...
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.)
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
(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).
Flags: needinfo?(shorlander)
Adds 2x version of most of the icons visible in the main browser window. I might have missed a few.
Status: NEW → ASSIGNED
Iteration: --- → 40.1 - 13 Apr
Points: --- → 5
Attached patch WIP (obsolete) — Splinter Review
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.
Thanks!
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8587499 - Attachment is obsolete: true
Attachment #8587844 - Flags: feedback?(MattN+bmo)
Attachment #8587844 - Attachment description: WIP 2 → patch
Attachment #8587844 - Flags: feedback?(MattN+bmo) → review?(MattN+bmo)
Attached patch patchSplinter Review
updated to tip
Attachment #8587844 - Attachment is obsolete: true
Attachment #8587844 - Flags: review?(MattN+bmo)
Attachment #8588077 - Flags: review?(MattN+bmo)
(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.
It can be fixed by using usercss (Stylus) and some @2x pngs.
Attached file Some 2x pngs and HiDPI.css included. (obsolete) —
Flags: needinfo?(qydwhotmail)
Attached image Example by using hidpi.css (obsolete) —
Flags: needinfo?(qydwhotmail)
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...
Flags: needinfo?(dolske)
Attachment #8588864 - Flags: feedback+
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
Attachment #8588077 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8588861 - Attachment is obsolete: true
(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 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...
https://hg.mozilla.org/mozilla-central/rev/87f15ce8266d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Deprioritizing on our list for Manual QA verification, since it's @2x assets mostly verified by Dão and Blake during implementation.
Flags: qe-verify+ → qe-verify-
Depends on: 1153209
No longer depends on: 1153209
Depends on: 1153243
After clicking the download icon, it becomes blurry.

And some addons' icons are stretched but some are not.
Flags: needinfo?(dao)
Attachment #8591167 - Flags: feedback+
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/Toolbar@2x.png"),
                              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/Toolbar@2x.png"), 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/Toolbar@2x.png"),
                              0, 396, 36, 360) center no-repeat;
  background-size: 12px;
}
Attachment #8591167 - Flags: feedback+
Attachment #8588864 - Attachment is obsolete: true
Attachment #8588864 - Flags: feedback+
Depends on: 1153323
Blocks: 1153529
Depends on: 1153952
Depends on: 1155642
Flags: needinfo?(dao)
Depends on: 1165360
Depends on: 1166083
Depends on: 1163636
Depends on: 1194725
Depends on: 1197624
Blocks: 1204375
No longer blocks: 1204375
Depends on: 1204375
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: