The toolbar border shadow is much too dark on WinXP Luna, and the menus are incorrect (need to be drawn by native theme renderer). These are fairly high visibility glitches.

As a temporary workaround, the modifications from http://texturizer.net/firefox/tips.html#app_xpmenus could be applied by default

if those were applied by default, it would look wrong on WinXP Classic, most third party visual styles, and all older Windows OSes.

(In reply to comment #2) > if those were applied by default, it would look wrong on WinXP Classic, most > third party visual styles, and all older Windows OSes. It doesn't look wrong in third party visual styles - the colours used are those defined by Windows (XP's Advanced Appearance dialogue), so it actually follows third party styles better than the current set-up. Correct, it would look "wrong" in Classic/pre-XP, but at the moment it looks equally "wrong" in Luna. According to Google Zeitgeist, 55% of Windows users are on XP, for whom the *default* setting is Luna. So the default Firefox setting should, in my opinon, match as well as it can. ...or not - it was just a suggestion for until we have proper native rendering.

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

Not a "blocker"

(In reply to comment #3) > It doesn't look wrong in third party visual styles - the colours used are those > defined by Windows (XP's Advanced Appearance dialogue), so it actually follows > third party styles better than the current set-up. Correct, it would look > "wrong" in Classic/pre-XP, but at the moment it looks equally "wrong" in Luna. > > According to Google Zeitgeist, 55% of Windows users are on XP, for whom the > *default* setting is Luna. So the default Firefox setting should, in my opinon, > match as well as it can. > > ...or not - it was just a suggestion for until we have proper native rendering. It does look wrong in third party styles though, very wrong. Well, I guess it depends on the style you're using. This has really bugged me for a long time, I really hope there is someone looking into this.

The group boxes and tree views should also be rendered by the native theme rendered, also the tabboxes still need work.

Nominating to block Firefox 1.5 Beta 2, because fixing this would stop the flamewars over bug 303806.

The menu highlight in 1.5 beta 1 (for example when you hover the mouse over "File") extends to fill height of the toolbar. This creates big ugly blue boxes when the navigation buttons are placed on the same toolbar as the menu, particularly when large icons are used. The problem seems to have been introduced by bug 253661 but according to comment #65 in that bug it's part of this bug. Bug 303806 was supposed to be a temporary fix until this bug was sorted but hasn't managed to fix this particular problem.

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

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

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

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

I'm currently having a go at hitting the native theme code hard enough to fix this. It's not pretty, but I've got -moz-appearance working with menuitems and menupopups with XP themes, as well as fixing a bug with the toolbar and toolbox impls., and now need to sort out the classic renderings (and make the CSS-only appearance match classic too).

i'm very appreciative that someone is finally working on things like this. any chance you could have a go at implementing -moz-appearance: dualbutton on windows, too? i put together a css hack that does it in the relevant bug, but a true fix would be so much better.

It's nice to be appreciated. Seriously, I'll work on the dualbutton if I can, but for now I'm sticking to the menus. Am I right in thinking the dualbutton is the buttons with drop-downs split into two halfs, e.g. the back and forward buttons? I've definately got enough an idea of the Windows theme system to get all the visuals needed, though I've not looked at how the buttons are identified in the nsITheme system. Sneak-peak of CSS-free menu apperances: http://twpol.dyndns.org/temp/appearance_menu_1.png (Luna VX) http://twpol.dyndns.org/temp/appearance_menu_2.png (Luna) No Classic shot yet (not coded that view) and those two aren't bug-free yet. :)

looking good! yeah, dual buttons are the ones with a dropdown like back and forward. if you want a look at the css hack i came up with, it's in bug 216266. hmm, have you tried this with windowblinds? it'd be extra-cool if it picks up windowblinds-styled menus too.

James, how does this look in Classic, by the way? I'm using that and would love it to work right (with black menu background colour, nonetheless).

Shouldn't this be assigned to James Ross now? btw target milestone should change p.s. great work :D

Henrik Pauli, currently it looks like ass in classic, because it is still using my XP theme code. ;) I've almost got the CSS-only (i.e. no theme code in sight) mode looking just like classic, however, and it is only a matter of time before the -moz-apperance version also works in classic. I only started yesterday, and there's a lot to do with 3 different modes to handle, after all. :) Re: window blinds, I will see how it looks, but can't promise anything. If it uses the uxtheme stuff in XP, I think it'll be good though. Ressigning me, clearing target milestone and setting P1.

Ok, Window Blinds is evil. It injects its own DLL into every process, and hooks into the uxtheme API calls, and that's the good news. The bad news is that is also hooks itself into rendering native menus, which obviously doesn't work for us. I expect that is because the uxtheme API wont let you get any menu information except a selection of colours (which is all themes can specify). Problem is that Window Blinds is drawing the menus itself, so the menu colours are reasonable but not going to give the same effects. So Window Binds LAF is not possible, but I'll make sure the real themes work.

Classic: http://twpol.dyndns.org/temp/appearance_menu_3.png Before anyone asks, no I'm not quite done yet. Currently the themed toolbars (previous images 1 and 2) have some small graphical errors (and in fact the classic view has some too, but elsewhere in the Fox), and the code can't yet switch between Classic apperance and Themed apperance by itself.

Certainly looks sweet so far! Hey, one thing that I just remembered. You know the check mark and radio bullet and the submenu arrow, right? On screenshot 1 it's of a different colour than the text -- currently, in the "original" mozilla builds, the unfocused check mark and bullet and arrow seems to be strictly black and the focused ones strictly white... I don't know the source of the browser so I don't know if it's possible to fix this, but it would be awesome if these widgets behaved as the colour of the menu suggests, using the text colour.

Silver, if the checkboxes and submenu arrows are also drawn natively, please make sure not to break RTL UI.

Currently the check, radio and submenu arrows are still just images attached via CSS. I plan to leave them like that for the initial patch, and then fix that separately (though I guess it could just be done now too...). Asaf, thanks for the reminder, though I've had RTL going through my head all day today with the CSS changes, so you need not worry just yet. :)

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

James, could you use native code calling uxtheme to render the menus? This is what Java does to render components on Windows.

"native code calling uxtheme" is exactly what the code is doing (for all the existing theming and the stuff I'm adding) - it uses the OpenThemeData API call (see MSDN) and associated calls. However, the problem is, this call does not provide access to whatever-it-is that uxtheme has for menus (the call fails with "Element not found" for "Menu"). As it happens, I don't think there *is* any theme data for menus apart from a list of about 4 colours - which I've already deciphered and coded it to use. :) Here's a little nugget of information: Windows Media Player 10 custom-draws its Media Library menus (the one listing all the albums etc.), and it doesn't match the LunaVX theme where my code does (it misses the borders of highlighted menu items). Current status: just trying to make the check/radio images work. Odd bug of the day: changing too or from Classic mode does NOT trigger a ThemeChange notification inside the nsITheme interface.

Right! I've got the menu check, radio and submenu images working. http://twpol.dyndns.org/temp/appearance_menu_1b.png http://twpol.dyndns.org/temp/appearance_menu_2b.png http://twpol.dyndns.org/temp/appearance_menu_3b.png And, the all important RTL version: http://twpol.dyndns.org/temp/appearance_menu_4.png Got a problem now with the bookmarks menu, but it's getting there!

James, might want to look through this Windows Vista UX Guidelines (preliminary) http://www.microsoft.com/downloads/details.aspx?FamilyID=fd380553-911e-4659-a085-4dd58ae4b9ae&displaylang=en&Hash=8VLJ9G9 Linking you so that maybe you can do some things now to make sure Firefox will work with vista UI and integrate nicely.

Silver, this is good stuff. If there's any extra work for Vista, we can do that in a followup bug. However, the linked doc is an interface design doc, not an API guide.

This is straight from my tree (though it probably wont apply, as I appended diffs from two different directories: widget and toolkit/themes/winstripe), and as such expect to find excessively long lines, bad indenting, and absolutely no comments! I'm attaching this now a) so I don't lose the thing, b) so anyone who wants can both try it themselves, and c) so people can spot errors in the code. If anyone wants a screenshot of X or Y woth this patch, I'll do it - though I think my 1b, 2b, 3b and 4 images should have covered everything this patch will affect.

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

Wow, this is great, James. I see that you've commented out the codes for linking to the radio and checkmark images. Does this mean we no longer need 'em?

I'm not quite sure what to do with them. The XBL elements they occupy are required to keep the positioning of the text correct, but the images are drawn natively... The actual images are drawn as part of the background to the element so, if the images in CSS are also specified, they go ontop of the native images and mess things up. As far as I can tell, they would only been needed if you had a normal menu item and just turned off -moz-appearance on it - you'd then get the CSS style, which looks just like Classic - which would leave you with gaps for those images.

