1054733 - [OS X ≤ 10.7] Checkmark in menuitem doesn't highlight (invert) when menuitem is hovered

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stefan [:stefanh]]

I have tested this on 10.6.8 and 10.7.0. The problem here is that Apple didn't used CUIDraw to render checkmarks and menuarrows on these earlier OS versions. It works fine on 10.7.5, though. So, at some point (10.7.1-10.7.5) Apple switched to render checkmarks and menuarrows with CUIDraw.

It's only the highlight that is affected here, normal and disabled states look fine. So apparently CUIDraw works to some extent.

I've spent some time testing this and it appears that Apple did used CUIDraw for some other icons, though. On 10.6.8, gdb output for the the generic Bookmarks icon looks like this:

Normal state:
-------------
Breakpoint 1, 0x00007fff89bdce6e in CUIDraw ()
{
    backgroundTypeKey = backgroundTypeLight;
    imageIsGrayscaleKey = 1;
    imageNameKey = "image.Bookmark";
    state = normal;
    widget = image;
}

Highlighted state:
------------------
Breakpoint 1, 0x00007fff89bdce6e in CUIDraw ()
{
    backgroundTypeKey = backgroundTypeDark;
    imageIsGrayscaleKey = 1;
    imageNameKey = "image.Bookmark";
    state = normal;
    widget = image;
}

Note that the state doesn't change, but the backgroundTypeKey do. Iotw, if we don't want to draw the icon by hand (like https://bug1012445.bugzilla.mozilla.org/attachment.cgi?id=8440307), it should be possible to use CUIDraw for the checkmark with some tweaks to the existing code.

Comment 1

[Stefan [:stefanh]]

Update: on 10.6.3 there is no bookmarks icon in Safari, but using the above key-value pairs seems to work.

Comment 2

[Stefan [:stefanh]]

Attachment: Demo patch

Here's a demo patch that uses the "old" method on versions before 10.8. What bothers me is that I need to conditionally add one key-value pair.

Comment 3

[Stefan [:stefanh]]

Attachment: 1054733.1.diff

I have tested this on 10.6.3, 10.6.8, 10.7.0, 10.7.5 and 10.9.4. From 10.7.5 we match the OS color. On 10.6.3 - 10.7.0 the checkmark colors differs slightly, but I don't think that matters. As you can see, the differences are quite small (darkest area):
10.6.8: native, normal state = rgb(0.42, 0.42, 0.43)
10.6.8: xul, normal state = rgb(0.32, 0.32, 0.32)
10.6.8: native, disabled state = rgb(0.71, 0.71, 0.72)
10.6.8: xul, disabled state = rgb(0.59, 0.59, 0.59)

Interestingly, Apple used a different checkmark on the earlier versions - they didn't used the NSMenuOnState image that we now use. But we used one css image for all OS versions before, so I don't think that matters.

I have also double-checked the CUIDraw output on all versions above except 10.6.3 and the only thing that differ us from from Apple is the @"is.flipped" key (which we need - otherwise the icon will be upside-down). It's worth to note that Apple doesn't seem to have used CUIDraw for menu icons on 10.6.3, but it is possible to use CUIDraw and it works.

Comment 4

[Stefan [:stefanh]]

https://hg.mozilla.org/try/rev/0c0eb0721490

Comment 5

[Stefan [:stefanh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58a8748ae6ff

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/58a8748ae6ff

Comment 7

[Stefan [:stefanh]]

Comment on attachment 8477839 [details] [diff] [review]
1054733.1.diff

Approval Request Comment
[Feature/regressing bug #]: Regression from bug 1012445
[User impact if declined]: Checkmark will not "highlight" when menuitem is hovered for users on older OS X versions
[Describe test coverage new/current, TBPL]: Been on m-c since 2014-08-26
[Risks and why]: Low risk - this will make us use a more correct CUIDraw input on earlier version of OS X.
[String/UUID change made/needed]: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e276d1a68b63

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Should have been:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c9102769c741