Closed Bug 781327 Opened 8 years ago Closed 8 years ago

Use 2x images for HiDPI browser UI

Categories

(Firefox :: Theme, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: shorlander, Assigned: fryn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 7 obsolete files)

166.32 KB, patch
Details | Diff | Splinter Review
83.36 KB, application/x-xpinstall
Details
246.32 KB, application/zip
Details
372.76 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
Attached file HiDPI Assets - i01 (obsolete) —
Bug 674373 is handling the platform changes needed for displaying HiDPI content. We also need to update the browser chrome to use higher resolution images.

I am attaching a zip file that contains most of the main window images at 2x resolution. Still missing a few things and I am working on the secondary UI next. Might also need some tweaking, but it should be enough to get started with.

Some notes:
- I used the "@2x" naming convention as suggested in Apple's documentation even though we don't use the same packaging as a native app

- In addition to the browser chrome we also need to support incontent UI, e.g., about:home, about:newtab

We also might want to consider changing our naming conventions for icon sizes. Currently we use <name>-16, <name>-32, <name>-48, etc. Which doesn't make much sense when you double the size and tack on "@2x": <name>-48@2x.png

Maybe use relative sizes: x-small, small, medium, large, xlarge, huge, suerhuge. But that is probably out of scope for this :)
You should be able to detect HiDPI mode within CSS using a media query such as

@media (min-resolution: 2dppx) {
  /* rules to load the appropriate hi-res images */
}

However, I don't think this by itself is sufficient; they'll be treated in exactly the same way as the existing small images, which get pixel-doubled for rendering on a retina screen so as to appear the "right" size. So here, you'll get the larger images used, but they'll just be... larger. We need to load the 2x images *and* tell the platform that it should now avoid pixel-doubling them.
Assignee: nobody → smichaud
Comment on attachment 650278 [details]
HiDPI Assets - i01

There are two copies of most files here -- some at the top level and some in Retina-Assets-i01.zip inside the zipfile.

For the record, shorlander told me to use the ones at the top level.
Comment on attachment 650278 [details]
HiDPI Assets - i01

There are several files in this zipfile (at the top level) for which I can't find counterparts in current trunk code:

close-tab@2x.png
folder-16@2x.png
notification-geolocation-16@2x.png
notification-webapps-16@2x.png
query-16@2x.png
tag-16@2x.png

I searched the tree for e.g. "close-tab.png", then grepped the tree for e.g. "close-tab".  I also searched (and grepped) for "notification-geolocation" and "notification-webapps".  I didn't find anything.

So for the moment I won't include these files in my patch.
(In reply to Steven Michaud from comment #3)
> There are several files in this zipfile (at the top level) for which I can't
> find counterparts in current trunk code:

Some of these are in sub folders or toolkit and I accidentally renamed a few.

> close-tab@2x.png
/toolkit/themes/pinstripe/global/icons/close.png

> folder-16@2x.png
/toolkit/themes/pinstripe/global/tree/folder.png

> notification-geolocation-16@2x.png
/browser/themes/pinstripe/Geolocation-16.png

> notification-webapps-16@2x.png
/browser/themes/pinstripe/webapps-16.png

> query-16@2x.png
/browser/themes/pinstripe/places/query.png

> tag-16@2x.png
/browser/themes/pinstripe/places/tag.png

I will upload a new zip with fixed names.
Attached file HiDPI Assets - i01 (obsolete) —
Removed extra files and fixed names.
Attachment #650278 - Attachment is obsolete: true
> - I used the "@2x" naming convention as suggested in Apple's documentation
> even though we don't use the same packaging as a native app

JarMaker.py chokes on the '@' character.  So (on Monday) I'll try changing this to "-2x".

Sigh.
Can't we fix JarMaker?
> Can't we fix JarMaker?

I think that's out-of-scope for this bug.  But I did open bug782324 on the subject.
Hi Steven, may I take this bug from you?
Do I need to apply any patches in bug 674373 that are not on mozilla-central to work on this?
This bug is *much* harder than I expected it to be -- which is why I haven't yet posted a patch.

Markus' "native theme rendering changes" patch from bug 674373 comment #62 is no help at all.  And so far I get exactly the same (bad) results with or without Jonathan Kew's latest patch from bug 674373.

I'm going to keep working on this, probably well into next week.

But I don't think it'd hurt to have us both working on this bug simultaneously.  There are probably any number of ways in which this bug might be fixed, and it'd be interesting to compare what we come up with.
Here's what I've been able to manage so far.

It doesn't work properly (as you'll see when the tryserver build is done).  But its logging shows the places where all the assets added by this patch are drawn (and possibly where all assets loaded from *.css files are drawn):

