981419 - [OSX] Toolbar checkmarks in toolbar context menu are cut off and unrecognizable

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[:Gijs (he/him)]

Attachment: Screenshot

I'm guessing it's somehow getting the panel styles applied, which it shouldn't...

Comment 1

[:Gijs (he/him)]

(same happens on another profile which has the web developer toolbar, when I tick that item)

The main View > Toolbars submenu seems to be fine...

Comment 2

[Justin Dolske [:Dolske]]

Ah:

-  skin/classic/global/menu/menu-check.png                            (menu/menu-check.png)
-  skin/classic/global/menu/[e-mail]                         (menu/[e-mail])
+  skin/classic/global/menu/menu-check.png                            (../../shared/menu-check.png)
+  skin/classic/global/menu/[e-mail]                         (../../shared/[e-mail])

The problem is that the old OS X menu-check.png is a 22x11 image, containing two 11x11 checkmarks (the extra is for the menuitem hover state, white-on-blue). The new image is a 16x16 image, containing a single centered 13x12 checkmark. The references in toolkit/themes/osx/global/menu.css need the old image.

Assuming the slightly-larger checkmark is intentional, the new panel checkmark should just be a new image instead of replacing the old one.


(In reply to :Gijs Kruitbosch from comment #1)

> The main View > Toolbars submenu seems to be fine...

That's a real Cocoa menu, so our stuff doesn't bother it.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut at the patch.

How's this for a patch?  Make sense?  Name isn't too horrible?
(Tested on OS X, in theory should work on Windows/Linux. ;)

Comment 4

[:Gijs (he/him)]

Comment on attachment 8389194 [details] [diff] [review]
The first cut at the patch.

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

Ship it. I was totally going to do exactly this. I considered naming this "bookmarks-menu-arrow" instead, but for something in toolkit this is better.

Comment 5

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/569c2bd2234e

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/569c2bd2234e

Comment 8

[:Gijs (he/him)]

Comment on attachment 8389194 [details] [diff] [review]
The first cut at the patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: checkmarks all over OS X's UI look bad after landing bug 979378
Testing completed (on m-c, etc.): locally, on fx-team, on m-c
Risk to taking this patch (and alternatives if risky): low, necessary for bug 979378 to work right
String or IDL/UUID changes made by this patch: none

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a7c0d70f4c5