Stephen Horlander [:shorlander] Reporter
	Description • 12 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 1 • 12 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 2 • 12 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 3 • 12 years ago

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.

	Stephen Horlander [:shorlander] Reporter
	Comment 4 • 12 years ago

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

	Stephen Horlander [:shorlander] Reporter
	Comment 5 • 12 years ago

Removed extra files and fixed names.

	Steven Michaud [:smichaud] (Retired)
	Comment 6 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 7 • 12 years ago

Can't we fix JarMaker?

	Steven Michaud [:smichaud] (Retired)
	Comment 8 • 12 years ago

> Can't we fix JarMaker?

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

	Frank Yan (:fryn) Assignee
	Comment 9 • 12 years ago

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?

	Steven Michaud [:smichaud] (Retired)
	Comment 10 • 12 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 11 • 12 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 12 • 12 years ago

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

	Steven Michaud [:smichaud] (Retired)
	Comment 13 • 12 years ago

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

	Steven Michaud [:smichaud] (Retired)
	Comment 14 • 12 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 15 • 12 years ago

See bug 674373 comment #217.

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

	Frank Yan (:fryn) Assignee
	Comment 16 • 12 years ago

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

	Frank Yan (:fryn) Assignee
	Comment 17 • 12 years ago

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

	Frank Yan (:fryn) Assignee
	Comment 18 • 12 years ago

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

	Asa Dotzler [:asa]
	Comment 19 • 12 years ago

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

	Michael Clackler
	Comment 20 • 12 years ago

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.

	Michael Clackler
	Comment 21 • 12 years ago

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

	Frank Yan (:fryn) Assignee
	Comment 22 • 12 years ago

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

	Frank Yan (:fryn) Assignee
	Comment 23 • 12 years ago

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.

	Michael Clackler
	Comment 24 • 12 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 25 • 12 years ago

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

	Stephen Horlander [:shorlander] Reporter
	Comment 26 • 12 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 27 • 12 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 28 • 12 years ago

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!

	Frank Yan (:fryn) Assignee
	Comment 29 • 12 years ago

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

	Stephen Horlander [:shorlander] Reporter
	Comment 30 • 12 years ago

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)

	Frank Yan (:fryn) Assignee
	Comment 31 • 12 years ago

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.

	Frank Yan (:fryn) Assignee
	Comment 32 • 12 years ago

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

	Frank Yan (:fryn) Assignee
	Comment 33 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 34 • 12 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 35 • 12 years ago

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.

	Frank Yan (:fryn) Assignee
	Comment 36 • 12 years ago

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.

	Frank Yan (:fryn) Assignee
	Comment 37 • 12 years ago

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

	Justin Dolske [:Dolske]
	Comment 38 • 12 years ago

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

	Frank Yan (:fryn) Assignee
	Comment 39 • 12 years ago

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.