Closed
Bug 1147702
Opened 10 years ago
Closed 10 years ago
Use a @2x Toolbar.png on Windows for basic hidpi support
Categories
(Firefox :: Theme, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: Dolske, Assigned: dao)
References
Details
Attachments
(5 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
image/png
|
Details |
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•10 years ago
|
Flags: needinfo?(shorlander)
Updated•10 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
Comment 1•10 years ago
|
||
Thank you!
Assignee | ||
Comment 2•10 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...
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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)
Comment 6•10 years ago
|
||
Adds 2x version of most of the icons visible in the main browser window. I might have missed a few.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 40.1 - 13 Apr
Points: --- → 5
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Thanks!
Assignee | ||
Comment 10•10 years ago
|
||
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•10 years ago
|
Attachment #8587844 -
Attachment description: WIP 2 → patch
Attachment #8587844 -
Flags: feedback?(MattN+bmo) → review?(MattN+bmo)
Assignee | ||
Comment 11•10 years ago
|
||
updated to tip
Attachment #8587844 -
Attachment is obsolete: true
Attachment #8587844 -
Flags: review?(MattN+bmo)
Attachment #8588077 -
Flags: review?(MattN+bmo)
Comment 12•10 years ago
|
||
(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•10 years ago
|
||
It can be fixed by using usercss (Stylus) and some @2x pngs.
Comment 14•10 years ago
|
||
Flags: needinfo?(qydwhotmail)
Comment 15•10 years ago
|
||
Flags: needinfo?(qydwhotmail)
Comment 16•10 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 17•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 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...
Assignee | ||
Comment 21•10 years ago
|
||
Flags: needinfo?(dolske)
Comment 22•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 23•10 years ago
|
||
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-
Comment 24•10 years ago
|
||
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•10 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•10 years ago
|
Attachment #8588864 -
Attachment is obsolete: true
Attachment #8588864 -
Flags: feedback+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(dao)
Assignee | ||
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•