With this patch, what happens to the menu bar when the toolbar is taller than normal? (e.g if the back/forward buttons have been moved onto the same row as the menu) Currently, when hovering over menu items, the highlight fills the whole height of the toolbar, which looks a bit odd. Other windows apps (e.g IE / Explorer) restrict the height of the highlight. I assume this would be fairly simple to fix with CSS. e.g menubar { max-height: 1.2em } or similar.

If only it was. :( (currently they do go the entire height) I tried some CSS on the menubar, but it inexplicably broke the spring component in Customize (it ended up being 2px high). I don't actually understand why, but since trunk Firefox is totally screwed WRT extensions and their overlays, I can't get to DOMI involved to find out why. It is on my list of "things that aren't quite right" though, along with the wrong text colour for highlighted menubar items in Windows Classic.

On this screen: http://twpol.dyndns.org/temp/appearance_menu_2b.png, shouldn't "View" be in white letters? Otherwise, awesome work, keep it up! This should really get in for 1.5.

It should, yes, as I already said in comment 37 ("...along with the wrong text colour for highlighted menubar items in Windows Classic"). It's not simple to fix, though, and I am currently evaluating various options.

It's too late in the game for this and this is something we've been targeting for the 2.0 release.

If there's a working patch for this between the b2 and rc1, why not? You can always back it out if it happens to bring serious regressions. The current situation, well, sucks (can't phrase it otherwise).

