418774 - -moz-image-region does not work in Mac OSX menus

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Ed Hume]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12

SphereGnome uses -moz-image-region: rect(...) code to specify icons in an image block to be used on menuitems in Firefox dropdown and context menus. 

This code works in Windows and Linux. In Mac OSX, the menu icons are compressed versions of the entire image block. Even folder-item.png is presented as a compressed block. The -moz-image-region: rect(...) code does not appear to be working.

Reproducible: Always

Steps to Reproduce:
1. Install SphereGnome, ColorGnome, FormalGnome or HiVisGnome on a Mac.
2. Select one of those themes to be used and restart.
Actual Results:  
Individual icon images are not shown.

Expected Results:  
Individual icon images should be shown.

The Noia themes reportedly work by not using -moz-image-region: rect(...) code to specify icons. The problem with this method is that it requires the browser to load an enormous number of individual image files. This would be an extra burden with my themes because my themes show active states, and often show hover states as well. The main block of SphereGnome menu icons has 122 icons on it, with around 2.5 images per icon. The Big and Jumbo versions have even more icons.

-moz-image-region: rect(...) really needs to be activated on OSX menus.

Comment 1

[Stefan [:stefanh]]

This is actually a cocoa widget issue (widget/src/cocoa/nsMenuItemIconX.mm). Note that active/hover state of images is not working either iirc - but that should probably be filed as a separate issue.

Comment 2

[KLB]

If this is a widget (whatever those are?) issue, why does it not cause problems with toolbar buttons, as those all use "-moz-image-region" as well?  The only place that the "-moz-image-region" is not working correctly is on the menulists.

Comment 3

[Stefan [:stefanh]]

