781327 - Use 2x images for HiDPI browser UI

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Stephen Horlander [:shorlander]]

Attachment: HiDPI Assets - i01

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>[e-mail]

Maybe use relative sizes: x-small, small, medium, large, xlarge, huge, suerhuge. But that is probably out of scope for this :)

Comment 1

[Jonathan Kew [:jfkthame]]

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.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

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 3

[Steven Michaud [:smichaud] (Retired)]

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:

[e-mail]
[e-mail]
[e-mail]
[e-mail]
[e-mail]
[e-mail]

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.

Comment 4

[Stephen Horlander [:shorlander]]

(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.

> [e-mail]
/toolkit/themes/pinstripe/global/icons/close.png

> [e-mail]
/toolkit/themes/pinstripe/global/tree/folder.png

> [e-mail]
/browser/themes/pinstripe/Geolocation-16.png

> [e-mail]
/browser/themes/pinstripe/webapps-16.png

> [e-mail]
/browser/themes/pinstripe/places/query.png

> [e-mail]
/browser/themes/pinstripe/places/tag.png

I will upload a new zip with fixed names.

Comment 5

[Stephen Horlander [:shorlander]]

Attachment: HiDPI Assets - i01

Removed extra files and fixed names.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> - 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.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Can't we fix JarMaker?

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> Can't we fix JarMaker?

I think that's out-of-scope for this bug.  But I did open bug782324 on the subject.

Comment 9

[Frank Yan (:fryn)]

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?

Comment 10

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch (*not* a fix)

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.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

(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().

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #11:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-01c7b200c5b2/try-macosx64/firefox-17.0a1.en-US.mac.dmg

Comment 14

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

See bug 674373 comment #217.

Whatever we do here will probably need to deal with that.

Comment 16

[Frank Yan (:fryn)]

Attachment: 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):
layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

Comment 17

[Frank Yan (:fryn)]

(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.

Comment 18

[Frank Yan (:fryn)]

Attachment: 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

Comment 19

[Asa Dotzler [:asa]]

(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?

Comment 20

[Michael Clackler]

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.

Comment 21

[Michael Clackler]

Another point noticed is that the arrow in the downloads toolbar button is way to large and overflowing the button size.

Comment 22

[Frank Yan (:fryn)]

Attachment: HiDPI Icons DLC add-on i1.6 - install this on top of tryserver build

(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.

Comment 23

[Frank Yan (:fryn)]

Attachment: patch i1.6 - requires fixes from bug 674373

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.

Comment 24

[Michael Clackler]

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.

Comment 25

[Jonathan Kew [:jfkthame]]

(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.

Comment 26

[Stephen Horlander [:shorlander]]

Attachment: HiDPI Assets - i02

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

Comment 27

[Jonathan Kew [:jfkthame]]

(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.

Comment 28

[Jonathan Kew [:jfkthame]]

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!

Comment 29

[Frank Yan (:fryn)]

(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. :)

Comment 30

[Stephen Horlander [:shorlander]]

Attachment: HiDPI Assets - i03

Added more 2x icons:

- [e-mail]
- [e-mail]  (for new tab page)
- [e-mail]
- [e-mail]  (yay throbbers!)
- [e-mail]
- [e-mail]


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)

Comment 31

[Frank Yan (:fryn)]

Attachment: patch

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.

Comment 32

[Frank Yan (:fryn)]

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. :)

Comment 33

[Frank Yan (:fryn)]

Attachment: patch

Unbitrot. Forgot that I had just landed a single-line change to a jar.mn on mozilla-central.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 35

[Jonathan Kew [:jfkthame]]

Stephen (and Frank) - I just noticed a discrepancy between the "full-screen" button icons in Toolbar-lion.png and [e-mail]. In the 1x version, the arrows point to top-left/bottom-right, while in the 2x version, they're top-right/bottom-left.

Comment 36

[Frank Yan (:fryn)]

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

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

(In reply to :Gavin Sharp (use [e-mail] 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/[e-mail]");
>   }
...
> }

::: 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/[e-mail]), 0, 280, 40, 240);

Oops, this should be a chrome:// obviously. I fixed it in my local copy.

Comment 37

[Frank Yan (:fryn)]

(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 [e-mail]. 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 38

[Justin Dolske [:Dolske]]

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.]

Comment 39

[Frank Yan (:fryn)]

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 [e-mail]. 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.