User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050914 Minefield/3.0pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050914 Minefield/3.0pre ID:2008050914 This is a spin-off from bug 411064 as requested by Simon Bünzli and is specifically to address Simon's comment in that bug: > 1. Then there's a rounding error due to a minimal height being given in em > instead of px. That's something which bug 337771 didn't fix completely. Code > to blame: > http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/themes/winstripe/global/menu.css&rev=1.14&mark=110#107 >(don't ask me where those 1.21em come from - I should know, but I don't). Work-around for userChrome.css: .menu-iconic-left, .menu-right { min-height: 13px !important; } Reproducible: Always Steps to Reproduce: 1. Launch a recent build of Gran Paradiso 2. Click View-> Toolbars-> Navigation toolbar 3. Observe that menu selection highlights are uneven and have regressed from FF 2.x. This is the case for all most all selection highlights as seen in the attached screenshots Actual Results: Observe that menu selection highlights are uneven and have regressed from FF 2.x. This is the case for all most all selection highlights as seen in the attached screenshots. Expected Results: Menu selection highlights are even and have not regressed from FF 2.x.

Is this only classic theme?

I believe this is occurring on all Windows themes. ~B

Classic, Luna, Royale, etc. But not Aero.

I'm think this is a Windows theme issue.

Are drivers not taking very low risk patches for the RC's? A patch for this issue would be extremely low risk. ~B

(In reply to comment #6) > Are drivers not taking very low risk patches for the RC's? > No, they are not. There are no-risk patches even safer than this that will not be accepted. Unless it's a showstopper, it's going to have to wait for 3.0.1.

Could someone mark this wanted next? Bryan

We can only ask for blocking1.9. Drivers can mark it as wanted next.

(In reply to comment #9) > We can only ask for blocking1.9. Drivers can mark it as wanted next. > Or I guess we could wait until 1.9.1 flags are available and request one of those. Do we need a wanted-next flag at this point in time?

There are four vertical alignment issues with menus. The first three are the ones that Simon enumerated in bug 411064 comment 27. 1) Checkboxes and radio items were reserved a 16x16 space. This was fixed by bug 411064 (and the only one of the four issues fixed so far). 2) The 1.21em min-height causes some menu items to be 1px taller than others. Simon had the correct fix for that in bug 411064 comment 37 (remove the min-height entirely), and I can confirm through my testing that this is indeed the right thing to do, and I think we should do it for this bug. 3) Menu flyouts can be incorrectly positioned 1px higher (no matter how misaligned the heights are, the top edge of the first menu item of the flyout menu should be flush with the top edge of the parent menu item, space permitting), and as Simon suggested, this is indeed a totally separate issue, caused by twip/pixel conversion rounding errors in the popup positioning code (bug 422607). I can confirm that the patch in that bug fixes the "zoom/text-zoom-in" issue in the first column, third row of Bryan's screenshot collage. 4) If there is a 16x16 icon in the menu, the icon is vertically mis-aligned (1px on top, 3px on bottom), and if there is also an arrow for a flyout menu on the right side, that would be vertically misaligned, too. This is the problem reported in bug 419439 and bug 429024. It's caused by the 1px/3px top/bottom padding in nsNativeThemeWin.cpp: <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/windows/nsNativeThemeWin.cpp&rev=3.127&mark=2087,2088#2086> While this ensures the proper alignment of text and glyphs in normal-height menu items, it doesn't work as well for taller menu items. So in a nutshell... 1) Adopt the removal of min-height from Simon's patch in bug 411064 comment 37. 2) Find a better way to vertically align the text and glyphs that doesn't cause problems with menus that have 16x16 icons. I'll look into this later tonight or tomorrow. 3) Popup positioning will be fixed by bug 422607, so we don't have to worry about it here.

