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.)

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.

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.

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.

So, should there be a seperate win32 Vista nightly build?

no. hoping that the same build will work on all flavors of windows 32.

Thanks. I was thinking that it might mess up the program for people on non-Vista systems that are Win32.

*** Bug 333486 has been marked as a duplicate of this bug. ***

What's the status of this? Need a new patch? Not really blocking?

This is a nice to have, but it's not blocking.

Recommend this block bug 352420 (vista support tracking).

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.

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).

Adding beltzner for ui review

I did not find a reference app for this. Icon placement is not yet ideal (a few pixels off I think)

This caused a regression on combo boxes and the location bar; I have a temporary fix, but I need to clean it up

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 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);

This should be in a state where it can be checked in.

Comment on attachment 269898 [details] [diff] [review] Fixes regressions in combo boxes and location bar This does not look good on WinXP.

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).

(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.

(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).

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.

(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).

(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.

(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.

In order to get the menubar height to be right in vista, the height in xp was shrunk as well

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.

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

+/* 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.

(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.

(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.

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?

actually, it looks like I did get that file - nevermind me...

I don't know what caused the regression, but this seems to fix it.

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.