Closed Bug 411064 Opened 14 years ago Closed 13 years ago

iconic menu items are 2px higher than normal ones

Categories

(Toolkit :: XUL Widgets, defect)

x86
Windows XP
defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: BoxerBoi76, Assigned: zeniko)

References

Details

(Keywords: polish, regression)

Attachments

(5 files, 9 obsolete files)

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
Attached image Screenshot showing issue... (obsolete) —
Attached image Screenshot showing issue... (obsolete) —
Attached image Screenshot showing issue... (obsolete) —
Attached image Screenshot showing issue... (obsolete) —
Attached image Screenshot showing issue... (obsolete) —
Attached image Screenshot showing issue... (obsolete) —
Keywords: regression
Keywords: polish
I can't find a duplicate, so confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is offly sloppy.  Come on folks, let's clean this up before release!
Flags: blocking-firefox3?
Component: Menus → Theme
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
QA Contact: menus → theme
Version: unspecified → Trunk
Component: Theme → XUL Widgets
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Product: Firefox → Toolkit
QA Contact: theme → xul.widgets
Summary: 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. → iconic menu items are 2px higher than normal ones
Blocks: 363130
Comment on attachment 307124 [details] [diff] [review]
patch?

this causes icons to be rendered outside of the menu item.
(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.
that was on XP classic.
Did you mean to write margin-top: -1px; margin-bottom: -2px;?
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.
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
Attached image Futher alignment issues... (obsolete) —
Attached image Futher alignment issues... (obsolete) —
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.
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
Any possible movement on this Gavin or Dao?

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

~B
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.
I'll see what I can do tonight!

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

~B
Attachment #295691 - Attachment is obsolete: true
Attachment #295692 - Attachment is obsolete: true
Attachment #295693 - Attachment is obsolete: true
Attachment #295694 - Attachment is obsolete: true
Attachment #295695 - Attachment is obsolete: true
Attachment #295696 - Attachment is obsolete: true
Attachment #307334 - Attachment is obsolete: true
Attachment #307335 - Attachment is obsolete: true
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
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?
Attached patch minimal fix for issues 1 and 2 (obsolete) — Splinter Review
Attachment #315741 - Flags: review?(gavin.sharp)
(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).
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 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?
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. ;)
(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.)
Duplicate of this bug: 429024
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.
> 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
Attached patch better fixSplinter Review
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.
Attachment #315741 - Attachment is obsolete: true
Attachment #315959 - Flags: review?(gavin.sharp)
Attachment #315741 - Flags: review?(gavin.sharp)
(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.
Simon,

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

~B
(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.
Do we think we'll get this resolved before release?

~B
Whiteboard: has patch
Gavin: ping!
Severity: trivial → minor
Whiteboard: has patch → [has patch][needs review gavin]
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.
Attachment #315959 - Flags: review?(gavin.sharp) → review+
(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).
Whiteboard: [has patch][needs review gavin] → [has patch][has review][needs approval]
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #318104 - Flags: approval1.9?
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.
Attachment #315959 - Flags: approval1.9?
(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 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).
Attachment #315959 - Flags: approval1.9?
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.
Attachment #318104 - Flags: approval1.9?
Comment on attachment 318104 [details] [diff] [review]
better fix (only second part, cf. comment #43)

Better safe than sorry, then.
Attachment #318104 - Flags: approval1.9?
Comment on attachment 318104 [details] [diff] [review]
better fix (only second part, cf. comment #43)

a1.9+=damons
Attachment #318104 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [has patch][has review][needs approval]
mozilla/toolkit/themes/winstripe/global/menu.css 	1.15

Bryan/Simon, can you file a followup about the issue from comment 44?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9
(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
(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.
(For future reference for anyone reading this bug, Bryan filed bug 433109 as a followup)
Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0 ID:2008051206
Status: RESOLVED → VERIFIED
Depends on: 433109
You need to log in before you can comment on or make changes to this bug.