Closed Bug 1012445 Opened 6 years ago Closed 6 years ago

Implement NS_THEME_CHECKMENUITEM for Mac OS X

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(3 files, 5 obsolete files)

We currently use css to display the menuitem checkmark. We could use the standard NSImageNameMenuOnStateTemplate icon provided by the OS instead.

Apple's docs are a bit unclear about the usage of this icon - for some reason it's listed under "Toolbar Named Images" with the comment: "A check mark. Drawing these outside of menus is discouraged.", but the icon looks and behaves the same as the standard OS menuitem checkmark.
Attached patch wip (obsolete) — Splinter Review
Just putting a wip patch here...
Blocks: 643184
Attached patch Another wip (obsolete) — Splinter Review
I thought I was done, but I made the mistake of not looking at the icon in detail... It appears that the icon is slightly lighter than the one in the native menus. I've seen that before when looking at the menuarrow icon and should have thought of it.

It took me some time to figure out a possible reason for the difference:

Apple uses CoreUI to draw these icons and Markus showed me for some time ago how to find out what dictionary values that are passed to CUIDraw for a certain widget/icon.

This is the output I get when the checkmark is rendered in Firefox’s native menu:

Process 17679 resuming
Command #2 'c' continued the target.
{
    backgroundTypeKey = kCUIBackgroundTypeMenu;
    imageNameKey = "image.MenuOnState";
    state = normal;
    widget = image;
}

The output when a checkmark in a xul menu renders looks like this (with my patch applied):
Process 17679 resuming
Command #2 'c' continued the target.
{
    backgroundTypeKey = backgroundTypeLight;
    imageIsGrayscaleKey = 1;
    imageNameKey = "image.MenuOnStateTemplate";
    kCUIUserInterfaceLayoutDirectionKey = kCUIUserInterfaceLayoutDirectionLeftToRight;
    state = normal;
    widget = image;
}

Note that it doesn't matter if I use the same NSImage as in the native menu (it's identical to the template version and the result is exactly the same) 
As you can see the output is different from the checkmark in the native menu. For example, the backgroundTypeKey value is different. The backgroundTypeKey value for the xul menu checkmark comes from this code:
+  [mIconCell setBackgroundStyle:CheckBooleanAttr(aFrame, nsGkAtoms::menuactive) &&
+                                !isDisabled ? NSBackgroundStyleDark : NSBackgroundStyleLight];

By alterning the backgroundStyle of the NSImagecell the icon will change appearance when the menuitem is selected/hovered. However, the checkmark in the native menu seems to use a backgroundStyle that is not public and my guess is that that's the reason for the difference.

I'll see if I can come up with something here...
Attachment #8425655 - Attachment is obsolete: true
Markus suggested trying with some different numbers in setBackgroundStyle, to see if that would result in CUIDraw using the kCUIBackgroundTypeMenu style. I’ve used 0-20 and then looked at the output from CUIDraw. There is actually one more background style (there are 4 documented), but that’s not the one we want: using 4, 12 or 20 will result in CUIDraw using kCUIBackgroundTypeDarkRaised.

I'll play a bit with CUIDraw and see if there might be some other factor that affects this.
OK, so it looks like the kCUIBackgroundTypeMenu is the key here. I hacked up some code to let CUIDraw do the drawing and this is the result:
--------------------------------------------------
    backgroundTypeKey = backgroundTypeLight;
    imageIsGrayscaleKey = 1;
    imageNameKey = "image.MenuOnStateTemplate";
    "is.flipped" = 1;
    state = normal;
    widget = image;
Removing the imageIsGrayscaleKey entry doesn't make a difference, but changing the backgroundTypeKey does. The below entries works - that is, the icon looks as it should (with or without the imageIsGrayscaleKey entry):

    backgroundTypeKey = kCUIBackgroundTypeMenu;
    imageIsGrayscaleKey = 1;
    imageNameKey = "image.MenuOnStateTemplate";
    "is.flipped" = 1;
    state = normal;
    widget = image;