(In reply to comment #2)
> If this is a widget (whatever those are?) issue, why does it not cause problems
> with toolbar buttons, as those all use "-moz-image-region" as well?  The only
> place that the "-moz-image-region" is not working correctly is on the
> menulists.
> 

No, "-moz-image-region" works in XUL, but it doesn't work in native mac menus. On mac, an application window doesn't have a menubar. Instead, the menubar is displayed at the top of the desktop. Basically, the mac widget code takes the menubar with its menus and puts it on top of the desktop. The problem here is that using "-moz-image-region" to display icons in a menuitem rendered/created by the mac widget code doesn't work.

Comment 4

[Ed Hume]

Thanks for the clear explanation of our trouble.

And because Firefox is trying to behave as a proper OSX app, we won't be seeing our menus show up in xul, will we?

Comment 5

[Stefan [:stefanh]]

(In reply to comment #4)
> Thanks for the clear explanation of our trouble.
> 
> And because Firefox is trying to behave as a proper OSX app, we won't be seeing
> our menus show up in xul, will we?
> 

Correct. But this is only about menus in the window's menubar. A bookmarks folder menu, menulist, context menu etc will be in XUL.

Comment 6

[KLB]

I think I understand why the proper folder icon is not showing up in menu lists, but the question is what is our solution?  Is it going to be fixed in the widget or do we have to create a stand alone icon for folders in menu lists?

Comment 7

[KLB]

My solution to this was to create three separate menu icons rather than to use a single multi-icon PNG.  this eliminated the need for "-moz-image-region"

An example of the changed code (from browser.css) is:

/* ::::: bookmark items ::::: */

.bookmark-item  {
  list-style-image: url("chrome://global/skin/icons/menu-item.png");
}

.bookmark-item[container] {
  list-style-image: url("chrome://global/skin/icons/folder-closed.png");
}

.bookmark-item[container][open] {
  list-style-image: url("chrome://global/skin/icons/folder-opened.png");
}

Comment 8

[Ed Hume]

That will work fine for older-item.png. However, in SphereGnome and related themes I provide icons for at least 122 different menu items. Each one has a normal and active state. Some have disabled states. In SphereGnome_Big I generally provide hover icons as well. Although I could split up these massive files, loading and storing the resultant collection of hundreds of images would surely cause a performance hit for Firefox.

Comment 9

[Samuel Sidler (old account; do not CC)]

As this was true in Firefox 2, we aren't going to take it in a Firefox 3 maintenance release, but requested wanted1.9.1 to get it on the radar for the next major version.

Comment 10

[Alfred Kayser]

Any progress on this? Anyone that sees a way to fix this?
See comment #8 why this is so needed.
As long as this bug is not fixed themers either remove all menu icons from Mac menus, or have to add a lot of images to the theme file, just for the Mac menu's... The latter has big performance impact (as loading and caching 122 images does generate serious memory and cpu load).

Comment 11

[Stefan [:stefanh]]

The only way this'll get fixed is if someone adds support for -moz-image-region in widget/src/cocoa/nsMenuItemIconX.mm

Comment 12

[Alfred Kayser]

Attachment: Concept: Slide&dice icon image before setting it as an menu icon

This is a piece of code that shows how one can do some slice&dice on the image (i.e. apply -moz-image-region) before setting the image as the icon for the menu item. 

Two hard parts here:
1. I don't have a Mac, so I won't be able to compile and test this...
2. Getting the values from the nsDOMIRect from the -moz-image-region style rule requires some walking through the nsCSSIPrimitiveValues...

Ad 1. Someone with access to a Mac could solve this part.
Ad 2. Someone with knowledge about the nsDOMIRect and nsCSSIPrimitiveValue things could very easily address this.

Comment 13

[Robert Kaiser]

this is wanted1.9.1+ and we currently have some visual glitches in SeaMonkey due to this, is there any chance to still get a fix in 1.9.1? Else, we'll need to go and split up our images we assign to Mac menus.

Comment 14

[Stian Oksavik]

For the record, this is still happening with the Mac OS X build of 3.1 beta 2. I haven't tried a Mercurial checkout yet.

Comment 15

[Ben Frisch]

Attachment: Real Patch to Slice & Dice the Image Icon before Setting It - v.1

The attached patch should implement the concept discussed earlier as long as the values in -moz-image-region are in pixels.  I decided that if the CSS for  -moz-image-region is invalid, the entire image should be shown instead, but I'm flexible.  I also made the alpha first swap code ignore the stride if there is no -moz-image-region specified.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 411913 [details] [diff] [review]
Real Patch to Slice & Dice the Image Icon before Setting It - v.1

Thanks for doing this, Benjamin! I'd like to review this, too.

Comment 17

[Ben Frisch]

Attachment: Real Patch to Slice & Dice the Image Icon before Setting It - v.2

This patch should actually handle all of the possible invalid pixel values by just drawing the entire image.  The patch also takes the stride into account when cropping the image.

Comment 18

[Markus Stange [:mstange]]

In normal XUL menus, does -moz-image-region also apply to images set with the "image" attribute?

Comment 19

[Ben Frisch]

Attachment: Real Patch to Slice & Dice the Image Icon before Setting It - v.3

I did some tests and -moz-image-region does only apply to images set with list-style-image.  As such I modified the patch to only check -moz-image-region if there is a list-style-image.  I also modified the patch to handle the case where the region is the full image height or width but not the full other dimension properly by showing it as specified.

Comment 20

[Markus Stange [:mstange]]

+  PRPackedBool          mHasImageRegion;
   NSMenuItem*           mNativeMenuItem; // [weak]
+  float                 mImageTop;
+  float                 mImageRight;
+  float                 mImageBottom;
+  float                 mImageLeft;

Instead of storing the coordinates as four floats, could you maybe store it as one nsIntRect? Then you'd have to move part of the width/height error testing into GetIconURI. Also, you could remove mHasImageRegion and have an empty rect signal "no image region" (and then test for mImageRegion.IsEmpty()).

+      // Get Image Top Left Dimension Value in the Image Region

I think you mean "Get Image Region Top...".

+      nsCOMPtr<nsIDOMCSSPrimitiveValue> dimensionValue;
+      imageRegionRect->GetTop(getter_AddRefs(dimensionValue));
+      primitiveValue = do_QueryInterface(dimensionValue);

You save a line here: dimensionValue is already a nsIDOMCSSPrimitiveValue, so you can use that instead of primitiveValue in the lines following. The detour over do_QueryInterface isn't necessary. The code example I'll give further down will clarify this.

Overall, getting the four rect sides is very repetitive. If you want to save some more lines, here's some code that would make it a little shorter:

-----------------------

typedef nsresult (nsIDOMRect::*GetRectSideMethod)(nsIDOMCSSPrimitiveValue**);

static PRInt32
GetDOMRectSide(nsIDOMRect* aRect, GetRectSideMethod aMethod)
{
  nsCOMPtr<nsIDOMCSSPrimitiveValue> dimensionValue;
  (aRect->*aMethod)(getter_AddRefs(dimensionValue));
  if (!dimensionValue)
    return -1;

  PRUint16 primitiveType;
  nsresult rv = dimensionValue->GetPrimitiveType(&primitiveType);
  if (NS_FAILED(rv) || primitiveType != nsIDOMCSSPrimitiveValue::CSS_PX)
    return -1;

  float dimension = 0;
  rv = dimensionValue->GetFloatValue(nsIDOMCSSPrimitiveValue::CSS_PX, &dimension);
  if (NS_FAILED(rv))
    return -1;

  return NSToIntRound(dimension);
}

and then in GetIconURI it's just:

  PRInt32 top = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetTop);
  PRInt32 right = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetRight);
  PRInt32 bottom = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetBottom);
  PRInt32 left = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetLeft);

