411064 - iconic menu items are 2px higher than normal ones

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Bryan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010514 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010514 Minefield/3.0b3pre

Blue selection highlight for menu items should be consistent.  In some cases it's a pixel to low or two pixels to high.  See screen shots.  This is similar https://bugzilla.mozilla.org/show_bug.cgi?id=253661 which covered the exact same thing in FF 1.x.  This has regressed in FF 3.0.

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.

This is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=253661

Comment 1

[Bryan]

Attachment: Screenshot showing issue...

Comment 2

[Bryan]

Attachment: Screenshot showing issue...

Comment 3

[Bryan]

Attachment: Screenshot showing issue...

Comment 4

[Bryan]

Attachment: Screenshot showing issue...

Comment 5

[Bryan]

Attachment: Screenshot showing issue...

Comment 6

[Bryan]

Attachment: Screenshot showing issue...

Comment 7

[Ria Klaassen (not reading all bugmail)]

I can't find a duplicate, so confirming.

Comment 8

[Bryan]

This is offly sloppy.  Come on folks, let's clean this up before release!

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch?

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 307124 [details] [diff] [review]
patch?

this causes icons to be rendered outside of the menu item.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> (From update of attachment 307124 [details] [diff] [review])
> this causes icons to be rendered outside of the menu item.

With what config? It works fine for me using the default Vista theme, and the XP "classic" theme.

Comment 12

[Dão Gottwald [:dao]]

that was on XP classic.

Comment 13

[Dão Gottwald [:dao]]

Attachment: screenshot, XP Media Center style

Comment 14

[Dão Gottwald [:dao]]

Did you mean to write margin-top: -1px; margin-bottom: -2px;?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Oh, I just wasn't testing with a full icon :(

It seems like IE has tall 20px menuitems for the Favorites menu, which is the only menu where you can have arbitrary icons, and 17px tall menuitems everywhere else. I don't know whether 20px tall menu items is some kind of platform convention for iconic menus, or whether that's just specific to IE.

Comment 16

[Bryan]

There are other issues with the height of the highlights for each menu item as well.  I don't know if this patch resolves that as well.  You can see the issue here: https://bugzilla.mozilla.org/attachment.cgi?id=295691 - look at the difference between the height of the blue highlight over "Toolbars" and "Navigation Toolbar".

~B

Comment 17

[Bryan]

Attachment: Futher alignment issues...

Comment 18

[Bryan]

Attachment: Futher alignment issues...

Comment 19

[Bryan]

The attachment https://bugzilla.mozilla.org/attachment.cgi?id=307334 shows issues with alignment when the the keyhole back and forward buttons are enabled.  
"Technology" isn't left aligned with "Apple" above it.

Comment 20

[Bryan]

The attachment https://bugzilla.mozilla.org/attachment.cgi?id=307335 shows alignment issues with the Bookmarks menu.  Looks like it's off to the right by 1px.

I might add that "Help-> Check for Updates" has this same issue.  It's off to the left by 1px best I can tell.

~B

Comment 21

[Bryan]

Any possible movement on this Gavin or Dao?

~B

Comment 22

[Bryan]

Guys, can we get this in for B5?  It's an ovious glaring regression.

~B

Comment 23

[Simon Bünzli]

Bryan: It would be terribly helpful if you could create a single image composed from multiple screenshots which shows a comparison of Firefox 2, Firefox 3 and Windows Explorer side by side - and in the end duplicate the whole thing and highlight the "obvious glaring regression" with red in one half so that we don't have to hunt through half a dozen screenshots in order to spot the issue.

Comment 24

[Bryan]

I'll see what I can do tonight!

~B

Comment 25

[Bryan]

Attachment: Single image as requested by Simon that illustrates the alignment issues

Single image as requested by Simon that illustrates the alignment issues as seen by me.

~B

Comment 26

[Bryan]

There are other alignment issues shown in the single image that I didn't highlight because I didn't have time:

1)Screen shot upper left, the "Navigation Toolbar" text is spaced farther away by several pixels from the check mark when compared to FF2.0 (this applies in all cases where there are check marks in the menus).

2)Screen shot upper right, the (Off)(Character Encoding) isn't vertically aligned with the radio button to it's left when compared to FF2.0.

3)The history items listed in the Key hole combined history drop mark aren't vertically left aligned with the one that is selected (has the radio button).  It's to the right 1px.  And the radio button contained is not vertically aligned with the text itself that is to the right of it.

4)I previously filed https://bugzilla.mozilla.org/show_bug.cgi?id=393806 which covers an alignment issue with the "List all tabs" drop marker.

~B
~B