Attached patch Demo patch using CUIDraw (obsolete) — Splinter Review
Here's a patch with the CUIDraw approach. Note that I haven't fixed the vertical positioning. But it would be nice to do the NSImageCell approach - maybe there is a way to apply the filters manually?
Comment on attachment 8436156 [details] [diff] [review]
Demo patch using CUIDraw

Ah, forgot to mention that if you use backgroundTypeLight the image won't change if you hover the menuitem (that is, "pressed" doesn't work).

(and ignore the browser.css change)
How about something completely different: What does the result look like if you use HIThemeDrawMenuItem with a HIThemeMenuItemDrawInfo that has kThemeMenuSelected in the ThemeMenuState and kThemeMenuItemNoBackground in the ThemeMenuItemType? Do you get a checkmark at all?
The docs for this are in HITheme.h and Appearance.h.
I guess you mean something like this:

@@ -2207,26 +2263,26 @@ nsNativeThemeCocoa::DrawWidgetBackground
         // Clear the background to get correct transparency.
         CGContextClearRect(cgContext, macRect);
       }
 
       // maybe use kThemeMenuItemHierBackground or PopUpBackground instead of just Plain?
       HIThemeMenuItemDrawInfo drawInfo;
       memset(&drawInfo, 0, sizeof(drawInfo));
       drawInfo.version = 0;
-      drawInfo.itemType = kThemeMenuItemPlain;
-      drawInfo.state = (IsDisabled(aFrame, eventState) ?
-                         static_cast<ThemeMenuState>(kThemeMenuDisabled) :
-                         CheckBooleanAttr(aFrame, nsGkAtoms::menuactive) ?
-                           static_cast<ThemeMenuState>(kThemeMenuSelected) :
-                           static_cast<ThemeMenuState>(kThemeMenuActive));
+      drawInfo.itemType = kThemeMenuItemNoBackground;
+      drawInfo.state = static_cast<ThemeMenuState>(kThemeMenuSelected);

What happens is that the menuitem turns blue :-) The examples I've seen using HITheme for checkmarks involves drawing a textbox and then using a unicode character, so I don't think we can use HITheme for this.
Ouh. Thanks, it was worth a try :)
I have discovered that the normal (not disabled/highlighted) icons seems to just be the original (not processed) image with 0.75 alpha. For example, doing this (excerpts, but you get the idea) will produce an icon that is almost identical with the real ”native” one.
————————————————————————————————————————————
NSImage *image = [NSImage imageNamed:@"NSMenuOnState”];
[image setTemplate:NO];
.
.
.

[newImage lockFocus];
[image compositeToPoint:NSZeroPoint fromRect:NSZeroRect operation:NSCompositeSourceOver fraction:0.75];
[newImage unlockFocus];
[mIconCell setObjectValue:newImage];
————————————————————————————————————————————

I haven’t analyzed the output more than just taking screenshots and comparing the rendered icons in Pixie. And I’m saying ”almost” identical above, because the comparisons shows that some pixel alpha values differ slightly from the ”real” ones, but that shouldn’t be an issue here (we’re talking about very small differences). It could also be my sampling :-)

I have also tested with the NSMenuSubmenu and NSMenuScrollDown NSImages’s and they also match, so it’s probably valid for all template icons in menus. If this approach works, the same thing can be done in bug 333910 as well.

Before I’ll do anything else, I’ll have a look at the other states (disabled and highlighted).
OK, so the disabled state will also need manual processing. Leaving the processing to the OS produces an image that is too dark (interestingly, our theme icon is also darker than the native). But the highlighted state could be left to the OS.
I first thought that drawing the highlighted state manually would require a Core Image filter, but I was wrong. Using the NSCompositingOperation constant NSCompositeDestinationIn with a white destination rect gives us what we want.
Attached patch Draw all states by hand (obsolete) — Splinter Review
Now when I discovered that we could draw the highlighted state by hand I couldn’t resist to put together a patch that implements it. This patch also draws the image without the help of NSCell.
Attachment #8440307 - Attachment description: 1012445.1.diff → Draw all states by hand
Markus, I need to think a bit more of how I treat the rect's sizes, but does the "manual drawing" approach in attachment #8440307 [details] [diff] [review] looks reasonable to you?
Flags: needinfo?(mstange)
Sorry for the delay here. The patch looks good, but I like the CUIDraw approach more because it's closer to what OS X does internally, and less code.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #15)
> Sorry for the delay here. The patch looks good, but I like the CUIDraw
> approach more because it's closer to what OS X does internally, and less
> code.