--------------------------

@@ -352,32 +445,77 @@ nsMenuItemIconX::OnStopFrame(imgIRequest
                                           imgIContainer::FLAG_NONE,
                                           getter_AddRefs(image));
   if (NS_FAILED(rv) || !image)
     return NS_ERROR_FAILURE;
 
   PRInt32 height = image->Height();
   PRInt32 stride = image->Stride();
   PRInt32 width = image->Width();
-  PRUint32 imageLength = ((stride * height) / 4);
   if ((stride % 4 != 0) || (height < 1) || (width < 1))
     return NS_ERROR_FAILURE;
+  PRInt32 imageLength;

Don't declare this variable here. Declare it further down when you can initialize it at the same time. It's no problem to have two variables with the same name in the same function as long as they're in different code blocks.

   PRUint32* imageData = (PRUint32*)image->Data();
+  PRUint32* reorderedData;

Same for this variable.

+  if (mHasImageRegion) {
+    // Calculate sizes of the clipped image, and handle invalid CSS securely by
+    // just showing the whole image.

Out of interest, do you know how invalid -moz-image-region values are handled usually? It would probably be good to use the same error handling, but it's not super important.

+    PRInt32 newHeight = mImageBottom - mImageTop;
+    PRInt32 newWidth = mImageRight - mImageLeft;
+    if (newHeight > height || newHeight < 1 || newWidth > width || 
+        newWidth < 1 || mImageBottom > height || mImageTop > height || 
+        mImageTop < 0 || mImageRight > width || mImageLeft > width ||
+        mImageLeft < 0)

I think at least two comparisons here are unnecessary, but the proof isn't obvious to me right now... anyway, it doesn't really matter.

+    /* Use the clipped image dimensions */

//-style comments please.

I haven't quite wrapped my head around the stride stuff but it's probably fine. However, I'd prefer it if these two code blocks were unified, i.e. just regard the "no image region" case as a "image region spans the whole image" case. This will save more code lines.

Comment 21

[Josh Aas]

Comment on attachment 412933 [details] [diff] [review]
Real Patch to Slice & Dice the Image Icon before Setting It - v.3

Canceling my review until patch is updated per mstange's comments.

Comment 22

[Ben Frisch]

Attachment: Real Patch to Slice & Dice the Image Icon before Setting It - v.4

This patch should address all of your comments.  Thanks for the GetDOMRectSide function.  That cut back on code.  I took a look at Windows, and when it encounters invalid CSS in menus it will not draw the image except in an edge case where for example, if you use something like -moz-image-region: rect(16px, 32px, 32px, -16px); and the image is 32x48, it ends up drawing 8x16 portion of the image.  So, rather than try to figure out the exact reason for the edge case (I looked in nsImageFrame and it's probably related to a lack of checking of the nsIntRect.x value and the way images are scaled), I just changed the code to just remove the potential placeholder image and not draw an image on all error cases.  While I like the almost cross platform consistency, I am concerned that this would break some themes, especially ones that use moz-image-region on Windows but don't change their CSS on Mac OS to use moz-image-region: auto.  So, I am open to going back to drawing the whole image on error if that is preferred instead.

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 414189 [details] [diff] [review]
Real Patch to Slice & Dice the Image Icon before Setting It - v.4

This is looking really good!

>diff --git a/widget/src/cocoa/nsMenuItemIconX.h b/widget/src/cocoa/nsMenuItemIconX.h
>--- a/widget/src/cocoa/nsMenuItemIconX.h
>+++ b/widget/src/cocoa/nsMenuItemIconX.h
>@@ -40,25 +40,29 @@
>  * Retrieves and displays icons in native menu items on Mac OS X.
>  */
> 
> #ifndef nsMenuItemIconX_h_
> #define nsMenuItemIconX_h_
> 
> #include "nsCOMPtr.h"
> #include "imgIDecoderObserver.h"
>+#include "nsIDOMRect.h"
> 
> class nsIURI;
> class nsIContent;
> class imgIRequest;
> class nsMenuObjectX;
> 
> #import <Carbon/Carbon.h>
> #import <Cocoa/Cocoa.h>
> 
>+typedef nsresult (nsIDOMRect::*GetRectSideMethod)(nsIDOMCSSPrimitiveValue**);
>+static PRInt32 GetDOMRectSide(nsIDOMRect* aRect, GetRectSideMethod aMethod);

These things should go in nsMenuItemIconX.mm. Users of nsMenuItemIconX don't need them, so they shouldn't be exposed in the header file.

>@@ -79,16 +80,17 @@ PRAllocCGFree(void* aInfo, const void* a
> 
> NS_IMPL_ISUPPORTS2(nsMenuItemIconX, imgIContainerObserver, imgIDecoderObserver)
> 
> nsMenuItemIconX::nsMenuItemIconX(nsMenuObjectX* aMenuItem,
>                                  nsIContent*    aContent,
>                                  NSMenuItem*    aNativeMenuItem)
> : mContent(aContent)
> , mMenuObject(aMenuItem)
>+, mImageRegionRect()

The default constructor is called automatically, so this isn't needed.

>+  
>+  // Declare the of the CSS value pointers so the can be used by both
>+  // the list-style-image value check and the -moz-image-region value check.

This comment probably isn't necessary either.

>+    rv = cssStyleDecl->GetPropertyCSSValue(imageRegion,
>+                                         getter_AddRefs(cssValue));

wrong indent

>+      if (bottom > top && right > left) {
>+        mImageRegionRect.width = right - left;
>+        mImageRegionRect.height = bottom - top;
>+        mImageRegionRect.x = left;
>+        mImageRegionRect.y = top;
>+      } else {
>+      	return NS_ERROR_FAILURE;
>+      }

make this:

      if (top <= bottom || right <= left)
        return NS_ERROR_FAILURE;

      mImageRegionRect.SetRect(left, top, right - left, bottom - top);

And you should probably also check for top and left being != -1, because that's what GetDOMRectSide returns in error conditions.

>+  if (!mImageRegionRect.IsEmpty() && 
>+     (mImageRegionRect.x + mImageRegionRect.width > origWidth ||
>+      mImageRegionRect.y + mImageRegionRect.height > origHeight)) {

use mImageRegionRect.Right() and mImageRegionRect.Bottom()

>+  if (mImageRegionRect.IsEmpty()) {
>+    mImageRegionRect.x      = 0;
>+    mImageRegionRect.y      = 0;
>+    mImageRegionRect.width  = origWidth;
>+    mImageRegionRect.height = origHeight;
>+  }

use SetRect

>+  // We have to clip and reorder data to have alpha last because only Tiger can 
>+  // handle alpha being first. Also the data must always be big endian (silly).

Please also add a comment here about this being the place where you clip the image to the image region.

The changes in nsMenuItemX.mm don't seem to be related to -moz-image-region handling. Please file a new bug for them in order to keep this patch focused.

You're adding some lines that have whitespace at the end, please fix that. The mercurial "color" extension will highlight them; you can install it by adding the line "color =" to the [extensions] block in your .hgrc.

(In reply to comment #22)
> I just changed the
> code to just remove the potential placeholder image and not draw an image on
> all error cases.  While I like the almost cross platform consistency, I am
> concerned that this would break some themes, especially ones that use
> moz-image-region on Windows but don't change their CSS on Mac OS to use
> moz-image-region: auto.

I'm not sure I understand. What your current patch does would only bite them if they rely on that edge case you mentioned, no? If that's the case then I don't think we should bother.

Comment 24

[Ben Frisch]

Attachment: Real Patch to Slice & Dice the Image Icon before Setting It - v.5

This should address all of your comments.  Sorry, as you noticed, the nsMenuItemX.mm changes were a work in progress attempt at fixing another bug.  I guess the removal of the image on invalid image regions would break themes/extensions that worked around the lack of support for moz-image-region on Mac OS by having CSS that they only apply on Mac OS X that just overrides the image specified in another CSS file, rather than overriding both the image and value for moz-image-region specified in the other CSS file.  As I thought about it, I realized that the issue could probably be addressed well with just a sentence in the Firefox 3.7 for extension/theme developers documentation.

Comment 25

[Markus Stange [:mstange]]

Comment on attachment 415038 [details] [diff] [review]
Real Patch to Slice & Dice the Image Icon before Setting It - v.5

>diff --git a/widget/src/cocoa/nsMenuItemIconX.h b/widget/src/cocoa/nsMenuItemIconX.h
>--- a/widget/src/cocoa/nsMenuItemIconX.h
>+++ b/widget/src/cocoa/nsMenuItemIconX.h
>@@ -40,16 +40,17 @@
>  * Retrieves and displays icons in native menu items on Mac OS X.
>  */
> 
> #ifndef nsMenuItemIconX_h_
> #define nsMenuItemIconX_h_
> 
> #include "nsCOMPtr.h"
> #include "imgIDecoderObserver.h"
>+#include "nsIDOMRect.h"

This can go into the .mm file, too.

>+      PRInt32 top = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetTop);
>+      if (top < 0)
>+        return NS_ERROR_FAILURE;
>+      PRInt32 left = GetDOMRectSide(imageRegionRect, &nsIDOMRect::GetLeft);
>+
>+      if (left < 0 || bottom <= top || right <= left)
>+        return NS_ERROR_FAILURE;

Put the top < 0 check into the second if, please. Optimizing for a case that probably won't ever happen isn't worth 2 additional lines of code.

>+  // If the image region is invalid, don't draw the image to almost match
>+  // the behavior of other platforms.
>+  if (!mImageRegionRect.IsEmpty() &&
>+     (mImageRegionRect.XMost() > origWidth ||
>+      mImageRegionRect.YMost() > origHeight)) {

The two latter lines need to be indented 1 space further.

>+  if (mImageRegionRect.IsEmpty()) {
>+  	mImageRegionRect.SetRect(0, 0, origWidth, origHeight);

A tab slipped in.

OK, I think that's all that I can find. Great work!

(In reply to comment #24)
> I guess the removal of the image on invalid image regions would break
> themes/extensions that worked around the lack of support for moz-image-region
> on Mac OS by having CSS that they only apply on Mac OS X that just overrides
> the image specified in another CSS file, rather than overriding both the image
> and value for moz-image-region specified in the other CSS file.

Ah! Yeah, this makes sense, but as you said there's enough time for theme creators to fix their code before 3.7 is released.

Comment 26

[Ben Frisch]

Attachment: Real Patch to Slice & Dice the Image Icon before Setting It - v.6

Comment 27

[Ben Frisch]

Comment on attachment 415153 [details] [diff] [review]
Real Patch to Slice & Dice the Image Icon before Setting It - v.6

This should address all of the previous comments.  As such, I guess it is ready to be reviewed by Josh.

Comment 28

[Josh Aas]

Comment on attachment 415153 [details] [diff] [review]
Real Patch to Slice & Dice the Image Icon before Setting It - v.6

+  rv = dimensionValue->GetFloatValue(nsIDOMCSSPrimitiveValue::CSS_PX,
+&dimension);

Fix indentation here.

I looked over this but I'm more or less rubber stamping it, mstange's review looks solid enough for me.

Thanks for doing this, looks great.

Comment 29

[Ben Frisch]

Attachment: Real Patch to Slice & Dice the Image Icon before Setting It - v.7 [For Checkin]

This just changes the indentation as Josh asked.  As such, I guess the patch is ready for check-in once mozilla-central is open again.

Comment 30

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/84bada10eba4

Thank you, Benjamin!

Comment 31

[Alfred Kayser]

Can this be checkin for 3.6? It is marked as wanted192, and will improve things for themes and extensions.

Comment 32

[Justin Wood (:Callek)]

(In reply to comment #31)
> Can this be checkin for 3.6? It is marked as wanted192, and will improve things
> for themes and extensions.

I am no driver, but even as someone who HIGHLY wants this, I fear that the effect for themes and (to a lesser extent) extension devs for the 3.6 cycle is too risky. It would be almost no notice, and require some attention for the theme authors.

That said, I don't do themes, and would still love to see it if a driver does choose to a+!

Comment 33

[Robert Kaiser]

Callek, I don't see how it impacts anyone, as this doesn't break anything, but just makes the same thing work on Mac that works on other platforms.

Comment 34

[Stian Oksavik]

Does the target milestone of mozilla1.9.3a1 mean this will be fixed in FireFox 3.7? It's still broken in 3.6 final; see attached screenshot "menu.png".

Comment 35

[Stian Oksavik]

Attachment: Sample of icon rendering in themed Firefox menu on OS X

Comment 36

[[e-mail]]

(In reply to comment #34)
> Does the target milestone of mozilla1.9.3a1 mean this will be fixed in FireFox 3.7?
It actually means that it was fixed in time for Firefox 3.7 alpha 1.
> It's still broken in 3.6 final; see attached screenshot "menu.png".
Right, and someone with enough power could request approval for 3.6.x, but obviously that's no guarantee of anything.