845516 - Use 2x images for menu checkmarks

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Richard Marti (:Paenglab)]

Use 2x images for checkmarks (the menuitems with type="radio" to show which one is selected)

Comment 1

[Richard Marti (:Paenglab)]

Attachment: patch

Stephen, if you have better images for the checkmarks then please attach them.

I have no Retina Mac and can't check how it looks with HiDPI but it should look better than with the 1x images.

Comment 2

[Stephen Horlander [:shorlander]]

Attachment: Menu Checkmark @2x

Comment 3

[Stephen Horlander [:shorlander]]

Attachment: Menu Checkmark

Patch looked pretty good, was just a little off. Updated the menu-check.png to match the latest ones in OS X. I had to resize the normal sized image from 20 x 11 to 22 x 11.

Comment 4

[Stephen Horlander [:shorlander]]

Comment on attachment 718621 [details]
patch

See attachments

Comment 5

[Richard Marti (:Paenglab)]

Attachment: patch v2

Patch with your new images. I haven't changed the margins or paddings.

Stephen, please can you check if the space to the menu border and to the menu text is okay?

Comment 6

[Stephen Horlander [:shorlander]]

Comment on attachment 718928 [details] [diff] [review]
patch v2

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

Alignment looks good on normal res, in HiDPI the checkmark is 1px too high compared to native.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: patch v3

Moved the HiDPI checkmark 1px downwards with margin-top 1px; margin-bottom: -1px;

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 719119 [details] [diff] [review]
patch v3

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

::: toolkit/themes/osx/global/menu.css
@@ +180,5 @@
>  :not(menulist) > menupopup > menuitem[selected="true"][_moz-menuactive="true"]:not([disabled="true"]) > .menu-iconic-left {
> +  background-image: -moz-image-rect("chrome://global/skin/menu/menu-check.png", 0, 22, 11, 11);
> +}
> +
> +@media (-moz-mac-lion-theme) {

Why this?

I would have expected:

@media (min-resolution: 2dppx) {

(which is what's used in other hidpi patches, and restricts the changes only to Retina screens)

@@ +184,5 @@
> +@media (-moz-mac-lion-theme) {
> +  :not(menulist) > menupopup > menuitem[checked="true"] > .menu-iconic-left,
> +  :not(menulist) > menupopup > menuitem[selected="true"] > .menu-iconic-left {
> +    margin-top: 1px;
> +    margin-bottom: -1px;

Hmm. This is a little gross.

But, on contrast to comment 6, with this patch I see the checkmark as 1px too _low_ on hidpi! [1 device pixel, so 0.5 CSS pixels]

I wonder if the check image is being centered vertically, and pixel-snapping avoids the problem on lodpi but not hidpi. Lemme think about how to avoid this...

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

I tried fiddling so that the PNG was 24px tall (thus 12 css px), thinking that it might be a problem due to even/odd rounding. But that had no effect. :/ I thought about making it 23px tall, but that would break the convention of @2x images being exactly twice the dimensions of the lodpi image (since 11.5 is impossible). So I didn't try.

Tweaking the margin-top/margin-bottom to 0.5 exactly fixes the alignment. Checked on both menus with an favicon and without.

I'm willing to roll with this if Frank is.

Comment 10

[Richard Marti (:Paenglab)]

(In reply to Justin Dolske [:Dolske] from comment #8)
> Comment on attachment 719119 [details] [diff] [review]
> patch v3
> 
> Review of attachment 719119 [details] [diff] [review]:
> -----------------------------------------------------------------

> > +@media (-moz-mac-lion-theme) {
> 
> Why this?
> 
> I would have expected:
> 
> @media (min-resolution: 2dppx) {
> 

Oops, sorry. I don't know how this slipped in.

Comment 11

[Frank Yan (:fryn)]

Comment on attachment 720203 [details] [diff] [review]
Patch v.4

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

::: toolkit/themes/osx/global/menu.css
@@ +167,5 @@
>  :not(menulist) > menupopup > menuitem[checked="true"] > .menu-iconic-left,
>  :not(menulist) > menupopup > menuitem[selected="true"] > .menu-iconic-left {
>    -moz-margin-start: -15px;
>    -moz-padding-start: 15px;
> +  background: -moz-image-rect("chrome://global/skin/menu/menu-check.png", 0, 11, 11, 0) bottom left no-repeat transparent;

The problem is that the .menu-iconic-left is being centered vertically within a container with a height of opposite parity (even/odd) to .menu-iconic-left's client height.

Inserting `padding-top: 1px;` above the `-moz-margin-start: -15px;` line should address this with no need for the half-pixel margins below.
A comment could be included, e.g.:
/* match parities of client height and container height */

Dolske, what do you think of this in comparison to the margins?

Comment 12

[Frank Yan (:fryn)]

Comment on attachment 720203 [details] [diff] [review]
Patch v.4

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

Discussed this briefly with dolske on IRC.
I think I prefer the padding rule, and I don't anticipate any layout problems from it.

I verified that the PNGs are optimized.

r+ with the padding rule.

Comment 13

[Frank Yan (:fryn)]

Attachment: patch for checkin [feedback=dolske] [review=fryn]

Here's the updated patch with commit user and message.

Comment 14

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/339085b4e9e6

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/339085b4e9e6