I suspected that ;-) I'll put up a new patch later on this week.
Attached patch CSS partSplinter Review
This is the css part (I'll let Marcus have a look at the widget part first).
Attachment #8435386 - Attachment is obsolete: true
Attachment #8436156 - Attachment is obsolete: true
Attachment #8440307 - Attachment is obsolete: true
Attached patch Widget part (obsolete) — Splinter Review
The idea here is of course to also use DrawMenuIcon in bug 333910. What makes me a bit confused is the HIDPI rendering, so I might do something wrong when I fiddle with the sizes. I don’t have a retina mac, but I have tested the patch on 10.9 with Quartz Debug (to simulate HIDPI mode) and the checkmark scales nicely, but...
Attachment #8447736 - Flags: review?(mstange)
Attached patch Widget partSplinter Review
I've made a few style changes and rewrote the alignment code a bit. The previous code was a little confusing to me. Does this look good to you and does it still do what you intended?
Attachment #8448719 - Flags: feedback?(stefanh)
I'll have a look, but note that this will only be valid for the checkmark (but that could be changed later on in bug 333910, of course:
+static const CGFloat kMenuIconIndent = 6.0f;
Comment on attachment 8448719 [details] [diff] [review]
Widget part

Thanks for the suggestions. What I wanted to do was to align the checkmark properly (6) either to the left or the right and also handle the case when the menuitem rect is smaller than the icon (do no alignment in that case).

I think your alignment code will not align the checkmark at all in RTL mode and move the checkmark to the rightmost end in LTR mode. Let me see if I can come up with something that you like better than the previous.
Attachment #8448719 - Flags: feedback?(stefanh)
How about this? If the menuitem rect size is equal to or less than the icon size, we don't align. Otherwise we use what have left over for alignment (but honor kMenuIconIndent).
Attachment #8447736 - Attachment is obsolete: true
Attachment #8447736 - Flags: review?(mstange)
Attachment #8448902 - Flags: review?(mstange)
Comment on attachment 8447735 [details] [diff] [review]
CSS part

Dao, mind take a look at this? I don't think adjustments of the widget part will have any impact here (and I'm leaving for vacation on Saturday).
Attachment #8447735 - Flags: review?(dao)
Comment on attachment 8448902 [details] [diff] [review]
Widget part - new version with alignment

Oh, yes, my std::max should have been an std::min, good catch. Is there a reason not to keep "CGFloat paddingEndX = paddingX - paddingStartX;"?
Attachment #8448902 - Flags: review?(mstange)
Attachment #8448902 - Flags: review+
Attachment #8448902 - Flags: review+
(In reply to Markus Stange [:mstange] from comment #24)
> Comment on attachment 8448902 [details] [diff] [review]
> Widget part - new version with alignment
> 
> Oh, yes, my std::max should have been an std::min, good catch. Is there a
> reason not to keep "CGFloat paddingEndX = paddingX - paddingStartX;"?

"CGFloat paddingEndX = std::max(0.0, paddingX - kMenuIconIndent);" should handle the RTL position here. paddingX - kMenuIconIndent is the distance we want to move the icon in RTL, if paddingX is less than kMenuIconIndent, we don't move the icon to the right.
Attachment #8447735 - Flags: review?(dao) → review+
And I pushed the above to try yesterday (as one changeset): https://tbpl.mozilla.org/?tree=Try&rev=580542a41d07
https://hg.mozilla.org/mozilla-central/rev/47b307e47648
https://hg.mozilla.org/mozilla-central/rev/3b4802b7826d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1054733
You need to log in before you can comment on or make changes to this bug.