Comment 27

[Simon Bünzli]

Lovely, there are at least three different issues visible in your attachment:

1. We reserve space for a full 16x16 icon, even when we know that we'll display a checkbox/radio glyph. This is fall-out from bug 363130 which Gavin's patch from comment #9 tries to address. Code to blame: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/themes/winstripe/global/menu.css&rev=1.14&mark=113-116#113

Work-around for userChrome.css:

menuitem[type="checkbox"] > .menu-iconic-left > .menu-iconic-icon,
menuitem[checked="true"] > .menu-iconic-left > .menu-iconic-icon,
menuitem[type="radio"] > .menu-iconic-left > .menu-iconic-icon {
	height: inherit !important;
}

2. 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;
}

3. Finally we've got a similar problem with horizontal alignment, which however is also visible in the Luna themes and thus belongs to a different bug (which might already be filed).

Gavin, Dão, Alex: What chances would a patch making the above modifications have at this point?

Comment 28

[Simon Bünzli]

Attachment: minimal fix for issues 1 and 2

Comment 29

[Simon Bünzli]

(In reply to comment #25)
> Created an attachment (id=315683) [details]

I'm not sure, whether this is really a regression from Firefox 2.0, though. Your screenshots indicate that you've installed the Classic Menus extension (or use a similar CSS hack) which isn't a fair comparison... (Firefox 2.0's menus looked significantly more out of place, anyway).

Comment 30

[Simon Bünzli]

From winstripe's menu.css:
> .menu-iconic-left,
> .menu-right {
>   min-width:  1.28em;
>   min-height: 1.21em;
> }

James: I haven't managed to find an explanation for where these specific em values introduced in bug 313388 come from. Would you mind enlightening us (in case you still remember, that is)?

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 315741 [details] [diff] [review]
minimal fix for issues 1 and 2

Looks good to me in general...

> .menu-iconic-left,
> .menu-right {
>+  /* XXXzeniko em's lead to rounding errors (and aren't for horizontal lengths, anyway) */
>   min-width:  1.28em;
>-  min-height: 1.21em;
>+  min-height: 13px;
> }
> 
> .menu-iconic-icon {
>   width: 16px;
>   height: 16px;
> }
>+/* XXXzeniko the above should apply more selectively (see bug 411064) */
>+menuitem[type="checkbox"] > .menu-iconic-left > .menu-iconic-icon,
>+menuitem[checked="true"] > .menu-iconic-left > .menu-iconic-icon,
>+menuitem[type="radio"] > .menu-iconic-left > .menu-iconic-icon {
>+  height: inherit;

Where are you inheriting the height from? Why not height: auto?

Comment 32

[James Ross]

Simon, all the em values (and in fact pretty much every value I used in my patches) come from experimenting and verifying what the native menus do; in this case, the represent the area native menus allocate for icons/arrows as a derivative of the text size. I spent many hours checking a wide range of font sizes and many different themes to get the values, so by all means just causally blow them away. ;)

Comment 33

[Simon Bünzli]

(In reply to comment #31)
> Why not height: auto?

My bad, should of course be height: auto.

(In reply to comment #32)
> the represent the area native menus allocate for icons/arrows as a
> derivative of the text size.

Concerning specifically the min-height of .menu-iconic-left and .menu-right: is there a specific reason we have to pre-allocate that space at all? If there's content, it'll dictate the height itself - and else the menuitem's height is determined by the main label... what am I missing? At least in my few (Firefox-only) experimentations, even completely dropping the min-height didn't have any visible consequences besides fixing issue #2 from comment #27.

> so by all means just causally blow them away. ;)

Which is why I'm asking before making any changes. (As for my "em" related comment, we'd probably have to replace those lengths with the corresponding "ch" ones at some point. Of course, your expertise would be most appreciated in the process.)

Comment 35

[James Ross]

As best I can remember, it was to do with the vertical alignment of the images
in relation to the text, but I'm not absolutely certain. It is definitely worth
checking the following things with any patch to the menu sizing:
 - With normal and large fonts (i.e. changing the DPI).
 - With a variety of font sizes in the appearance settings.
 - With undersized, 'correct' sized and oversized menu icons.

As a slight aside, I'd be very curious to know when and why the collection of
values no longer give the right menu sizes - what changed? Anyway, not hugely
important at this stage, but I'm naturally curious.

Comment 36

[Bryan]

> I'm not sure, whether this is really a regression from Firefox 2.0, though.
> Your screenshots indicate that you've installed the Classic Menus extension (or
> use a similar CSS hack) which isn't a fair comparison... (Firefox 2.0's menus
> looked significantly more out of place, anyway).