nsImageBoxFrame::PaintImage() (in /layout/xul/base/src/nsImageBoxFrame.cpp)
nsTreeBodyFrame::PaintImage() (in /layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp)

We'll need to intervene "nearby" to get the 2x images working properly.
(Following up comment #11)

The @media at-command isn't the best way to load these 2x images.

We want to decide which images to use (2x or 1x) "near" where those images are drawn.  For that to work, we need to pass more information down from the *.css files than we're currently doing -- ideally that for each asset there are a set of images available to use, each at a different resolution.

Apple and Google have already done some work on a solution to this problem -- they've designed and implemented the -webkit-image-set() CSS function:

http://lists.w3.org/Archives/Public/www-style/2012Feb/1103.html
http://trac.webkit.org/changeset/111637

Rather than try to reinvent the wheel, I think we should try to implement this ourselves.  For the next few days (into next week), I'll be trying to scope out what this would involve.  I'll use the existing Webkit and Chromium implementations as resources.

Since the image-set() function isn't yet part of any standard, we'd presumably call our implementation -moz-image-set().
Turns out I'm not going to have time to continue work on this for a while.

So Frank Yan's going to take it, after all.
See bug 674373 comment #217.

Whatever we do here will probably need to deal with that.
Assignee: smichaud → fryn
Status: NEW → ASSIGNED
Here is an add-on that makes most of the icons in Firefox high-DPI.
It uses the HiDPI Assets i01 that Stephen uploaded.
The add-on requires some built-in HiDPI platform support, so install it while using the following tryserver build by Jonathan Kew (from bug 674373 comment 228):
layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
Attachment #657078 - Flags: ui-review?(shorlander)
(In reply to Frank Yan (:fryn) from comment #16)
> Created attachment 657078 [details]
> HiDPI Icons DLC add-on i01 - install this on top of tryserver build
> 
> Here is an add-on that makes most of the icons in Firefox high-DPI.
> It uses the HiDPI Assets i01 that Stephen uploaded.
> The add-on requires some built-in HiDPI platform support, so install it
> while using the following tryserver build by Jonathan Kew (from bug 674373
> comment 228):

Clipboard fail. The correct link for the build is:
https://people.mozilla.com/~fyan/tmp/firefox-hidpi-wip.mac.dmg

To be clear, this is plenty hacky and simply a way to achieve the desired result without having to modify the platform further.
Fixed identity icon and new tab button glitches.
Now 80% less hacky.

Like before, install this while using the following build:
https://people.mozilla.com/~fyan/tmp/firefox-hidpi-wip.mac.dmg
Attachment #657078 - Attachment is obsolete: true
Attachment #657078 - Flags: ui-review?(shorlander)
Attachment #657137 - Flags: ui-review?(shorlander)
(In reply to Frank Yan (:fryn) from comment #18)
> Created attachment 657137 [details]
> HiDPI Icons DLC add-on i1.5 - install this on top of tryserver build
> 
> Fixed identity icon and new tab button glitches.
> Now 80% less hacky.
> 
> Like before, install this while using the following build:
> https://people.mozilla.com/~fyan/tmp/firefox-hidpi-wip.mac.dmg

I installed, and this is beautiful.  What are the next steps in turning this into shipping code?
This looks really good.  The only think I can see is the "check mark" icon used to indicate the active tab in the forward/back and "all tabs" drop down menus is the low-res icon.  I did not see it in the HiDPI assets icon zip file.
Another point noticed is that the arrow in the downloads toolbar button is way to large and overflowing the button size.
(In reply to Asa Dotzler [:asa] from comment #19)
> I installed, and this is beautiful.  What are the next steps in turning this
> into shipping code?

Bug 674373 needs to be fixed first. There are still several glitches in the patches there, including Flash not getting scaled properly and rects not getting redrawn properly in the UI sometimes.

My add-on's code is rather isolated. It will only affect people using Retina displays, so the risk is low.

(In reply to Michael Clackler from comment #21)
> Another point noticed is that the arrow in the downloads toolbar button is
> way to large and overflowing the button size.

Fixed in this update.
Attachment #657137 - Attachment is obsolete: true
Attachment #657137 - Flags: ui-review?(shorlander)
The add-on in patch form.
It requires the fixes from bug 674373.

I lumped all the @2x images in the same folder and all the CSS rules in one block at the bottom of browser/themes/pinstripe/browser.css purely for short-term convenience.
In the long term, we should of course organize the rules and files with their corresponding non-2x entities.

I'd prioritize landing a fix that works, even if it's front-end-only and looks something like this, over waiting for a generalized solution that covers the platform bits that Steven explained in the above comments, if we can't get that soon.
Download button is definitely fixed with latest version.  I just noticed the circle around the back button is not "dimmed" when the browser looses focus as it does on the regular nightly.
(In reply to Frank Yan (:fryn) from comment #23)
> I'd prioritize landing a fix that works, even if it's front-end-only and
> looks something like this, over waiting for a generalized solution that
> covers the platform bits that Steven explained in the above comments, if we
> can't get that soon.

Agreed. We may well want to implement some kind of image-set() feature eventually, but we don't need to let that block a front-end fix here.

One more small image that doesn't seem to have HiDPI treatment yet is the downward-pointing arrow (triangle) that appears, for example, in the Bookmarks toolbar items that have associated drop-down menus. (The similar-sized triangle in the Search field is hi-res, however.)

Oh, and also the "dotted square" that appears in tab headers and bookmark items when there's no favicon to show, and the progress spinners that appear in the tab headers while the page is loading.
Attached file HiDPI Assets - i02 (obsolete) —
Fixed the inactive back button and added some additional 2x icons for:

- /browser/themes/pinstripe/actionicon-tab.png
- /toolkit/themes/pinstripe/global/icons/chevron.png
- /toolkit/themes/pinstripe/mozapps/places/defaultFavicon.png
- /browser/themes/pinstripe/places/folderDropArrow.png
- /browser/themes/pinstripe/Geolocation-64.png
- /browser/themes/pinstripe/places/history.png
- /browser/themes/pinstripe/panel-expander-closed.png
- /browser/themes/pinstripe/panel-expander-open.png
- /browser/themes/pinstripe/places/unfiledBookmarks.png
Attachment #650957 - Attachment is obsolete: true
(In reply to Frank Yan (:fryn) from comment #23)
> Created attachment 657719 [details] [diff] [review]
> patch i1.6 - requires fixes from bug 674373
> 
> The add-on in patch form.

Oops - looks like you forgot to "hg add" the actual graphics to this patch. If you could also update it to include the new resources from Stephen's latest archive, that'd be great.

There's a tryserver build that includes this patch (but without CSS support for the newest icons as mentioned in comment #26) at https://tbpl.mozilla.org/?tree=Try&rev=50af84dce998. Looking pretty good to me; the HiDPI Icons add-on is no longer needed to get crisp UI.
I'm hoping we'll be able to land initial HiDPI support in bug 674373 fairly soon.

Frank, any chance you could update the patch here to include the latest hi-res resources from Stephen (and any other changes you think are needed) so we can get this reviewed and landed soon too? Thanks!
(In reply to Jonathan Kew (:jfkthame) from comment #28)
> I'm hoping we'll be able to land initial HiDPI support in bug 674373 fairly
> soon.
> 
> Frank, any chance you could update the patch here to include the latest
> hi-res resources from Stephen (and any other changes you think are needed)
> so we can get this reviewed and landed soon too? Thanks!

Yup, I'll have a shippable patch with the necessary changes to make this shippable uploaded tonight. :)
Attached file HiDPI Assets - i03
Added more 2x icons:

- connecting@2x.png
- controls@2x.png  (for new tab page)
- identity@2x.png
- loading@2x.png  (yay throbbers!)
- page-livemarks@2x.png
- Secure-Glyph@2x.png


Also when updating some of the icons I found that I didn't have a good scalable version so I took the opportunity to modernize them. So I had to update some 1x icons to match their new 2x counterparts:

- bookmarksToolbar.png
- controls.png  (for new tab page)
- folderDropArrow.png
- geolocation-16.png
- history.png
- identity.png
- unfiledBookmarks.png


I also resized one old icon to match the new 2x icon:

- chevron.png  (from 11x8 to 13x9)
Attachment #658348 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Jonathan, this patch is ready for review and for integration in any tryserver builds you make. :)

Stephen, feel free to apply it to verify that everything looks right.
Attachment #657719 - Attachment is obsolete: true
Attachment #665464 - Flags: review?(gavin.sharp)
Comment on attachment 665464 [details] [diff] [review]
patch

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

Gavin, it's a large patch, but it's almost all PNGs and copy-paste, so a review shouldn't be very difficult, if you trust that I did my arithmetic correctly. :)
Attached patch patchSplinter Review
Unbitrot. Forgot that I had just landed a single-line change to a jar.mn on mozilla-central.
Attachment #665464 - Attachment is obsolete: true
Attachment #665464 - Flags: review?(gavin.sharp)
Attachment #665481 - Flags: review?(gavin.sharp)
Comment on attachment 665481 [details] [diff] [review]
patch

Dolske said he'd take this review - bouncing to him.

For bug posterity and my own edification, can you explain the general approach in more detail? I'm a bit confused about why some styles need to specify a different size, while some changes are just changing the image, etc.
Attachment #665481 - Flags: review?(gavin.sharp) → review?(dolske)
Stephen (and Frank) - I just noticed a discrepancy between the "full-screen" button icons in Toolbar-lion.png and Toolbar-lion@2x.png. In the 1x version, the arrows point to top-left/bottom-right, while in the 2x version, they're top-right/bottom-left.
Comment on attachment 665481 [details] [diff] [review]
patch

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

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #34)
> For bug posterity and my own edification, can you explain the general
> approach in more detail? I'm a bit confused about why some styles need to
> specify a different size, while some changes are just changing the image,
> etc.

Sure. The basic approach is, for each usage of each image, to make a new rule in which the 1x path is replaced with the 2x path and explicitly scale down the image to the number of CSS pixels at which it should be displayed.
In some cases, because there is already a rule that locks the size, I don't need to provide a duplicate rule. For example, see the following case:

> .bookmark-item > .toolbarbutton-icon {
>   width: 16px;
>   min-height: 16px;
>   max-height: 16px;
> }

Here, due to bookmark-items having images loaded from potentially anywhere, this rule already existed to prevent breakage.
Therefore, when I wrote the following rule, I didn't need to specify the size again:

> @media (min-resolution: 2dppx) {
>   .bookmark-item {
>     list-style-image: url("chrome://mozapps/skin/places/defaultFavicon@2x.png");
>   }
...
> }

::: browser/themes/pinstripe/downloads/downloads.css
@@ +212,5 @@
> +    background-size: 20px;
> +  }
> +
> +  #downloads-indicator:not([counter]) #downloads-indicator-counter {
> +    background-image: -moz-image-rect(url(images/Toolbar-lion@2x.png), 0, 280, 40, 240);

Oops, this should be a chrome:// obviously. I fixed it in my local copy.
(In reply to Jonathan Kew (:jfkthame) from comment #35)
> Stephen (and Frank) - I just noticed a discrepancy between the "full-screen"
> button icons in Toolbar-lion.png and Toolbar-lion@2x.png. In the 1x version,
> the arrows point to top-left/bottom-right, while in the 2x version, they're
> top-right/bottom-left.

Good catch! Stephen and I will fix that before pushing. :)
Comment on attachment 665481 [details] [diff] [review]
patch

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

FTR clarification: we've decided to do things this way as a short-term solution to getting better OS X hidpi appearance. The intention is to replace this with a better mechanism at some point. The HTML5 specs for this stuff is still evolving and our platform support for this is still new, but we want to ship something ASAP.

1) Let's file a followup bug on figuring out the One True Way for doing this in the long term.

2) Let's also file a followup metabug for further HiDPI improvements; we know there are still a variety of image resources we haven't had time to update or are tricky to update. This bug/patch is mostly for getting primary UI going. [I say metabug because I suspect there are enough followup tweaks that a ongoing series of "yet more hidpi stuff" will be hard to follow.]
Attachment #665481 - Flags: review?(dolske) → review+
Pushed straight to mozilla-central as recommended by Dolske:
https://hg.mozilla.org/mozilla-central/rev/e10fd37030e3

> (In reply to Jonathan Kew (:jfkthame) from comment #35)
> > Stephen (and Frank) - I just noticed a discrepancy between the "full-screen"
> > button icons in Toolbar-lion.png and Toolbar-lion@2x.png. In the 1x version,
> > the arrows point to top-left/bottom-right, while in the 2x version, they're
> > top-right/bottom-left.

We didn't get to this yet, so I am filing a followup bug for it.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Version: unspecified → Trunk
Blocks: 795664
Blocks: 795665
Blocks: 795667
Depends on: 797230
Blocks: 817366
No longer blocks: 795665
Depends on: 834591
Depends on: 834592
Blocks: 795665
No longer depends on: 834592
No longer depends on: 834591
You need to log in before you can comment on or make changes to this bug.