Problem #4 (nutshell #2) is surprisingly difficult. I see two general approaches to this problem: 1) Make a change to <menuitem> (use 2px/2px instead of 1px/3px). 2) Make a change to menu-iconic-left (use +1px/-1px to compensate). The problem with #1 is that it will require changing the padding or margins of all of the child elements that make up the <menuitem> to compensate. Compensating for this in CSS will also affect menus rendered by uxtheme (e.g., the Aero styling), so it needs to be changed in nsNativeThemeWin.cpp (or overridden there for the uxtheme case), but that would require creating a new -moz-appearance for the accel text, which is an idea that I don't like: it seems like overkill. The problem with #2 is similar. The only way to shift the 16x16 icon down by 1px without changing the amount of layout space consumed is through +1/-1 top/bottom margins. But if that's done through CSS, then it would affect uxtheme menus, but margins can't be set by nsNativeThemeWin.cpp. Simon, do you have a better idea on how to do this, because it looks fairly intractable to me at the moment.

(In reply to comment #12) > Problem #4 (nutshell #2) is surprisingly difficult. > I guess I spoke too soon. I think this ought to work: * cut the top/bottom <menuitem> borders from 1px/3px to 0px/2px * compensate for the reduced height by adding 1px/1px top/bottom padding or margins to the menu text (since it's symmetric, it'll be safe for uxtheme menus) * add a 2px top padding to NS_THEME_MENUIMAGE in the non-uxtheme case to compensate for the 2px <menuitem> bottom border and achieve alignment

I've added the flags as requested, but why isn't this in Firefox::Theme?

(In reply to comment #14) > why isn't this in Firefox::Theme? > Firefox doesn't do anything special with menu appearance; all the stuff governing the appearance of menus reside in Toolkit and Core, so this is a bug that affects all toolkit apps.

What this patch does: 1) Fix issue #2 in comment 11 by eliminating the unnecessary min-width. 2) Fix issue #4 in comment 11 by implementing comment 13 3) Add a 1px top padding to the right flyout arrow so that it aligns properly when the menu item is of an even height (it does not affect alignment when the menu is at an odd height) (this is really just a part of issue #4) 4) [side issue] While testing, I noticed a minor bug: the image in menu-iconic-left was missing the menu-iconic-icon class (there was CSS code depending on that, too), so I fixed that. 5) [side issue] With #2 and #3 fixed, I decided that having the icons crowded too close to the edge looked pretty bad, especially now that the icons are no longer crowded at the top, thus making the left-crowding more obvious. The -1px margin hack was something that I really should have removed in bug 332314 instead of perpetuating it, as that was originally there to make up for the small gutter size (which had been slightly increased by bug 332314). It was ugly, and even uglier now, so I killed it (its removal also makes it possible for bug 435825 to achieve the correct horizontal icon alignment in Vista). 6) [side issue] The space reserved for the right arrow was reverted to 1.28em. It was changed to 1.45em by bug 332314, but that change was not necessary for that bug (and was only the result of the two items happening to share the same rule block), and was reverted here. I've tested this patch on XP and Vista, with both Classic and non-Classic themes, and with various font sizes, and everything aligns correctly. (n.b.: There are a few other remaining minor menu alignment issues, but they are unrelated to this bug/patch, and they will be addressed by bug 422607 (see issue #3 in comment 11) and bug 435825.)

Comment on attachment 322601 [details] [diff] [review] patch >Index: toolkit/themes/winstripe/global/menu.css >=================================================================== >+ -moz-margin-start: 2px !important; /* update nsNativeThemeWin.cpp if this changes */ You might want to be more specific as to which bit in that large file would have to be updated. The rest looks alright at a cursory glance.

(In reply to comment #18) > You might want to be more specific as to which bit in that large file would > have to be updated. > Sure. Actually, that comment shouldn't even be a part of this patch; I intended it for bug 435825, but I forgot to separate it out when making the patches.

Just noticed this and can't find another bug, thought you might be interested. Might be that your patch resolves this already! ~B

(In reply to comment #20) > Screenshot of additional issue... > The patch addresses that (see comment 17, point 3)

Is this on anyones radar? We're waiting on approval! :-) ~B

No, you're waiting on review.

Comment on attachment 322601 [details] [diff] [review] patch r=me on nsNativeThemeWin parts, esp. since it only affects classic.

but now we're waiting for approval request ... ;)

Ping!

Gavin, do you have to review this patch too?

as an type of escalation: can we get a fast(er) review and checkin for this almost three month old patch before it's rotten, please?

Comment on attachment 322601 [details] [diff] [review] patch Sorry for the delay.

Same as before, except with comment 19 addressed (removing a comment from menu.css that I had intended for bug 435825 and not for this bug).

Is it possible to get this checked in or are checkins limited at this time? ~B

Is there reason why the approved patches cannot be checked in? ~B

Hm this "bug" exists in Win XP, IE 6 and IE 8 Beta 2, dont have IE 7 right now, but if IE 6 and IE 8 have it, it should be the same in IE 7. Assuming Firefox 3 is much more integrated with native Win environment than Firefox 2, I cant see this as bug really.

Oops, sorry ignore my previous post, I was talking about menupopup without icons followed by menuitem with icon and the menuitems with and without icons are with different height. This screenshot is with Firefox 3.0.1 and the menuitems are equal. I can't reproduce this bug.

(In reply to comment #33) > Hm this "bug" exists in Win XP, IE 6 and IE 8 Beta 2, dont have IE 7 right now, > but if IE 6 and IE 8 have it, it should be the same in IE 7. IE does not have the alignment issues as shown in the screen shot I attached. ~B

With current latest trunk: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080830031750 Minefield/3.1b1pre Not reproducible too, the result is same as the screenshot from Firefox 3.0.1

(In reply to comment #36) > Not reproducible too, > the result is same as the screenshot from Firefox 3.0.1 Emil, There are other issues you're obviously missing and are addressed by the patch provided in this bug. Take a look here for an explanation: https://bugzilla.mozilla.org/show_bug.cgi?id=433109#c17 ~B

You are right, but since the patch fixes the alignment and the alignment is already fixed, shouldn't the patch be updated to avoid conflicts? Attaching another screenshot from latest trunk build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080830031750 Minefield/3.1b1pre The alignment is even better than 3.0.1 on other menus which aren't on top and submenus are appearing on the same line.

(In reply to comment #38) > You are right, but since the patch fixes the alignment and the alignment is > already fixed, shouldn't the patch be updated to avoid conflicts? Emil, Kai Lu states "I've tested this patch on XP and Vista, with both Classic and non-Classic themes, and with various font sizes, and everything aligns correctly." The patch Kai submitted addresses the issues it's intended to and won't cause conflicts. ~B

Emil, the main vertical menu issues that this bug addresses are the positioning of the icon with respect to the highlight, and a rounding problem where some menu items will be 1px taller than expected. Another rounding issue, which would result in the menu itself being positioned 1px too high was addressed by another patch (and that was the fix that you see in your screenshot) and was never within the purview of this bug. The patch should be fine as-is. Bryan, there are about 50 other bugs waiting in the checkin-needed queue right now. This is just a trivial polish bug, and no other bug or work depends on this bug, so I don't think there is any need to rush this.

After this patch, the chevrons in the bookmarks menu for Most Visited, Recent Tags and Recently Bookmarked are not aligned vertically with the user created folder chevrons. They're one pixel to the right! I don't have time to create a bug so hopefully someone else will. I'll note that the bookmarks toolbar chevron (I moved it there in my case) is also one pixel to the right. ~B

Tested with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080915032512 Minefield/3.1b1pre, Vista with Aero on. For custom menus like that one, the bottom edge of the submenu selector is 1 pixel too high. Doesn't occur with the built in menus (like the Character Encoding one), or on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080903201543 Minefield/3.1b1pre -- actually, I can't reproduce the original bug in that case either.

Please file a new bug - comments on a resolved bug are likely to be lost.

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080927033433 Minefield/3.1b1pre ID:20080927033433