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

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Dolske, Assigned: dao)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 40
x86
Windows 8.1
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Flags: needinfo?(shorlander)
Flags: qe-verify+
Flags: firefox-backlog+

Comment 1

3 years ago
Thank you!
(Assignee)

Comment 2

3 years ago
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)
Created attachment 8584500 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Iteration: --- → 40.1 - 13 Apr
Points: --- → 5
(Assignee)

Comment 7

3 years ago
Created attachment 8587499 [details] [diff] [review]
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.
(Assignee)

Comment 9

3 years ago
Thanks!
(Assignee)

Comment 10

3 years ago
Created attachment 8587844 [details] [diff] [review]
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.
Attachment #8587499 - Attachment is obsolete: true
Attachment #8587844 - Flags: feedback?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8587844 - Attachment description: WIP 2 → patch
Attachment #8587844 - Flags: feedback?(MattN+bmo) → review?(MattN+bmo)
(Assignee)

Comment 11

3 years ago
Created attachment 8588077 [details] [diff] [review]
patch

updated to tip
Attachment #8587844 - Attachment is obsolete: true
Attachment #8587844 - Flags: review?(MattN+bmo)
Attachment #8588077 - Flags: review?(MattN+bmo)
Created attachment 8588251 [details]
attachment  8588077 [details] [diff] [review] 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

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

Comment 14

3 years ago
Created attachment 8588861 [details]
Some 2x pngs and HiDPI.css included.
Flags: needinfo?(qydwhotmail)

Comment 15

3 years ago
Created attachment 8588864 [details]
Example by using hidpi.css
Flags: needinfo?(qydwhotmail)

Comment 16

3 years ago
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+

Comment 18

3 years ago
Created attachment 8590223 [details]
[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.
Attachment #8588861 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
(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.
(Assignee)

Comment 20

3 years ago
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
Last Resolved: 3 years ago
status-firefox40: --- → fixed
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-

Updated

3 years ago
Depends on: 1153209
(Assignee)

Updated

3 years ago
No longer depends on: 1153209

Comment 24

3 years ago
Created attachment 8591167 [details]
[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.
Flags: needinfo?(dao)
Attachment #8591167 - Flags: feedback+

Comment 25

3 years ago
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+

Updated

3 years ago
Attachment #8588864 - Attachment is obsolete: true
Attachment #8588864 - Flags: feedback+

Updated

3 years ago
Depends on: 1153323

Updated

3 years ago
Blocks: 1153529
Depends on: 1153952
(Assignee)

Updated

3 years ago
Depends on: 1155642
(Assignee)

Updated

3 years ago
Flags: needinfo?(dao)
Depends on: 1165360
Depends on: 1166083

Updated

3 years ago
Depends on: 1163636

Updated

3 years ago
Depends on: 1194725

Updated

3 years ago
Depends on: 1197624

Updated

3 years ago
Blocks: 1204375
(Assignee)

Updated

2 years ago
No longer blocks: 1204375
Depends on: 1204375
You need to log in before you can comment on or make changes to this bug.