Last Comment Bug 355789 - Use vista native uxtheme for menu rendering
: Use vista native uxtheme for menu rendering
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Themes (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal with 6 votes (vote)
: mozilla1.9alpha8
Assigned To: Rob Arnold [:robarnold]
:
Mentors:
: 333486 (view as bug list)
Depends on: 401027 337771 355792 420198 508734
Blocks: 333484 352420
  Show dependency treegraph
 
Reported: 2006-10-06 16:09 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2009-08-05 22:01 PDT (History)
38 users (show)
vladimir: blocking1.9-
reed: wanted1.9+
dveditz: wanted1.8.1.x-
sdwilsh: in‑testsuite-
sdwilsh: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
MENU theme reference PNG (37.46 KB, image/png)
2006-10-06 16:12 PDT, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details
patch for native menu theme (with bugs) (11.37 KB, patch)
2006-10-06 18:06 PDT, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Review
new patch (still with bugs) (31.36 KB, patch)
2007-06-12 13:49 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
Fixed some issues, caused some others (35.89 KB, patch)
2007-06-13 12:08 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
Shows checkboxes/radio menuitems, menu separators and more (39.49 KB, image/png)
2007-06-17 21:08 PDT, Rob Arnold [:robarnold]
no flags Details
Bookmark/history icons in the menu (38.79 KB, image/png)
2007-06-17 21:10 PDT, Rob Arnold [:robarnold]
no flags Details
Menu bar w/mouse hover (14.81 KB, image/png)
2007-06-17 21:11 PDT, Rob Arnold [:robarnold]
no flags Details
Old menu look (27.76 KB, image/png)
2007-06-17 21:14 PDT, Rob Arnold [:robarnold]
no flags Details
Old bookmarks/history look (33.12 KB, image/png)
2007-06-17 21:15 PDT, Rob Arnold [:robarnold]
no flags Details
Old menubar hover (11.20 KB, image/png)
2007-06-17 21:16 PDT, Rob Arnold [:robarnold]
no flags Details
Contains temporary CSS hacks for combo boxes/location bar (35.78 KB, patch)
2007-06-19 15:05 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
Same as previous patch sans tabs (36.30 KB, patch)
2007-06-19 15:42 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
Fixes regressions in combo boxes and location bar (36.19 KB, patch)
2007-06-26 13:37 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
Proposed solution (42.14 KB, patch)
2007-07-18 17:40 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
Fixes menubar height (43.15 KB, patch)
2007-07-24 10:04 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Review
Toolbar comparison (78.33 KB, image/png)
2007-07-24 10:08 PDT, Rob Arnold [:robarnold]
no flags Details
Forgot the throbber changes (44.37 KB, patch)
2007-07-24 10:11 PDT, Rob Arnold [:robarnold]
vladimir: review+
Details | Diff | Review
Fixing small regression (299 bytes, patch)
2007-08-17 13:56 PDT, Rob Arnold [:robarnold]
vladimir: review+
vladimir: approval1.9+
Details | Diff | Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2006-10-06 16:09:06 PDT
With the uxtheme in Vista, the uxtheme new-world theme mechanism now supports menu themeing.  We should use this on Vista to get a native look, because the menu look is now much more complex than a simple color.

(Note: this is only tangentally related to the trainwreck in bug 243078, because this is Vista-only; the MENU theme bits are not present in any earlier version of uxtheme.dll)

(Note#2: I already have a patch for this, I'm just working on fixing some issues with it before attaching it.)
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2006-10-06 16:12:49 PDT
Created attachment 241502 [details]
MENU theme reference PNG

Reference PNG for the menu theme bits.  I used parts 1-6, even though many things only have only 2 or so parts, just to make sure there was nothing missing.  These elements draw the "invalid" parts the same as the first part.  The constants are defined in vsstyle.h in the Vista SDK; the first 6 are various _TMSCHEME constants that seem to draw bogus things.
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2006-10-06 18:06:52 PDT
Created attachment 241511 [details] [diff] [review]
patch for native menu theme (with bugs)

Patch; I think that the nsNativeThemeWin portion of this is pretty solid (note that the "MENU" theme stuff is not available under anything less than Vista, so we'll fall through into the same code paths we've always taken there).

The menu.css stuff needs a ton of work.  The resulting menubar looks too fat, with too much padding up and down.  Harder to fix is the handling of things like popup arrows and checkboxes and stuff -- we should probably just draw these using the native theme code, and I'll try to do that in a new version of the patch, but for now all the "-hover.png" versions of these icons have the lighter color baked in -- so they're a light gray or even white, which usually shows up against dark hover highlights on themes but looks wrong when Vista just draws the outline around the menu item.

We also need a bit more padding, and the menu background doesn't match the rest of Vista; we need more space for icons and we need the slight gutter to be drawn.
Comment 3 Vladimir Vukicevic [:vlad] [:vladv] 2006-10-15 00:59:46 PDT
Note to self:  Office 2007 doesn't follow the Vista menu theme at all, at least OneNote and Visio 2007 don't.  The menus are white text on a dark gray background; the menus themselves on hover get an orange gradient.  On dropdown, the hover look there is a translucent orange background with a solid 1px orange border.  It doesn't even follow the Vista color scheme.
Comment 4 matthaeus123 2006-10-27 13:53:43 PDT
So, should there be a seperate win32 Vista nightly build?
Comment 5 Doug Turner (:dougt) 2006-10-27 14:08:58 PDT
no.  hoping that the same build will work on all flavors of windows 32.
Comment 6 matthaeus123 2006-10-27 14:18:08 PDT
Thanks. I was thinking that it might mess up the program for people on non-Vista systems that are Win32.
Comment 7 Mike Schroepfer 2006-11-08 11:47:00 PST
*** Bug 333486 has been marked as a duplicate of this bug. ***
Comment 8 Daniel Veditz [:dveditz] 2007-01-12 16:01:56 PST
What's the status of this? Need a new patch? Not really blocking?
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2007-05-29 15:52:57 PDT
This is a nice to have, but it's not blocking.
Comment 10 Taral 2007-05-31 13:15:45 PDT
Recommend this block bug 352420 (vista support tracking).
Comment 11 Rob Arnold [:robarnold] 2007-06-12 13:49:37 PDT
Created attachment 268130 [details] [diff] [review]
new patch (still with bugs)

Still have magic numbers in some places (see menuitem padding)
All formulas used in menu widget placement are my guesses and not based on documentation.
Menu separators are too tall (slightly).
Menu highlight text should be black, not white.
Menu icons do not appear in the correct place (this can be fixed like checkboxes/submenu arrows).
Context menus do not have a gutter (see IsTopLevelMenu).

Untested on XP, but works on Vista Aero/Vista Classic.
Comment 12 Rob Arnold [:robarnold] 2007-06-13 12:08:36 PDT
Created attachment 268257 [details] [diff] [review]
Fixed some issues, caused some others

Incompatible CSS changes were made to accommodate the following:

Menu separators are still too tall (can be fixed in CSS, just didn't get to it).
Menu highlight text is now appropriate in all themes.
Menu icons placement is ok now.
Context menus now have gutters (and proper boarders)

Untested on XP. Looks good on Vista Aero, too wide on Vista Classic (probably same with XP classic).
Comment 13 Rob Arnold [:robarnold] 2007-06-13 12:18:00 PDT
Adding beltzner for ui review
Comment 14 Rob Arnold [:robarnold] 2007-06-17 21:08:52 PDT
Created attachment 268737 [details]
Shows checkboxes/radio menuitems, menu separators and more
Comment 15 Rob Arnold [:robarnold] 2007-06-17 21:10:43 PDT
Created attachment 268738 [details]
Bookmark/history icons in the menu

I did not find a reference app for this. Icon placement is not yet ideal (a few pixels off I think)
Comment 16 Rob Arnold [:robarnold] 2007-06-17 21:11:28 PDT
Created attachment 268739 [details]
Menu bar w/mouse hover
Comment 17 Rob Arnold [:robarnold] 2007-06-17 21:14:33 PDT
Created attachment 268740 [details]
Old menu look
Comment 18 Rob Arnold [:robarnold] 2007-06-17 21:15:28 PDT
Created attachment 268741 [details]
Old bookmarks/history look
Comment 19 Rob Arnold [:robarnold] 2007-06-17 21:16:02 PDT
Created attachment 268742 [details]
Old menubar hover
Comment 20 Rob Arnold [:robarnold] 2007-06-19 11:54:19 PDT
This caused a regression on combo boxes and the location bar; I have a temporary fix, but I need to clean it up
Comment 21 Rob Arnold [:robarnold] 2007-06-19 15:05:37 PDT
Created attachment 268986 [details] [diff] [review]
Contains temporary CSS hacks for combo boxes/location bar

All popups which want the menu look should be menupopup elements so that popup elements can be free of native theming for use with other widgets.
Comment 22 Dão Gottwald [:dao] 2007-06-19 15:17:48 PDT
Comment on attachment 268986 [details] [diff] [review]
Contains temporary CSS hacks for combo boxes/location bar

some tabs crept in:

>+popupset > popup
>+{
>+	-moz-appearance: menupopup;

>+	  int width = leftMargin + checkboxSize.cx + rightMargin;
>+    int height = topMargin + checkboxSize.cy + bottomMargin;

>+	  int width = max(itemSize.cx, checkboxSize.cx + gutterSize.cx);
>+    int height = max(itemSize.cy, checkboxSize.cy);

>+    case NS_THEME_CHECKMENUITEM:
>+	   case NS_THEME_RADIOMENUITEM:

>+    if(part == MENU_POPUPITEM)
>+	  {
>+      if(aWidgetType != NS_THEME_MENUITEM)

>+      drawThemeBG(theme, hdc, MENU_POPUPITEM, state, &widgetRect, &clipRect);
>+	  }

>+    SIZE borderSize;
>+	   getThemePartSize(theme, hdc, MENU_POPUPBORDERS, 0, NULL, TS_TRUE, &borderSize);

>+	   RECT sepRect = widgetRect;
>+	   sepRect.left += gutterSize.cx;
>+    //sepRect.bottom += 2;

>+  if(!theme)
>+	   return PR_FALSE;

>+  if(aWidgetType == NS_THEME_MENUITEM && IsTopLevelMenu(aFrame))
>+		  return NS_OK; // Don't worry about it for top level menus

>+	   SIZE gutterSize(GetGutterSize(theme, hdc));
>+    aResult->width = max(aResult->width, gutterSize.cx);
Comment 23 Rob Arnold [:robarnold] 2007-06-19 15:42:28 PDT
Created attachment 268997 [details] [diff] [review]
Same as previous patch sans tabs
Comment 24 Rob Arnold [:robarnold] 2007-06-26 13:37:25 PDT
Created attachment 269898 [details] [diff] [review]
Fixes regressions in combo boxes and location bar

This should be in a state where it can be checked in.
Comment 25 Rob Arnold [:robarnold] 2007-06-27 11:25:43 PDT
Comment on attachment 269898 [details] [diff] [review]
Fixes regressions in combo boxes and location bar

This does not look good on WinXP.
Comment 26 Simon Bünzli 2007-06-28 00:58:11 PDT
Rob: while you're at making this look good on XP, maybe you could include as much as possible from the patch to bug 337771 so that I don't have to start over if you get this in before I do.

And while I'm at it:

+  if(IsThemed())
Missing space between if and ( [if is not a function].

+  margin-left: 30px !important;
This doesn't look RTL safe. Why not use -moz-margin-start?

+.menu-iconic-left {
+  width: 22px;
+  margin-right: 5px;
+}

Can't you set those dimensions from the native theme code?

+menulist > menupopup
+{
+  -moz-appearance: none !important;
+}

This appears in two different files. Is that intended?

+menuitem[type="checkbox"] > hbox > image 
+{
+  -moz-appearance: menucheck;
+}

Wouldn't it be more appropriate to use -moz-appearance: menucheck for menuitem[type="checkbox"] > .menu-iconic-left ? That way you could set the required dimensions from C++ (see above). The same for menuradio...

-  color: HighlightText;
+  color: -moz-menuhovertext;

Please change the background-color from Highlight to -moz-menuhover as well, for consistency.

+popup[type="autocomplete"]
+{
+  -moz-appearance: none !important;
+}

Why not use popupset > popup:not([type="autocomplete"]) instead right above? The less !important the better for themers.

+static PRBool IsTopLevelMenu(nsIFrame *aFrame)
+{
+  PRBool isTopLevel;

Please set isTopLevel explicitly to PR_FALSE or PR_TRUE to indicate what the default/fallback value is.

+  else if(aWidgetType == NS_THEME_MENUITEM || aWidgetType == NS_THEME_CHECKMENUITEM || aWidgetType == NS_THEME_RADIOMENUITEM)

No space after if (happens several times in this method).
Comment 27 Rob Arnold [:robarnold] 2007-07-03 17:57:50 PDT
(In reply to comment #26)
> Rob: while you're at making this look good on XP, maybe you could include as
> much as possible from the patch to bug 337771 so that I don't have to start
> over if you get this in before I do.

I think we have slightly incompatible implementations.

> 
> +.menu-iconic-left {
> +  width: 22px;
> +  margin-right: 5px;
> +}
> 
> Can't you set those dimensions from the native theme code?

Only if they have a -moz-appearance which currently mine don't. I intend to fix this.

> 
> +menulist > menupopup
> +{
> +  -moz-appearance: none !important;
> +}
> 
> This appears in two different files. Is that intended?

Nope, good catch.

> 
> +menuitem[type="checkbox"] > hbox > image 
> +{
> +  -moz-appearance: menucheck;
> +}
> 
> Wouldn't it be more appropriate to use -moz-appearance: menucheck for
> menuitem[type="checkbox"] > .menu-iconic-left ? That way you could set the
> required dimensions from C++ (see above). The same for menuradio...

Here's where we should agree on the semantics of the different moz-appearance styles. Ideally, my patch should not change any CSS except for 'tagging' elements with -moz-appearance styles, so those margin and size adjustments will have to go. I can set them from C++ if they have a moz-appearance, but I do not want to use the menucheck and menuradio appearances there; I need those two appearances on the actual images used so that (and this is a change I made) the image does not draw itself and I can then draw the native vista checkbox/radio images (which are fetched from the theme itself). I use the same trick for the submenu arrows.

I see that your patch removes the images from the checks/radios/submenuarrows so that will eliminate a great deal on my end (which is fine; less code is better).

> 
> -  color: HighlightText;
> +  color: -moz-menuhovertext;
> 
> Please change the background-color from Highlight to -moz-menuhover as well,
> for consistency.

Sure. The actual colors are fine though so I won't change the implementation.

> 
> +popup[type="autocomplete"]
> +{
> +  -moz-appearance: none !important;
> +}
> 
> Why not use popupset > popup:not([type="autocomplete"]) instead right above?
> The less !important the better for themers.

This is my first time doing any real CSS; I like your solution better though.

Comment 28 Simon Bünzli 2007-07-04 12:22:19 PDT
(In reply to comment #27)
> I think we have slightly incompatible implementations.

I've moved mine closer and already included some bits of your last patch. Can you now adapt your patch around mine or do you need anything else?

Note that we seem to prefer menucheckbox instead of menucheck (consistency) and menuarrow instead of submenuarrow for -moz-appearance (see bug 337771 comment #6).
Comment 29 Rob Arnold [:robarnold] 2007-07-18 17:40:43 PDT
Created attachment 272892 [details] [diff] [review]
Proposed solution

CSS changes consist of margin->padding and -moz-appearance tagging
Theme looks great on Vista, XP and classic. Untested on Win2k, but the code should behave like classic. Also fixes combo box and dropdown regressions on vista caused by bug 337771. The vista theme should be pixel perfect, and the changes to xp are close enough that I can't tell. Classic is classic.
Comment 30 Simon Bünzli 2007-07-18 18:16:01 PDT
(In reply to comment #29)
> Also fixes combo box and dropdown regressions on vista caused by bug 337771.

The dropdown regression will be fixed in bug 388317 in an even simpler way; combo boxes should be fixable in the same way (without !important).
Comment 31 Rob Arnold [:robarnold] 2007-07-19 10:51:29 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > Also fixes combo box and dropdown regressions on vista caused by bug 337771.
> 
> The dropdown regression will be fixed in bug 388317 in an even simpler way;
> combo boxes should be fixable in the same way (without !important).
> 

I take it you are going to fix the combo boxes too? It would be nice to have a list of all the known regressions and their bug numbers as some of them are quite significant.
Comment 32 Simon Bünzli 2007-07-19 11:00:26 PDT
(In reply to comment #31)
> I take it you are going to fix the combo boxes too?

Well, now that I know about the issue... For further issues, you should be able to monitor the dependencies of bug 243078. There shouldn't be that many regressions in the first place, though.
Comment 33 Rob Arnold [:robarnold] 2007-07-24 10:04:43 PDT
Created attachment 273605 [details] [diff] [review]
Fixes menubar height
Comment 34 Rob Arnold [:robarnold] 2007-07-24 10:08:45 PDT
Created attachment 273607 [details]
Toolbar comparison

In order to get the menubar height to be right in vista, the height in xp was shrunk as well
Comment 35 Rob Arnold [:robarnold] 2007-07-24 10:11:42 PDT
Created attachment 273608 [details] [diff] [review]
Forgot the throbber changes
Comment 36 Vladimir Vukicevic [:vlad] [:vladv] 2007-07-30 17:31:55 PDT
Comment on attachment 273608 [details] [diff] [review]
Forgot the throbber changes


>+#define MENU_POPUPITEM 14
>+
>+#define MPI_NORMAL 1
>+#define MPI_HOT 2
>+#define MPI_DISABLED 3
>+#define MPI_DISABLEDHOT 4
>+
>+// From tmschema.h in the Vista SDK
>+#define TMT_TEXTCOLOR 3803

For all of these, if we ever include the right include file, this will break... so we need to
either wrap these in #ifndef TMT_TEXTCOLOR #define TMT_TEXTCOLOR 3803 #endif type things, or call
these MOZ_TMT_TEXTCOLOR.  I'd do the former.  (for the MPI_* block it should be ok to just #ifndef MPI_NORMAL for the whole block)

Then again, there's a whole pile of these later on in the file, so maybe we shouldn't bother.

>-    case eColor_highlighttext:
>+    case eColor__moz_menubarhovertext:OSVERSIONINFOEX:

I think the stuff after the : is bogus?

Looks fine otherwise.
Comment 37 Shawn Wilsher :sdwilsh 2007-08-06 10:47:42 PDT
Checking in widget/src/windows/nsLookAndFeel.cpp;
new revision: 1.63; previous revision: 1.62
Checking in widget/src/windows/nsNativeThemeWin.cpp;
new revision: 3.88; previous revision: 3.87
Checking in widget/src/windows/nsNativeThemeWin.h;
new revision: 3.27; previous revision: 3.26
Checking in widget/src/windows/nsWindow.cpp;
new revision: 3.701; previous revision: 3.700
Checking in widget/src/windows/nsWindow.h;
new revision: 3.237; previous revision: 3.236
Checking in layout/style/nsCSSKeywordList.h;
new revision: 3.94; previous revision: 3.93
Checking in layout/style/nsCSSProps.cpp;
new revision: 3.154; previous revision: 3.153
Checking in gfx/public/nsThemeConstants.h;
new revision: 1.21; previous revision: 1.20
Checking in toolkit/themes/winstripe/global/menu.css;
new revision: 1.13; previous revision: 1.12
Checking in toolkit/themes/winstripe/global/popup.css;
new revision: 1.13; previous revision: 1.12
Checking in browser/themes/winstripe/browser/browser.css;
new revision: 1.71; previous revision: 1.70
Comment 38 Neil Deakin 2007-08-06 10:57:43 PDT
+/* auto complete popups don't render well as vista menus */
+popupset > popup:not([type="autocomplete"])
+{

Why does this check for popupset? The popupset element is not a required element.
 
Comment 39 Rob Arnold [:robarnold] 2007-08-06 10:59:08 PDT
(In reply to comment #38)
> +/* auto complete popups don't render well as vista menus */
> +popupset > popup:not([type="autocomplete"])
> +{
> 
> Why does this check for popupset? The popupset element is not a required
> element.

This was fixed before being checked in; I noticed that the context menu in about:config was incorrect.
Comment 40 neil@parkwaycc.co.uk 2007-08-07 07:38:19 PDT
(In reply to comment #27)
>Ideally, my patch should not change any CSS except for 'tagging' elements with
>-moz-appearance styles, so those margin and size adjustments will have to go.
So, in theory, could I write a theme that emulated Windows 2000 in CSS, and then apply -moz-apperance styles, and it would automatically work in Luna/Vista?

>I need those two appearances on the actual images used so that the image does
>not draw itself and I can then draw the native vista checkbox/radio images
I always thought that -moz-appearance was able to suppress CSS painting.

(From update of attachment 273608 [details] [diff] [review])
>+  nsIMenuFrame *menuFrame(nsnull);
Personally I don't like C++-style initialisers for pointers.
Comment 41 Shawn Wilsher :sdwilsh 2007-08-07 20:22:03 PDT
I missed the changes in browser/themes/winstripe/browser/browser.css but the tree is closed right now, so I cannot check them in.  Rob - remind me tomorrow?
Comment 42 Shawn Wilsher :sdwilsh 2007-08-09 15:20:33 PDT
actually, it looks like I did get that file - nevermind me...
Comment 43 Rob Arnold [:robarnold] 2007-08-17 13:56:16 PDT
Created attachment 277136 [details] [diff] [review]
Fixing small regression

I don't know what caused the regression, but this seems to fix it.
Comment 44 Daniel Veditz [:dveditz] 2007-12-19 12:30:01 PST
Schrep marked this wanted for the 1.8 branch, but at this point I'm guessing we wouldn't really want to take it given upcoming FF3 will have the fix and the downside of potential regressions. Not to mention the need to steal resources from FF3 to get this in.

Note You need to log in before you can comment on or make changes to this bug.