You are correct, I did have the Classic Menus extension installed and forgot about.  This would be more of a regression from this bug then (which I note above) https://bugzilla.mozilla.org/show_bug.cgi?id=253661

The iconic menu item issue is the actual regression that I pointed to that was resolved by 253661 but others have occurred in FF3.0 as well from 2.0 IIRC.

~B

Comment 37

[Simon Bünzli]

Attachment: better fix

Verified with dozens of font/size variations: all menu items remain correctly sized with the min-height removed.

As for the rule from bug 363130: It'd be best if it only applied to menu(item)s of class .menu-iconic. Not going to risk such a change at this stage, though.

Comment 38

[Simon Bünzli]

(In reply to comment #35)
> As a slight aside, I'd be very curious to know when and why the collection of
> values no longer give the right menu sizes - what changed?

The height issue might have been due to one of several changes in CSS rounding (where IIRC there were issues until the first Beta). The horizontal issue stems from a bad fix to bug 393804 which simply overrules your lengths no matter what.

Comment 39

[Bryan]

Simon,

Your fix doesn't resolve the alignment issues in the Bookmarks or Help menus, correct?

~B

Comment 40

[Simon Bünzli]

(In reply to comment #39)
No - and that's not what this bug is about. Feel free to file a new bug and reference bug 393804 comment #16 for a start.

Comment 41

[Bryan]

Do we think we'll get this resolved before release?

~B

Comment 42

[Simon Bünzli]

Gavin: ping!

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 315959 [details] [diff] [review]
better fix

>Index: toolkit/themes/winstripe/global/menu.css

> .menu-iconic-left,
> .menu-right {
>   min-width:  1.28em;
>-  min-height: 1.21em;
> }

Which cases in Bryan's screenshot does this fix? I'm a bit nervous about making this change at this point, so if the other change fixes the majority of the issues I think we should avoid making this one.

Comment 44

[Simon Bünzli]

(In reply to comment #43)
> Which cases in Bryan's screenshot does this fix?

This fixes the second and third issue on the left and the middle one on the right, i.e. all issues where every second or so <menu> is 1px too high, causing  the corresponding <menupopup>s to be incorrectly aligned (see point #2 from comment #27).

Comment 45

[Simon Bünzli]

Attachment: better fix (only second part, cf. comment #43)

Comment 46

[Simon Bünzli]

Comment on attachment 315959 [details] [diff] [review]
better fix

Drivers: This patch fixes a second bug WRT vertical alignment in menus on Windows Classic (cf. comment #44), might however have minimal unpredicted consequences as to causing certain menu items to be slightly misaligned in the edge case where the menu items aren't of the usual size _and_ use images as checkboxes (overriding at least one -moz-appearance). If you're not willing to take that chance, please approve the following patch.

Comment 47

[Dão Gottwald [:dao]]

(In reply to comment #46)
> (From update of attachment 315959 [details] [diff] [review])
> Drivers: This patch fixes a second bug WRT vertical alignment in menus on
> Windows Classic (cf. comment #44)

As far as I can tell, that doesn't only apply to Classic.

Comment 48

[Damon Sicore (:damons)]

Comment on attachment 315959 [details] [diff] [review]
better fix

Does this still need an approval?  Which patch is correct.  Please re-request approval (just to make sure I'm approving the correct patch, i'm clearing the flag).

Comment 49

[Damon Sicore (:damons)]

Comment on attachment 318104 [details] [diff] [review]
better fix (only second part, cf. comment #43)

Clearing the approval request. Please re-request approval.  Not sure from the patch which one is correct or if both are.

Comment 50

[Simon Bünzli]

Comment on attachment 318104 [details] [diff] [review]
better fix (only second part, cf. comment #43)

Better safe than sorry, then.

Comment 51

[Damon Sicore (:damons)]

Comment on attachment 318104 [details] [diff] [review]
better fix (only second part, cf. comment #43)

a1.9+=damons

Comment 52

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/themes/winstripe/global/menu.css 	1.15

Bryan/Simon, can you file a followup about the issue from comment 44?

Comment 53

[Bryan]

(In reply to comment #27)
> 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;
> }

Simon,

It doesn't look like this particular fix made it into the patch.  Did you file another bug or should I?

~B

Comment 54

[Simon Bünzli]

(In reply to comment #53)
Please just go ahead and file one. Note that IMO we should just remove that min-height as AFAICT it's not needed any longer at all.

Comment 55

[Kai Liu]

(For future reference for anyone reading this bug, Bryan filed bug 433109 as a followup)

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0 ID:2008051206