355789 - Use vista native uxtheme for menu rendering

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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] (needinfo me, slow to respond)]

Attachment: 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] (needinfo me, slow to respond)]

Attachment: 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] (needinfo me, slow to respond)]

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]

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

Comment 5

[Doug Turner (:dougt)]

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

Comment 6

[matthaeus123]

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

Comment 7

[Mike Schroepfer]

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

Comment 8

[Daniel Veditz [:dveditz]]

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

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 10

[Taral]

Recommend this block bug 352420 (vista support tracking).

Comment 11

[Rob Arnold [:robarnold]]

Attachment: 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]]

Attachment: 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]]

Adding beltzner for ui review

Comment 14

[Rob Arnold [:robarnold]]

Attachment: Shows checkboxes/radio menuitems, menu separators and more

Comment 15

[Rob Arnold [:robarnold]]

Attachment: 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]]

Attachment: Menu bar w/mouse hover

Comment 17

[Rob Arnold [:robarnold]]

Attachment: Old menu look

Comment 18

[Rob Arnold [:robarnold]]

Attachment: Old bookmarks/history look

Comment 19

[Rob Arnold [:robarnold]]

Attachment: Old menubar hover

Comment 20

[Rob Arnold [:robarnold]]

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]]

Attachment: 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]]

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]]

Attachment: Same as previous patch sans tabs

Comment 24

[Rob Arnold [:robarnold]]

Attachment: 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]]

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]

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]]

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

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

Attachment: 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]

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

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

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

Attachment: Fixes menubar height

Comment 34

[Rob Arnold [:robarnold]]

Attachment: 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]]

Attachment: Forgot the throbber changes

Comment 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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]

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]

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

(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

[[e-mail]]

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

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]

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

Comment 43

[Rob Arnold [:robarnold]]

Attachment: Fixing small regression

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

Comment 44

[Daniel Veditz [:dveditz]]

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.