Closed Bug 1054733 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(2 files)

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.
Update: on 10.6.3 there is no bookmarks icon in Safari, but using the above key-value pairs seems to work.
Attached patch Demo patchSplinter Review
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.
Attached patch 1054733.1.diffSplinter Review
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.
Attachment #8477839 - Flags: review?(mstange)
Status: NEW → ASSIGNED
Attachment #8477839 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/58a8748ae6ff
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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
Attachment #8477839 - Flags: approval-mozilla-aurora?
Attachment #8477839 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.