(In reply to comment #40) > It's too late in the game for this and this is something we've been targeting > for the 2.0 release. I talked to Mike Connor yesterday on IRC and he stated that given we have three weeks till the next beta it's possible this could land (that's not verbatim but pretty close) providing this is wrapped up soon. ~B

it was possible, but that wasn't a definitive statement. This certainly isn't a blocker, but that doesn't automatically mean we'll reject a safe and reasonably baked patch. this is a 300+ line patch though, so the risk of regressions is statistically high.

The good thing about this patch is that regressions are totally visible and there's a fair few testers in the community - the anti-Luna brigade ;) - who want to see this land for Firefox 1.5.

This is the same as James'patch, but this one should apply cleanly.

I think you guys are wrong to not consider this important enough to block 1.5, but then why not ask all the users who are getting seriously irrate with the problem itself? (heck, *I* am one such irrate user, I just happen to be irrate enough and knowledgable enough about the codebase to actually fix it) I wont be at my computer until Friday, when I will fix the remaining niggles I have with the impl. hopefully, and you will then have over two weeks to get it through the appalling review process in time for FF 1.5 Beta 2. Please start picking a reviewer and super-reviewer NOW, because I am not interested in fighting just to get someone to review this in time to not disappoint your users.

James, I started a thread on mozillazine to discuss and test your patch(s) and to report any bugs there so as to not spam this bug. http://forums.mozillazine.org/viewtopic.php?t=317569

(In reply to comment #3) > According to Google Zeitgeist, 55% of Windows users are on XP, for whom the > *default* setting is Luna. So the default Firefox setting should, in my opinon, > match as well as it can. The information we don't have is how many of those people are actually using Luna. If 9% or more of Windows XP users are not using Luna, then that means the change to Luna-style menus us making Firefox appear "wrong" on more than 50% of Windows users' machines. Common sense would be that Firefox should at least appear "normal" on the majority of users' machines, but I suspect the move was not motivated by common sense.

note that those stats are as of June 2004, and most stats I've skimmed put XP at 65% or higher and continuing to grow.

but there are no stats on how many people use luna. most people i know who use XP turn it off because it's, well, hideous.

That doesn't really mean anything. Most people I know seem to post screenshots with completely untouched WM settings and I have no idea how they can live like that :) Speculation won't lead us anywhere really. Let's go try to find some statistics... or start a meme on LiveJournal on this topic or something. Should be more effective than this "most of my friends" thing.

Statistics aren't going to get this bug fixed and in either case this isn't the place for them. Please direct such comments to this thread: http://forums.mozillazine.org/viewtopic.php?t=317569

This is the original patch ported to the 1.8 branch, plus lots of improvements (like fixing those damn autocomplete popups). One unfortunate bit was I had to rewrite the bookmark toolbar chevron code because it completely failed to cope with borders on the toolboxes/toolbars. It now also works great in RTL too! The only issues I know of with it are a slight mis-alignment in the bookmarks menus (to be fixed shortly), and I believe it uses the Win2000 menubar styles on Win9x versions too, which I don't know if it is worth fixing (I don't have anything but 2K and XP to test with, unfortunately). I've made a zip of my branch build with this patch in, for anyone that wants to test it: http://silver.warwickcompsoc.co.uk/temp/Firefox_1.5_Theme_Patch.zip

Everything looks great to me! While I don't like the fact that the blue bg is there for hovering over the menus, it is native with my theme, so you seemed to have done one hell of a good job. I don't see any problems on my end. (In reply to comment #46) > I think you guys are wrong to not consider this important enough to block 1.5, > but then why not ask all the users who are getting seriously irrate with the > problem itself? (heck, *I* am one such irrate user, I just happen to be irrate > enough and knowledgable enough about the codebase to actually fix it) > > I wont be at my computer until Friday, when I will fix the remaining niggles I > have with the impl. hopefully, and you will then have over two weeks to get it > through the appalling review process in time for FF 1.5 Beta 2. > > Please start picking a reviewer and super-reviewer NOW, because I am not > interested in fighting just to get someone to review this in time to not > disappoint your users. As for reviewer and superreviewer, I say mano should review, and mconnor should superreview, with of course ben approving.

spam: Of couse, it does not matter that i'm not peering win32 widget/gfx or that mconnor isn't a sr. If Tongleback says, we do. enough spam from me, go ahead and stop adding useless comments too.

I'm thinking a toolkit peer (e.g. mconnor) for review, and a gfx/widget peer for superreview (e.g. neil or roc) would be about right. Of course, anyone else is free to look over it and find problems. :)

Wow, this just looks great! I have tested this with Luna and a custom theme (Watercolor) and it seems to work very good in both themes. I have found a little glitch, though. I use the Cutemenus extension and menu items without an icon appear a little too much to the left. A good example can be seen in the Edit menu with the two last items: http://www.tweakers.net/ext/f/66533/full.png. I don't know if this is something that has to be fixed by the extension or by you, but considering the fact you are working on this, I thought at least you had to know it.

The problem is that the icons used are larger than the space allocated to the menu item images by Windows (13 or 14px). The bookmarks menu has this issue too, but has CSS to make the items without images line up, and I've just corrected that CSS so they do line up with my patch. The question is, should all menu items allocate space for a 16px-wide image? In fact, if you actually look closely at the CuteMenus effect on the File menu, the first two items don't even line up with each other, as the new tab image is 17x17, and the rest are 16x16 (I think import is actually bigger too).

This is basically it. The final patch. Probably. :) http://silver.warwickcompsoc.co.uk/temp/Firefox_1.5_Theme_Patch.zip has been updated with a new build.

The little arrow to show that there's a submenu is not shown (Win98 with a clean profile).

Nice improvement on the menus and context menu on Classic! I also like the corrected vertical spacing of the menu items. I found a few issues that this patch doesn't yet address: - In Windows XP Luna (or Media Center style), the context menu still has no 3D shadow. - The toolbar border shadow is still too dark. - When clicking menulists (e.g. Options->Content->Default Font), the the menu still doesn't slide down, but opens instantly.

Steffen, none of those are even close to related to this bug, except the toolbar border, which is the same colour as it was before, and can be fixed separately. Interesting thing with Win98... wonder what is going on there.

Actually, the toolbar border is *better* than it was - it looks perfect on one theme I have here, it is just not quite matching in the other theme. I actually think IE hard-codes the colours, anyway, so we're looking better than IE in that sense.

This update just corrects the padding of items in the menubar, and removes the use of DFCS_TRANSPARENT, which was unnessessary (and might be the problem with Win98). James C, could you please try out the updated build at http://silver.warwickcompsoc.co.uk/temp/Firefox_1.5_Theme_Patch.zip and see if the submenu arrows work on Win98 or not. Thanks.

The latest build didn't fix the submenu arrows on Win98.

Just a note on Royale, The Menu BAckrounds use the Same Colour as the Toolbar backround (grey), instead of White

Just wanted to write about disabled submenu hovering in win2k. see part "1" of the screenshot. The arrow is white, while it should remain dead. But I noticed new build and tried it before posting. The result is shown in part "2" of the schot.

S. Jewers, I think you've confused the theme engine there [1]. In particular, the fact the background of the text on the menubar items is not the same as the menubar colour - I've see that here, and you need to kick the theme engine to fix it (usually switching theme to Classic and back does it, reboot as a final resort). [1] It looks like the theme says to use flat menus, but the theme engine has got confused as is not doing the flat menu rendering.

Tim, part 2 is not a disabled menu item. I've fixed the colour change on hover here now. I would really like some help with working out why the arrows don't appear on Win98... do checkmarks/radiomarks appear ok?

Just to confirm: I get the same behavior as Shayne Jewers on a cleanly restarted WinXP using the Silver theme (no theme hacking involved) with the most recent version of the patch and a clean profile.

What are you confirming? You're using a different theme - and one that is known to work fine!

My bad. Looks like I've managed to hose up my visual configuration. It works fine with an untouched Silver theme, but not under the one I've fiddled around with (IIRC I've done so only through the official display properties). Notwithstanding all other programs (including vanilla Firefox) display properly.

I just wanted to confirm that this patch works fine with Royale (Media Center style).

The arrows are shown on Win98 with the latest build, as are checkmarks and radiomarks. The only problem I'm having now is that I have my personal bookmarks menu items in the menubar to save space and they are much further to the right in the build with this patch than in the latest nightly. Also, this makes the menubar a couple of pixels taller than by default and the menu items are not vertically centred. Little things and I'm not sure if they're both caused by this patch but I thought it was worth pointing them out.

The bootmarks toolbar change is a result of me fixing the code that decides what to display. In fixing that, it now behaves just like a normal spring element, and will always be allocated the same space as other springs on the same toolbar, irregardless of the items on it. It also means it works a lot better in RTL mode. As for the vertical alignment - that matches IE here in my latest build. If that is not quite vertically centered according to you, so be it. We've got to come to some compromises - the menus are pixel-aligned currently. Going to attach new patch now Win98 problem is fixed.

This is hopefully the last update for now; it fixes the images not displaying on Windows 98 (and probably 95/ME), and also makes the hover effects apply to disabled items correctly. Assuming no serious issues are found with this, I'd like to fix any remaining problems (e.g. colours of the toolbar separators, if they need fixing) in separate much smaller patches.

(In reply to comment #69) > [1] It looks like the theme says to use flat menus, but the theme engine has got > confused as is not doing the flat menu rendering. Actually, Royale doesen't use flat menus :S i'll try switching to classic and back, to see if it fixes

Selecting a search engine adds a checkmark to the left of the engine name which is visible when selecting an engine with an icon with a transparent background (e.g. Yahoo). Sidebar header should have top/bottom borders.

The search engines thing is a bug with their menu, really - I can't do anything about that. Also, the sidebar header looks better without borders in Classic (thanks to the bad decisions on the apperance of the content area, and the sidebar itself). If the 'theme people' want to come and mess with that stuff later, they can. IF you want me to fix it, tough, I'm not going to until this patch has landed and I can actually do things without making another huge patch.

Fair enough, just thought they were worth pointing out. BTW - was the problem with Cutemenus causing menuitems to be misaligned fixed? I thought an earlier version of this patch fixed it but it seems to be happening again...

The issue is with the extension, basically. It is using images bigger than the size available, which pushes the text across. The alternative, squashing the images, would ruin the favicons in bookmarks (they're bigger too). As I think I said at the time, unless there is a policy decision to make the image area of menus always big enough for 16x16, it will be up to the icon to be small enough (13x13).

(In reply to comment #69) > you need to kick the theme engine to > fix it (usually switching theme to Classic and back does it, Tried that, didn't work :S btw if you wanna test it on royale, you can download it at http://tinyurl.com/97qpj

Looks fine to me: http://twpol.dyndns.org/temp/ff_theme_royale.png It seems that fully-native menus (i.e. those found on apps that don't use ReBars, e.g. Paint: http://twpol.dyndns.org/temp/paint_theme_royale.png ), there is a rendering problem with the Windows Theme Engine - it really should not be drawing the text with a grey background onto the almost-white background, and then drawing it all grey on click, that's just broken, and IE + my patch render it much better.

yours looks diffrent than mine o_O http://img79.imageshack.us/img79/1117/untitled0pw.png must be something on my end....... (sorry for the bugspam btw)

(In reply to comment #85) > must be something on my end....... Royale is a Tablet PC skin. It acts strangely sometimes on XP, like distorting the icons on the taskbar. This can happen if you don't like the normal window title height and fiddle around with this setting. I have also seen this phenomena from the screen shot, even on normal windows menus, ie not in Firefox but in bundled apps. This was very long ago, hence not necessarily connected to this patch.

I just tried the Royale theme again, and it did the same as attachment 196477 [details] this time - it's definately not playing the same game as us. I think it is probably best not to worry about exactly what it is up to until the main patch here is checked-in, or we may be here for weeks. :)

keep in mind there are several (slightly different) versions of the royale theme floating around. they might behave differently.

(In reply to comment #85) > yours looks diffrent than mine o_O > http://img79.imageshack.us/img79/1117/untitled0pw.png > > must be something on my end....... > > (sorry for the bugspam btw) Ok this is to all the guys who are reporting problems with the royale theme. This "bug " is nothing to do with James Ross patch, so James dont worry :) Anyway, to get rid of the rendering bug, go to appearance tab and choose Royale and apply. The bug will be gone, I posted here as I did not want James going off checking his patch when it nothing that he has done wrong. Hope that helps.

Comment on attachment 196519 [details] [diff] [review] Final version of menubar, menuitem, toolbar patch My build is hosed, will take a little while before I can get to this. I'll review the c++ part of the patch, but you should find someone else to do the theme xul, css etc.

Ere, I'll review the xul/css changes, not a problem there, its the widget code that we really need your review on.

Comment on attachment 196519 [details] [diff] [review] Final version of menubar, menuitem, toolbar patch Is this a branch patch or something? >RCS file: /cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.cpp,v >RCS file: /cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.h,v

It has to be, or it stands *zero* chance of landing on the branch. As it happens, it should apply to trunk fine after correcting the paths to those two files, and one adjustment for an extra header.

Neil, see previous comment. Also, to make things clear, this patch is simply to clean up the mess made of the Firefox 1.5 Beta 1 release in time for Beta 2. Once that has actually happened, I'll move forward with a full trunk fix, which will include a few extra things that I can't fit in time-wise on the branch.

Silver, please ask someone else for review if you're in hurry. I can't review (or do) anything at the moment due to ongoing problems with my network connection.

*sigh* mconnor, suggestions?

Comment on attachment 196519 [details] [diff] [review] Final version of menubar, menuitem, toolbar patch I only reviewed the the c++ files. Comments for attachment 196519 [details] [diff] [review] Please move these: +#define DFCS_RTL 0x00010000 // OURS, not Microsoft's! +#define DFCS_CONTAINER 0x00020000 // OURS too! to below the the ifndef then define's statements. You may also want to redefine all of these constants with the form: #ifdef VALUE #undef VALUE #endif #define VALUE This ensure that the constant's value is consistent regarless of the SDK. You do not need to use :: when calling: + rv = ::SystemParametersInfo(SPI_GETFLATMENU, 0, &isFlatMenus, 0); I never liked comments that are sort of cryptic: + case NS_THEME_TOOLBAR: { + // Don't draw this as background, use special code. Can you expand on what you are doing so that the casual reader can understand without thinking too hard? Are you always guarteed that the |content| will be non-null if aFrame is non-null?? This pattern exist elsewhere in the patch. + if (aFrame) { + nsIContent* content = aFrame->GetContent(); + nsIContent* parent = content->GetParent(); Please rework the NS_THEME_TOOLBAR code block so that aState is only touched once (use an else) Could you comment on why you care about this specific state? I think i understand, just add some comments. + if ((part >= 0) && (state >= 0)) Assign nsnull to menuFrame when declaring. This pattern exist in other parts of the patch. + nsIMenuFrame *menuFrame; + CallQueryInterface(aFrame, &menuFrame); + + if (menuFrame) { It is a good idea to comment why these values were used: + (*aResult).left = 3; + (*aResult).right = 4; If menuFrame is null, you will crash: + if (menuFrame) { + nsIMenuParent *menuParent = menuFrame->GetMenuParent(); + if (menuParent) + menuParent->IsMenuBar(isTopLevel); + } + + if (isTopLevel) { ... + } else { + PRBool isContainer; + menuFrame-> In DrawMenuImage(), can or should these be cached: + int checkW = ::GetSystemMetrics(SM_CXMENUCHECK); + int checkH = ::GetSystemMetrics(SM_CYMENUCHECK); And if so, should we stash hMonoBitmap for reuse? Changes to layout/style/ should be run by dbaron or jst.

(In reply to comment #97) > You do not need to use :: when calling: > + rv = ::SystemParametersInfo(SPI_GETFLATMENU, 0, &isFlatMenus, 0); I know I don't, but it is the style used in this file so I'm sticking to it. > I never liked comments that are sort of cryptic: > + case NS_THEME_TOOLBAR: { > + // Don't draw this as background, use special code. > > Can you expand on what you are doing so that the casual reader can understand > without thinking too hard? I can try... > Are you always guarteed that the |content| will be non-null if aFrame is > non-null?? This pattern exist elsewhere in the patch. As far as I can make out from http://wiki.mozilla.org/Gecko:Key_Gecko_Structures_And_Invariants, yes. > Please rework the NS_THEME_TOOLBAR code block so that aState is only touched > once (use an else) That would needlessly complicate that bit of code. > Could you comment on why you care about this specific state? I think i > understand, just add some comments. > + if ((part >= 0) && (state >= 0)) I will add comments. > In DrawMenuImage(), can or should these be cached: > > + int checkW = ::GetSystemMetrics(SM_CXMENUCHECK); > + int checkH = ::GetSystemMetrics(SM_CYMENUCHECK); > > And if so, should we stash hMonoBitmap for reuse? They could be cached, but I'd have to go and fix the bug that means the theme code doesn't get told about metric changes, only theme changes (and even then, not always). I'm not willing to try and fix that with this patch.

I've already updated my patch based on the comments, should I attach now or wait for more comments?

ofcourse attach it

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] This patch look okay. I am somewhat concerned about using values that are obtained from visual inspection. What if they change with some service pack? Given this isn't a problem, r=me. you should get an addl. review by mconnor et. al on the xul/css changes.

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] Requesting additional review on behalf of silver, for XUL/CSS portion.

affects thunderbird too?

I've only just had time to give this patch a whirl, and then it was on suite for some reason (!), so I had to guess at some of the menu.css changes. I would really appreciate it if the menu images could be drawn opaquely, so that xul|* { -moz-appearance: none; } didn't break anything.

What would it 'fix' by drawing them opaquely? Surely that's not even realistically possible, given the dynamic backgrounds?

Sorry, but I only have the default XP themes to play with... do third-party themes allow you to have custom popup backgrounds too?

No, they are limited to solid colours. What doesn't make sense from your comments is what drawing them opaquely would affect - it would simply waste time drawing the background colour on the background.

Transparancy requires two BitBlts but opacity only requires one. But the reason I was interested in drawing the menu parts opaquely is that it makes themes more portable to OSes that don't have native theme rendering, as they can just specify a fallback image. I believe ordinary checkboxes and radio buttons work like that, although that may require an individual appearance for each part.

Fallback images only work as backgrounds, since that is all the nsITheme stuff can override. That is why the patch removes the image setting CSS - those images end up ontop of *everything* the native code draws. A solution - and this is something I have through about, but specifically not tried to do on the branch - would be to add some extra theme "parts" to nsITheme, for the various images. Then set -moz-appearance: menuitemcheckmark; or whatever on the XBL's element, as well as setting the fallback image using background-image.

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] sr=me for branch but see below for nits. >Index: browser/base/content/browser.xul >Index: browser/components/bookmarks/content/bookmarksMenu.js Maybe you mentioned it in a comment that I've overlooked, but this looks like a different patch. >+#navigator-toolbox { >+ border-bottom-width: 1px; >+ -moz-border-bottom-colors: ThreeDShadow; >+} Out of interest what's special about this toolbox? >+ mMenuActiveAtom = do_GetAtom("_moz-menuactive"); Ideally this belongs in nsNativeTheme.cpp where it can be shared with GTK2 and future implementations. >+ HRESULT rv; Nit: BOOL SystemParametersInfo() >+ // This will simply fail on Windows versions prior to XP, so we get >+ // non-flat as desired. [Trunk note: FYI NT3.51 desires flat menus, while Windows 95 desires flat menubars. So it would seem that you can't write a Windows 95 theme for Windows XP.] >+ // state == 1 iff this toolbar is the first inside the toolbox, which >+ // means we should omit the top border for correct rendering. Would a toolbox top margin of -1 to push the top border under the non client area be even hackier? It would seem to simplify the code in some places... >+ if (isTopLevel) { >+ (*aResult).top = (*aResult).bottom = 1; >+ (*aResult).left = 3; >+ (*aResult).right = 4; >+ } else { >+ (*aResult).top = 0; >+ (*aResult).bottom = (*aResult).left = (*aResult).right = 1; >+ } [Trunk-only note: given separate menuitemcheck and menuitemradio appearance parts it may simplify the code to create separate menuitem and menubaritem appearances. It would also help to fix Windows 95 ;-) ] >+ // XXXjgr We should ideally be caching these, but we wont be notified when >+ // they change currently, so we can't do so easily. Same for the bitmap. >+ int checkW = ::GetSystemMetrics(SM_CXMENUCHECK); >+ int checkH = ::GetSystemMetrics(SM_CYMENUCHECK); Do this in UpdateConfig() perhaps? >+ // XXXjgr This will go pear-shaped if the image is bigger than the >+ // provided rect. What should we do? [Trunk note: all the more reason to implement this as a separate part!] >+ COLORREF oldTextCol = ::SetTextColor(hdc, 0x00000000); >+ COLORREF oldBackCol = ::SetBkColor(hdc, 0x00FFFFFF); >+ ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0, SRCAND); >+ ::SetTextColor(hdc, ::GetSysColor(aColor)); >+ ::SetBkColor(hdc, 0x00000000); >+ ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0, SRCPAINT); Nit: Seems to me that if you knew the solid background colour you could one SRCCOPY. >+ ::SetTextColor(hdc, oldTextCol); >+ ::SetBkColor(hdc, oldBackCol); Nit: I don't remember seeing it documented that you had to reset the colours before deleting a DC. > menu, > menuitem { >+ -moz-appearance: menuitem; > -moz-box-align: center; >- border: 1px solid transparent; So, normal menuitems never have a border, right? >+ min-width: 1px; Any particular reason? [(menu)popup]

(In reply to comment #111) > (From update of attachment 197534 [details] [diff] [review] [edit]) > sr=me for branch but see below for nits. > > >Index: browser/base/content/browser.xul > >Index: browser/components/bookmarks/content/bookmarksMenu.js > Maybe you mentioned it in a comment that I've overlooked, but this looks like a > different patch. The changes here are absolutely *required* to make the bookmarks toolbar not have a total fit trying to handle a border on the toolbar/toolbox. Without this, it behaves even worse than usual. > >+#navigator-toolbox { > >+ border-bottom-width: 1px; > >+ -moz-border-bottom-colors: ThreeDShadow; > >+} > Out of interest what's special about this toolbox? This is a hack to make the content area running up against the toolbox not look crap, which is entirely the result of the decision to not have a sunken border on the content area. > >+ // state == 1 iff this toolbar is the first inside the toolbox, which > >+ // means we should omit the top border for correct rendering. > Would a toolbox top margin of -1 to push the top border under the non client > area be even hackier? It would seem to simplify the code in some places... I'm not sure which is worse, to be honest - I don't like the code to alter the rendering as it is, but a -1 margin doesn't sound any better. > >+ // XXXjgr We should ideally be caching these, but we wont be notified when > >+ // they change currently, so we can't do so easily. Same for the bitmap. > >+ int checkW = ::GetSystemMetrics(SM_CXMENUCHECK); > >+ int checkH = ::GetSystemMetrics(SM_CYMENUCHECK); > Do this in UpdateConfig() perhaps? I believe I already commented on this, but I left that out for now as the UpdateConfig call is not made in all cases it should be, and I didn't wish to try and fix that on branch. On trunk, I can work in a fix for that and actually cache a few other things too. > >+ COLORREF oldTextCol = ::SetTextColor(hdc, 0x00000000); > >+ COLORREF oldBackCol = ::SetBkColor(hdc, 0x00FFFFFF); > >+ ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0, SRCAND); > >+ ::SetTextColor(hdc, ::GetSysColor(aColor)); > >+ ::SetBkColor(hdc, 0x00000000); > >+ ::BitBlt(hdc, imgPos.x, imgPos.y, checkW, checkH, hMemoryDC, 0, 0, SRCPAINT); > Nit: Seems to me that if you knew the solid background colour you could one > SRCCOPY. I can change it if you like, but I'm not sure I want to change any logic at this stage. > >+ ::SetTextColor(hdc, oldTextCol); > >+ ::SetBkColor(hdc, oldBackCol); > Nit: I don't remember seeing it documented that you had to reset the colours > before deleting a DC. I don't know if you are supposed to, but the code I've seen for this kind of drawing does it. I can remove them if you want. > > menu, > > menuitem { > >+ -moz-appearance: menuitem; > > -moz-box-align: center; > >- border: 1px solid transparent; > So, normal menuitems never have a border, right? Not in the CSS - all the borders and stuff is native because, well, it depends on things CSS simply doesn't know a thing about. :) > >+ min-width: 1px; > Any particular reason? [(menu)popup] That's in the original if you read up a few lines. ;)

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] Requesting approval on behalf of silver. It appears the css was reviewed by neil, so it seems additional review is no longer needed by mconnor. I don't see why this shouldn't go in after b2 is released (after all, there's already been heavy and successful testing done with this patch), and the sooner this patch is landed, the more time there is to back it out in the unlikely event serious regressions arise.

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] r=me, subject to appropriate updates to the patch. Clearing approval request until this gets trunk landing and baking. This is what we are doing for anything that isn't a blocker, and isn't low to zero risk. Landing on branch for testing and backing out is not acceptable at this stage of the cycle.

Oh for Pete's sake guys. The patch is for branch only. *Now* you're suddenly saying a banch-only patch must land on trunk first! WTF?

James, the patches would have to be landed on the trunk sooner or later anyways.

No, they wont. Did you not read the bug? The current patch is nothing more than a band-aid to make Firefox 1.5 not suck. See comment 94.

(In reply to comment #117) > No, they wont. Did you not read the bug? The current patch is nothing more than > a band-aid to make Firefox 1.5 not suck. See comment 94. Ya, I read it...just forgot eveything that has been said in this bug. James has a point, no one said anything a few weeks ago when he stated this is for branch only and stand no chance of landing on the trunk becuase of differerences. His builds with the patch included was tested by many people and there are no bugs left that any of us could find and we tested this with a fine toothed comb with all different windows OSes, a lot of themes and fonts/font sizes. Please land this and if anything nasty is found back it out, not talking about backing it out a few weeks from now, just give it a day or two.

Though I, too, believe that this should really be strictly a branch patch, if the "bandaid" requires testing on trunk, this should suffice. I've tested it at least a bit, and it both compiles and accurately emulates IE for Classic and Luna. Not sure about the other esoteric themes that people can test, but that's why there are trunk testers, no? Since the code is essentially the same (+ unbitrotting), methinks the flags should carry over (correct me if I'm wrong and this requires another two-week review). Since the code on trunk changes rapidly, I recommend that this be checked in fairly soon. That will also give a bigger window for finding regressions. Silver, if you disagree with this patch being checked into trunk at all, even if it's going to be checked out later, feel free to cancel the request for review, etc. But if this is the only way it's going to have a chance to get on branch, I think this is worth it.

I strongly object to a few things that drivers have done here: a) Waiting until we'd spent 2.5 weeks getting r/sr on a branch patch before adding random conditions, like "land on trunk and we'll think about branch". b) Requiring landing a branch-specific band-aid patch on trunk at all. If drivers really are now requiring a trunk landing before branch, then you are welcome to get it landed. You do not have my backing in the trunk landing, but I wont object to it actually happening. I have way too much crap going on right now to have to worry about drivers pissing me about, so I want a straight answer right now that WONT CHANGE TOMORROW - do you (drivers) require that this patch be landed on trunk before being granted approval for branch? The only valid answers are "yes" and "no".

Silver, it's already landed (by timeless) and I'm watching the tree as we speak. Again, I agree with your sentiment, but if this is what's required for it to get on branch I'm willing to do it. Again, after the whole 1.5 mess is done a real fix can happen.

Who gave sr for that to land [1]? Why didn't they or you say in this bug? Did people just throw out common sense on this bug or what? [1] Yes I can read Bonsai. I can also read Bugzilla flags. Inconsitency is bad.

That SR request was somewhat whimsical. Neil was too tired to check it in, and (as I had compiled it and it is virtually identical to your patch, save the locations of the nsNativeThemeWin.* files and some bitrot) it had already largely gone through the r+sr process. If the tree burns, I'll (try) to fix it. I apologize for any confusion.

After testing a build that has the patch built in it seems that the colour of the menu text on hover stays black instead of turning into white.

(In reply to comment #124) > After testing a build that has the patch built in it seems that the colour of > the menu text on hover stays black instead of turning into white. You're using Luna a presume? Looking great on my Classic theme (winXP), good work guys!

(In reply to comment #125) > You're using Luna a presume? > Looking great on my Classic theme (winXP), good work guys! Using a cusotm theme.

(In reply to comment #126) > Using a cusotm theme. It would be helpful to know which theme, and a download location if possible.

(In reply to comment #127) > (In reply to comment #126) > > Using a cusotm theme. > > It would be helpful to know which theme, and a download location if possible. It doesn't matter, because I've just tried with Luna and I get the same problem.

Caleb, I would have hoped you would compare with the native rendering... in Luna at least, the menu text hover colour IS THE SAME as the non-hover colour. You can configure it to be different in Apperances > Advanced, if you like, and that works fine here (on my 1.5 branch build - no update available for trunk yet).

(I am assuming the trunk port of the patch was correct - I really don't have the time to check)

(In reply to comment #129) > Caleb, I would have hoped you would compare with the native rendering... in Luna > at least, the menu text hover colour IS THE SAME as the non-hover colour. You > can configure it to be different in Apperances > Advanced, if you like, and that > works fine here (on my 1.5 branch build - no update available for trunk yet). Yeah I was vague. It seems that even in the custom theme I'm using (Royale Glass as part of the crappy Longhorn Transformation Pack which I don't recommend installing) shows the right behaviour in IE6 and the wrong one in Firefox. Hovering over menu items in IE changes the text colour to white while in Firefox it stays black. I dug up that build you posted in the MozillaZine forums and it shows the same behaviour. In Appearance > Advanced the colour for "Selected Items" is white.

The text colour is entirely controlled by CSS anyway, and I only changed the bit for menu items directly on a menu bar. Is it those top-level items or the ones in popups? (screenshot!)

(In reply to comment #132) > The text colour is entirely controlled by CSS anyway, and I only changed the bit > for menu items directly on a menu bar. Is it those top-level items or the ones > in popups? (screenshot!) Top-level, bottom-level, ANY-level: http://img13.imageshack.us/img13/4990/screenshot4yd.jpg This font colour of the "Selected Items" property in the Appearance options seems to control the font colour of these hovered items.

(In reply to comment #133) > This font colour of the "Selected Items" property in the Appearance options > seems to control the font colour of these hovered items. Yes, that's correct. It all works perfectly over here (1.5 branch), and more to the point, for the non-top-level items, was not changed by this patch (on branch) anyway. It is possible something's changed on trunk that conflicts with it, which is just another reason I stuck to branch for the patches.

It works ok here on an almost new profile (only NTT to see the build id)

I'm guessing it's a trunk issue since I'm seeing the same thing as Caleb. Other than that, things are looking great! Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051006 Firefox/1.6a1

It must be a very clever trunk issue, as it doesn't happen on the tinderbox build I got. :P

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051007 Firefox/1.6a1 {Build ID: 2005100704} Looks wonderful! Great work! But, this can not use on Windows 9X. The GDI resource is consumed at once when displaying the part of the menu and the toolbar increases and the reaction of UI dulls. 75% GDI resource that was when Windows starts becomes 10% in less than no time. However, when this shuts the browser window, the GDI resource is released.

I know Windows 9x is dodgy at the best of times, but I'm not quite sure how it can persistently consume resources like that, when it destroys the bitmap, objects, etc. all as soon as it is done drawing. Does the resource usage level vary when you open or close menus or submenus? Does it go down at all, for that matter (except closing the window)?

The GDI resource is consumed in addition and never liberated until the browser window is shut. When the bookmark of the menubar that is sets of a lot of menu items is displayed many times, the consumption of the GDI resource is reproduced.

Ok, the bookmarks toolbar and menu are different... is the usage actually worse than previously when you have the bookmarks toolbar hidden and only use normal menus? Is it then made a lot worse by opening either bookmarks things? Some proper resource usage numbers in various configurations would be good to judge what it actually going on... if someone has the time and patience to try all these configurations both before and after the patch, keeping the same Firefox settings throughout: - Bookmarks toolbar off at startup, don't open any menus except to quit after recording the usage. - Same, but flick through all the menus *except* the bookmarks one, then record usage. - Same, but also flick through the bookmarks menu before recording usage. - This time start with the bookmarks toolbar on, and record usage without using any menus. - Same, but flick through all the menus *except* the bookmarks one, then record usage. - Same, but flick through all the menus before recording usage. It would be advisable to disable or uninstall all extensions before gathering any data, too, as they may adversly affect the resource usage. That's 12 different samples in total, and more than that in restarts of Firefox, so don't feel you have to do it if you don't - I'll be linking this comment on MozillaZine and asking for any help they can provide in gathering this data.

The bookmark of the menubar is one remarkable mere example. The bookmark toolbar is not related directly. The basis is a repetition of the opening of the menu(context menu) and shutting it.

+ // for this item. We will pass any nessessary information via aState, that should be "necessary"...

http://tonglebeak.com/1008_branch_theme_aboutDialog_patches.zip There's the 10/08 branch I built with silver's branch patch applied. In the mozillazine thread, this build according to users who were affected on the trunk, has no problems with GDI resource, so it's something just trunk related, meaning the GDI problem shouldn't stop this patch from 1.5

If this is now in the trunk, I have one question: why do context menus have no drop-shadows now that they are natively drawn (no, it's not bug 281709)?

Actually, it is precicely bug 281709. The menu itself does not include the drop-shadow inside the window, it mearly tells Windows to apply one when it appears - which is what bug 281709 is about. This bug, and native rendering in general, is only about the contents of windows, not how they externally interact with the OS.

Mr. Aaron, Yes, that bild works fine on Windows 98SE. But, the problem on trunk caused with this patch is unsettled. The possibility that the problem occurs in potential also on branch is had. The user of Windows 9x doesn't hope for the reproduction of the resource bug on Mozilla Suite 1.4 (Bug 204374). Napoleon said, 'I am hurrying up very much. Therefore, please report slowly.'

I filed bug 311798. Maybe, this one is regression of this.

(In reply to comment #120) > I strongly object to a few things that drivers have done here: > > a) Waiting until we'd spent 2.5 weeks getting r/sr on a branch patch before > adding random conditions, like "land on trunk and we'll think about branch". > b) Requiring landing a branch-specific band-aid patch on trunk at all. > I have way too much crap going on right now to have to worry about drivers > pissing me about, so I want a straight answer right now that WONT CHANGE > TOMORROW - do you (drivers) require that this patch be landed on trunk before > being granted approval for branch? The only valid answers are "yes" and "no". Then "Yes" :). For large scale changes such as this one, we have always required patches like this to land on the trunk. The patch must then bake on the trunk so any regressions can be addressed there before we will consider it for the branch. This has been a long standing policy when we reach this point of the release cycle that we've employed for many many years. It's not something new nor should it be a surprise. The trunk is used to test risky changes, we don't want to be doing our regression fixes directly on the branch this close to final release.

The problem is, it *was* a surprise. Both how long it took for anyone to do anything, which meant it missed Beta 2 when it didn't need to miss it, and secondly because I stated very clearly in this bug that I was doing a branch-only patch and /not a single person objected or even hinted that it might need to go onto trunk/. While you may not have changed you (undocumented, as ever) policy, you still took me by surprise, and as such you now have a patch I wrote checked-in to trunk that is not going to get any support or fixing from me.

This is basically just an updated version on attachment 197534 [details] [diff] [review], but with a fix for bug 311798 in (so will not regress that on branch). Whether or not the patch in bug 311798 gets review, this is here so I don't lose it, and other people can see just how evil it is. Should the patch in bug 311798 get review and branch approval, and attachment 197534 [details] [diff] [review] does too, it will be *this* attachment that lands.

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] Silver, I think we should get this onto the branch on Tuesday or Wednesday. I'll huddle with the other drivers tomorrow and see if we can get this approved.

From IRC: <Silver> Anyway, I need to do other stuff. And to top everything off, I'm away from tomorrow morning until Friday. So I'm not sure this is the best time to land it. On top of that, Silver's current feedback line patch doesn't work quite properly (at least on trunk, which is identical except for a line that I had to move up two lines), whereas the one that has SR right now does. If we're going to land, we should combine that one with this one and (hopefully) Tonglebeak's patch for changing the location bar flex in browser.xul, rather than use his 'branch patch + fix bad assumptions' patch. We should also wait until we've made sure that there are no GDI resource concerns from Win9x and ME users on branch. While they don't appear to, it's possible that canvas caused this, so we need to confirm this with the canvas builds. So, while it's conceivable that this could land on Tuesday or Wednesday, it'd be much more doable by the time Silver was back (on Friday).

Comment on attachment 199109 [details] [diff] [review] Fix the bad assumptions in browser.css with some more assumptions +#personal-bookmarks { + min-width: 21px; /* size of the chevron button */ +} + what about pinstripe?

Re comment 153: I've already fixed the bookmark toolbar foder issue. Re comment 154: trivial to fix, what other themes in CVS need that hack? (and why the heck aren't they all based on one with platform overrides?)

Pinstripe is a totally different theme (default on mac), so platform overrides wouldn't do. Gnomestripe probably doesn't need the fix, IIRC it doesn't override browser.css. Hope that helps ;)

Ah, ok. I can only see a winstripe and pinstripe folder in mozilla/browser/themes, so just those two got updated and hopefully that'll cover all the themes used by default. Also, re availability: I am away from my computer from 10 minute until 2pm Friday, UK time (BST). I will be able to get bugmail, and remote access to my computer, on Wed and Thu for the exact periods of 9:30am to 5:30pm UK time (BST). I *can* land during that time, and can watch the tree and so on, but only if everything (approvals, etc.) is ready on Wed or Thu *morning*. (If you want someone else to land, make sure to update the final patch I attached with the fix document in bug 311798 and the pinstripe update. :) )

Noticed "toolbar, menubar {border-style: none !important}" to remove toolbar separators is broken on the trunk probably since 2005-10-06. This is visible using the default winstripe theme.

(In reply to comment #158) > Noticed "toolbar, menubar {border-style: none !important}" to remove toolbar > separators is broken on the trunk probably since 2005-10-06. > This is visible using the default winstripe theme. > this works /* Remove seperator between toolbars*/ #toolbar-menubar { min-height: 12px !important; padding: 0px !important; margin: 0px !important; border: none !important; } #nav-bar { border: none !important; padding: 0px !important; } #PersonalToolbar { border: none !important; } #navigator-toolbox { border-bottom-width: 0px !important;}

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] This was going to be landed by mconnor earlier today. His build environment didn't come together in time, though. Plussing per triage meeting. Will land on behalf of mconnor on behalf of patch author.

Comment on attachment 197534 [details] [diff] [review] Updated with comments [checked in] Landed on the Mozilla 1.8 branch. Checking in browser/base/content/browser.xul; /cvsroot/mozilla/browser/base/content/browser.xul,v <-- browser.xul new revision: 1.268.2.5; previous revision: 1.268.2.4 done Checking in browser/components/bookmarks/content/bookmarksMenu.js; /cvsroot/mozilla/browser/components/bookmarks/content/bookmarksMenu.js,v <-- bookmarksMenu.js new revision: 1.48.2.5; previous revision: 1.48.2.4 done Checking in browser/themes/winstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v <-- browser.css new revision: 1.17.2.4; previous revision: 1.17.2.3 done Checking in gfx/src/windows/nsNativeThemeWin.cpp; /cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.cpp,v <-- nsNativeThemeWin.cpp new revision: 3.45.4.1; previous revision: 3.45 done Checking in gfx/src/windows/nsNativeThemeWin.h; /cvsroot/mozilla/gfx/src/windows/Attic/nsNativeThemeWin.h,v <-- nsNativeThemeWin.h new revision: 3.16.20.1; previous revision: 3.16 done Checking in layout/style/nsCSSKeywordList.h; /cvsroot/mozilla/layout/style/nsCSSKeywordList.h,v <-- nsCSSKeywordList.h new revision: 3.74.6.3; previous revision: 3.74.6.2 done Checking in layout/style/nsCSSProps.cpp; /cvsroot/mozilla/layout/style/nsCSSProps.cpp,v <-- nsCSSProps.cpp new revision: 3.130.6.3; previous revision: 3.130.6.2 done Checking in toolkit/themes/winstripe/global/autocomplete.css; /cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v <-- autocomplete.css new revision: 1.5.8.1; previous revision: 1.5 done Checking in toolkit/themes/winstripe/global/global.css; /cvsroot/mozilla/toolkit/themes/winstripe/global/global.css,v <-- global.css new revision: 1.8.4.1; previous revision: 1.8 done Checking in toolkit/themes/winstripe/global/menu.css; /cvsroot/mozilla/toolkit/themes/winstripe/global/menu.css,v <-- menu.css new revision: 1.5.2.5; previous revision: 1.5.2.4 done Checking in toolkit/themes/winstripe/global/popup.css; /cvsroot/mozilla/toolkit/themes/winstripe/global/popup.css,v <-- popup.css new revision: 1.4.2.4; previous revision: 1.4.2.3 done Checking in toolkit/themes/winstripe/global/toolbar.css; /cvsroot/mozilla/toolkit/themes/winstripe/global/toolbar.css,v <-- toolbar.css new revision: 1.7.2.3; previous revision: 1.7.2.2 done Checking in widget/public/nsILookAndFeel.h; /cvsroot/mozilla/widget/public/nsILookAndFeel.h,v <-- nsILookAndFeel.h new revision: 1.47.2.1; previous revision: 1.47 done Checking in widget/src/windows/nsLookAndFeel.cpp; /cvsroot/mozilla/widget/src/windows/nsLookAndFeel.cpp,v <-- nsLookAndFeel.cpp new revision: 1.50.2.1; previous revision: 1.50 done I'll respin our Firefox and Thunderbird build systems this evening.

fixed1.8

Did you deal with the proposal of comment #159?

Did you deal with the proposal of comment #159? Is this an issue on the branch at all?

(In reply to comment #164) > Did you deal with the proposal of comment #159? Is this an issue on the branch > at all? I landed the attachment that was cited in the bug. Nothing else.

Chase, please read comment 157 (esp. the last paragraph).

(In reply to comment #166) > Chase, please read comment 157 (esp. the last paragraph). Sure, I read it. No offense meant but what's that mean to me? I don't have time to work on this bug anymore than I already have. I landed the patch (repeating, here) on behalf of mconnor on behalf of the triage meeting. mconnor can sort out the rest tomorrow.

Argh. Argh. Argh. If you're going to land something on someone else's behalf, and least get the damn thing right. You've missed both the pinstripe fix, and regressed bookmark dragging per bug 311798 (see attachment 199109 [details] [diff] [review]). Way to go. I hope someone's planning to fix both ASAP.

(In reply to comment #164) > Did you deal with the proposal of comment #159? Is this an issue on the branch > at all? Was this comment meant for me? I´ve edited userchrome.css but it doesn´t work. Haven´t tried this on the branch

That comment was not meant for you, Judha. To be honest, no-one here should care about what you said in comment 158, because it is obvious that this bug changes the behaviour of it - that's the entire point of this bug. It is also not the responsibility of people on the bug to tell you how to make your CSS work again, you should instead use the MozillaZine forums or a similar place. Also, Chase, marking this bug FIXED was completely and utterly the wrong thing to do. If you'd actually read the bug before commiting random patches on it, you'd know this, and know why - I would suggest drivers be way more careful about forcing stuff through without understanding it.

James, so how about fixing what was done. If this doesn't happen pretty damn fast, it's not going to happen. Drivers (and chase) were working to try to give this patch (set of patches) a shot at making Firefox 1.5. You were away, our time was short. If you're back, please help fix it. If it doesn't happen today, it's very likely that we won't be able to take this.

(In reply to comment #171) > James, so how about fixing what was done. If this doesn't happen pretty damn > fast, it's not going to happen. Drivers (and chase) were working to try to give > this patch (set of patches) a shot at making Firefox 1.5. You were away, our > time was short. If you're back, please help fix it. If it doesn't happen today, > it's very likely that we won't be able to take this. I've explained my situation - I have 1.5 hours max access to my box. I am not back, and what is so special about today? It's still 2 weeks 'til RC1... I need approval for attachment 199109 [details] [diff] [review] before I can fix this, and I do NOT want to think what the CVS conflicts on branch are going to be like in my tree... (you've made it MUCH harder for me to land).

Ok, local tree is ok. Waiting for approval to land...

Should we be creating new bugs for issues that are discovered? I just noticed that the 3D border around the "Object - DOM Node" in the DOM Inspector is missing (at least the top portion of the border) using the latest branch build. BUILD: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051012 Firefox/1.4.1 ID:2005101203 ~B

That'll be DOM I assuming what borders exist on the header thing, probably. And frankly, you can do whatever you want - everyone else is! (Note that *standard practise* is to file followup bugs about minor issues like that.) Drivers need to make a call and set the flag. I will land or not according to that. I will not make any more changes to anything on this bug, as drivers are already making up random deadlines that are almost impossible to meet.

Comment on attachment 199109 [details] [diff] [review] Fix the bad assumptions in browser.css with some more assumptions james, I'm giving you approval to land. we can get these additional reviews after landing. thanks for jumping right in this morninng. I know we have two more weeks, but this is the kind of change that really needed to happen before beta 2 and we're making a very rare exception in taking something this large this late in the game so we wanted it in as soon as was possible. Again, sorry for making things more difficult. We were just trying to give this patch a chance.

Ok, will land as soon as CVS has cleaned up the mess (sigh) that fast-update just created. Please note: I CAN NOT WATCH ANY TREE AFTER 5:30pm BST - that is 40 minutes from now. If anything occurs after that, someone else will need to handle it.

this bug appears to have caused regression Bug 311675 which is now on the branch.

Which, if you read my patch and this bug, is dealt with by the patch I'm just about to land (it sets a min-width on the toolbar, which although not ideal [the flex change would be better], it was what we came up with at the time).

Comment on attachment 199109 [details] [diff] [review] Fix the bad assumptions in browser.css with some more assumptions The drag-drop indication changes are a bit hairy, maybe that idea of altering the border on the label would be simpler (or you could implement native drag feedback... maybe not). Either way try to use > in style rules wherever possible - .bookmark-item[dragover-bottom="true"] .menu-drop-marker-box causes the style system to check all ancestors of all .menu-drop-marker-box elements in case they're a bookmark item. >Index: gfx/src/shared/nsNativeTheme.cpp >+ mMenuActiveAtom = do_GetAtom("_moz-menuactive"); >Index: gfx/src/shared/nsNativeTheme.h >+ nsCOMPtr<nsIAtom> mMenuActiveAtom; I don't see these being removed from the windows/gtk2 versions, which would have been the point. >+ // for this item. We will pass any necessary information via aState, Ah yes, I only just noticed the misspelling in the previous patch :-)

If you read the comments, both issues have been addressed in the patch I'm still trying to check in (using >*> and the spelling).

Ok, checked in. Still updating and checking I've not missed a bit, after the mess fast-update made, though.

Landed one bit I missed. I think it is all in now, and I'm going off-line until 9:30am BST tomorrow morning. Although I'll be around then, I'd suggest anything that needs urgent fixing is done today by someone else (missing part of the check-in demonstrates what state I'm in this week).

Also caused bug 312205, I think.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20051012 Firefox/1.4.1 ID:2005101210 (win2k/classic) http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20051012+0932&maxdate=20051012+0940&cvsroot=%2Fcvsroot 09:32 and 09:40 checkins caused 2 regressions: 1. vertical align:middle for menu buttons changed to :top ? 2. favicons aren't resized anymore

(In reply to comment #181) >If you read the comments, both issues have been addressed in the patch I'm still >trying to check in (using >*> and the spelling). Sorry, I'm still backlogged with bugmail, so I could easily have missed or overlooked the comment to which you appear to refer. The spelling was not an issue per se, I was just glad someone was sufficiently awake to spot it.

Attachment 199109 [details] [diff] caused a signficant number of regressions by changing the anonymous content structure of menu.xml, especially the "menu-iconic" binding: many instances of CSS in the tree use selectors of the form http://lxr.mozilla.org/mozilla/source/mail/themes/pinstripe/mail/compose/messengercompose.css#760 or http://lxr.mozilla.org/mozilla/source/browser/themes/winstripe/browser/browser.css#138 We are going to need to back out these changes to menu.xml. We need to decide whether this means backing out the entire checkin here, or making targeted changes to menu.xml that do not affect this style of selector.

Mozilla/5.0 (Windows; U; Win98; ja-JP; rv:1.8b5) Gecko/20051012 Firefox/1.4.1 {Build ID: 2005101216} Menu arrow is not displayed in the rightmost of menuitem. It happens by the theme without '-moz-appearance: menuitem' in 'menu, menuitem' ,in a word, almost all themes. This happens only by branch. It doesn't happen in trunk.

(In reply to comment #188) > Created an attachment (id=199378) [edit] > screenshot > > Mozilla/5.0 (Windows; U; Win98; ja-JP; rv:1.8b5) Gecko/20051012 Firefox/1.4.1 > {Build ID: 2005101216} > > Menu arrow is not displayed in the rightmost of menuitem. > It happens by the theme without '-moz-appearance: menuitem' in 'menu, menuitem' > ,in a word, almost all themes. > This happens only by branch. It doesn't happen in trunk. Aren't the arrow there to show that there's a submenu for the menuitem? I don't see how there's a problem here (and I assume you meant "now displayed" instead of "not displayed," else I'd think you were cuckoo :P)

(In reply to comment #189) > (In reply to comment #188) > > Created an attachment (id=199378) [edit] [edit] > > screenshot > > > > Mozilla/5.0 (Windows; U; Win98; ja-JP; rv:1.8b5) Gecko/20051012 Firefox/1.4.1 > > {Build ID: 2005101216} > > > > Menu arrow is not displayed in the rightmost of menuitem. > > It happens by the theme without '-moz-appearance: menuitem' in 'menu, menuitem' > > ,in a word, almost all themes. > > This happens only by branch. It doesn't happen in trunk. > > Aren't the arrow there to show that there's a submenu for the menuitem? I don't > see how there's a problem here (and I assume you meant "now displayed" instead > of "not displayed," else I'd think you were cuckoo :P) Not displayed *in the rightmost of menuitem*. In other words, they are supposed to be right justified.

Well, maybe if he disables his Flatmenus extension, there will be no problem :)

I'm still not convinced that this patch gets the menu color always right. If you change the menu color in the display properties, Firefox simply ignores this change. While you can argue about the color of the menu bar (which Explorer also sees more as toolbar than as menu), the menus itself are for all apps I've tested consistently in the chosen color. And this is not after some wild themeing, but after a change even Joe User might make intentionally. BTW this seems to be reproducible under both Classic and Luna (see my comment #73 above).

(In reply to comment #187) > Attachment 199109 [details] [diff] [edit] caused a signficant number of regressions by changing the > anonymous content structure of menu.xml, especially the "menu-iconic" binding: > many instances of CSS in the tree use selectors of the form If you refuse to have the menu.xml changes, you can't have the entire thing. It is required to prevent regressing the bookmark dragging markers. You are welcome to fix the other places that make silly assumptions about the anonymous structure of menu.xml, I don't care either way.

(In reply to comment #187) > many instances of CSS in the tree use selectors of the form > http://lxr.mozilla.org/mozilla/source/mail/themes/pinstripe/mail/compose/messen gercompose.css#760 > or > http://lxr.mozilla.org/mozilla/source/browser/themes/winstripe/browser/browser. css#138 Since the only places that are broken are the emoticon popup in Tbird compose (the *other* popups are fine...), and the bookmarks menu favicons (plus a little bit in the toolkit themes, which seems to not have been very important anyway, as it hasn't made any difference now it doesn't apply), why not let someone fix it to cope? I'm quite happy to scan the tree for CSS that touches any of the CSS classes I've moved about, and update them, but it's your call. I know that changing the anon XBL is 'scary', but that doesn't mean it shouldn't be done - it only serves to highlight the shortfalls in what you lovingly call the 'platform'. If there has still been no acceptance to actually make this work, I will back out every single part of this bug from branch on Friday, at around 4PM BST.

Seeing that this is also put into the beta/rc releases of FF 1.5, the regressions caused by this very late checkin of so many changes, particulary the menu.xml, which also causes regressions for allmost all third party themes, I respectfully ask to back this out, or at least the menu.xml stuff. Note that the changes required to themes because of this change may well make the themes incompatible with the older builds and older releases. This bug is all about UI candy, and should not have such dramatic regressions just before a major release! At the very least, provide some help for those poor themers, having to fix their themes again, especially for the 'right arrow' not being right aligned...

Forgive me my ignorance, but shouldn't you draw a menu background for non-flat menus as well (from attachment 199109 [details] [diff] [review]): + case NS_THEME_MENUPOPUP: + if (mFlatMenus) { + ::FillRect(hdc, &widgetRect, (HBRUSH) (COLOR_MENU+1)); + ::FrameRect(hdc, &widgetRect, ::GetSysColorBrush(COLOR_BTNSHADOW)); + } else { + ::DrawEdge(hdc, &widgetRect, EDGE_RAISED, BF_RECT | BF_MIDDLE); + } + return NS_OK; Removing -moz-appearance: menupopup from the menus' styles corrects the issue (then we're back with the CSS classes which correctly apply the menu colors).

Yes, though the chance to fix that was weeks ago, when it was under review. I'm happy to fix it, but drivers, who knows...

James, if you can fix the regressions this introduced, there's still a chance we can keep this on the branch.

The regressions I see in testing (maybe all the same issue) are that favicons aren't sized properly, menu submenu expand arrows are not right aligned, the smiley images are missing from the smiley toolbar menu in HTML compose in Thunderbird, and one I'm not terribly concerned about, that the menus are aligned to top rather than middle when you make the menubar taller by adding, say, a large icon.

I will attempt to fix all those issues by tomorrow evening (I'm only at my computer from about 1PM BST tomorrow). Watch this space.

Solved some of the menu issues with my 'non-native themes' as follows: menuitem > stack, menu > stack, .menu-inner-content { -moz-box-flex: 1; } This makes these extra boxes behave, but may be there are better ways to this?

To fix the favicon sizing problem I changed the selectors from browser.css that Ben S pointed out. I put the altered rule in my userChrome.css and all of the problem favicons are 16 x 16 like they should be. Here's the selector I used: .bookmark-item > stack > .menu-inner-content > .menu-iconic-left > .menu-iconic-icon I think a similar change could be done for Thunderbird's smiley problem.

http://lxr.mozilla.org/mozilla/search?string=menu-iconic-left There are many more CSS selectors affected than just bookmark icon sizes and tbird smilies. I do not think that the XBL structure change is acceptable on the branch. We need to at least back out that part of the patch and figure out a different workaround for whichever bug it was solving.

Fix for the CSS selectors. [13-10-2005 18:31] <Hannibal> suppose I succeed in building TB [13-10-2005 18:31] <Hannibal> and the patch works [13-10-2005 18:31] <[Not]Silver> Yes... [13-10-2005 18:31] <Hannibal> can I just attach it and set r? + sr? [13-10-2005 18:32] <[Not]Silver> Set a? too, and say that I approved it as part of the regression fixing. bsmedberg requested me to put this up already. I'm still trying to get TB to build and test these fixes myself. Requesting r, sr and a before having tested it because several people on the bug stated that similar fixes worked for them.

Even if you patch all the in-tree uses, the problem remains for extension authors, theme creators and anyone else relying on the old structure. I really don't think such a change is a good idea this late in the game, especially after releasing two betas with the previous behavior.

Well, this patch certainly breaks all my themes. Mostly Crystal: http://gamergod.users.btopenworld.com/crystal.JPG Mrtechs Local install http://gamergod.users.btopenworld.com/mrtech.JPG http://gamergod.users.btopenworld.com/mrtech2.JPG Safire: http://gamergod.users.btopenworld.com/bk.JPG Hope this helps in your bugfixing.

(In reply to comment #205) > Even if you patch all the in-tree uses, the problem remains for extension > authors, theme creators and anyone else relying on the old structure. I really > don't think such a change is a good idea this late in the game, especially after > releasing two betas with the previous behavior. That's one of the reasons we have the 1.4.1 internal version, which will be bumped to 1.5 and make all the themes incompatible anyway.

> That's one of the reasons we have the 1.4.1 internal version, which will be > bumped to 1.5 and make all the themes incompatible anyway. Except for those marked as compatible up to the trunk (1.6a1 or 1.6 on AMO), and they probably still work on the trunk too.

(In reply to comment #206) > Well, this patch certainly breaks all my themes. > > Mostly Crystal: > http://gamergod.users.btopenworld.com/crystal.JPG > Mrtechs Local install > http://gamergod.users.btopenworld.com/mrtech.JPG > http://gamergod.users.btopenworld.com/mrtech2.JPG > Safire: > http://gamergod.users.btopenworld.com/bk.JPG > Hope this helps in your bugfixing. I should think that theme authors *will* have to fix themselves. They had to do that anyway, and as Caleb said, the version number will still be in their way *anyway*. Lastly, let's not forget that without this patch, the *default* theme will look horriffic on a large amount of Windows systems. As will quite a number of the available themes out there.

(In reply to comment #207) > That's one of the reasons we have the 1.4.1 internal version, which will be > bumped to 1.5 and make all the themes incompatible anyway. Just because there is an easy way to indicate that changes were made that could potentially break extensions/themes doesn't excuse making such changes late in a release cycle. Yes, they already have to bump up their version numbers and ensure compatibility, but that doesn't mean it's OK to throw in last minute changes that increases their burden.

This late in the game, I hoped that following the branch I only had to change the version number so that the themers and extensionmakers (extenders?) are able to very quickly release new versions, thoroughly tested and all, because of the availability of beta1 and beta2. But this change, changes a lot within one of the more core widgets of the system, forcing us to recode, retest, analyse and fix issues, retest again, all in a very short timeframe before 1.5 is released. As far as I understand the main part of this bug is indeed needed, but the change to menu.xml, adding stack, .menu-content-inner and other stuff, was a late addition to these series of patches... I am not convinced that this late change itself is really required, and that there are no other ways to solve. Please remember there are many other bugs with complete and tested patches put on hold to make sure 1.5 will be very stable release. Allmost 100.000.000 people will be very disappointed if it may look nice on Windows, but that all themes and extensions are broken and released very late, let alone any other to be detected regressions caused by this. Please note that the last patch introducted very complex selectors: .menulist-menupopup > menuitem > * > * > .menu-iconic-left, This will for sure have a performance impact! The last patch really shows the kind of mess that is caused by this all...

Well, there's a CSS-only patch in bug 311798 that has r+sr. I know Neil didn't like it because it mixed a few types of selectors, but taking any of them out makes the patch useless (I've tried), probably due to either CSS weight or position, and I don't want to use ! important because (as I recall) that sucks lots for anyone trying to modify the CSS after the fact (e.g. in userChrome.css). Besides, I don't want the changes to apply to all of them... The patch produces slightly different behavior, but doesn't change any XML files or whatnot. If you want to back out the menu.xml part of the patch, there IS an alternative. Fixing this one will mean that the behavior is identical for users coming from 1.0, though.

(In reply to comment #211) > Allmost 100.000.000 people will be very disappointed if it may look > nice on Windows, but that all themes and extensions are broken and released very > late, let alone any other to be detected regressions caused by this. I would be very surprised if the majority of Firefox users use a different theme then the default theme. IMO its more important to have the default Theme look great on all OS Versions than to maintain compatibility with 3rd party Themes.

So alot of themes broke. What is the fuss all about, really? Do you realize how many times core changes were made that broke extensions? Many. So why should this be treated any differently? It's up to the theme authors to be compatible with Firefox, not the other way around, so really the complaining about "omg this breaks 3rd party themes" shouldn't be taken seriously, as it's the theme author's responsibility to keep up.