Closed
Bug 411064
Opened 17 years ago
Closed 17 years ago
iconic menu items are 2px higher than normal ones
Categories
(Toolkit :: XUL Widgets, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9
People
(Reporter: BoxerBoi76, Assigned: zeniko)
References
Details
(Keywords: polish, regression)
Attachments
(5 files, 9 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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
Keywords: regression
Comment 7•17 years ago
|
||
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?
Updated•17 years ago
|
Component: Menus → Theme
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
QA Contact: menus → theme
Updated•17 years ago
|
Version: unspecified → Trunk
Updated•17 years ago
|
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
Comment 9•17 years ago
|
||
Comment 10•17 years ago
|
||
Comment on attachment 307124 [details] [diff] [review]
patch?
this causes icons to be rendered outside of the menu item.
Comment 11•17 years ago
|
||
(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•17 years ago
|
||
that was on XP classic.
Comment 13•17 years ago
|
||
Comment 14•17 years ago
|
||
Did you mean to write margin-top: -1px; margin-bottom: -2px;?
Comment 15•17 years ago
|
||
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.
Reporter | ||
Comment 16•17 years ago
|
||
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
Reporter | ||
Comment 17•17 years ago
|
||
Reporter | ||
Comment 18•17 years ago
|
||
Reporter | ||
Comment 19•17 years ago
|
||
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.
Reporter | ||
Comment 20•17 years ago
|
||
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
Reporter | ||
Comment 21•17 years ago
|
||
Any possible movement on this Gavin or Dao?
~B
Reporter | ||
Comment 22•17 years ago
|
||
Guys, can we get this in for B5? It's an ovious glaring regression.
~B
Assignee | ||
Comment 23•17 years ago
|
||
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.
Reporter | ||
Comment 24•17 years ago
|
||
I'll see what I can do tonight!
~B
Reporter | ||
Comment 25•17 years ago
|
||
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
Reporter | ||
Comment 26•17 years ago
|
||
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
Assignee | ||
Comment 27•17 years ago
|
||
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?
Assignee | ||
Comment 28•17 years ago
|
||
Attachment #315741 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 29•17 years ago
|
||
(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).
Assignee | ||
Comment 30•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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. ;)
Assignee | ||
Comment 33•17 years ago
|
||
(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•17 years ago
|
||
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.
Reporter | ||
Comment 36•17 years ago
|
||
> 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
Assignee | ||
Comment 37•17 years ago
|
||
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)
Assignee | ||
Comment 38•17 years ago
|
||
(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.
Reporter | ||
Comment 39•17 years ago
|
||
Simon,
Your fix doesn't resolve the alignment issues in the Bookmarks or Help menus, correct?
~B
Assignee | ||
Comment 40•17 years ago
|
||
(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.
Reporter | ||
Comment 41•17 years ago
|
||
Do we think we'll get this resolved before release?
~B
Assignee | ||
Comment 42•17 years ago
|
||
Gavin: ping!
Severity: trivial → minor
Whiteboard: has patch → [has patch][needs review gavin]
Comment 43•17 years ago
|
||
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+
Assignee | ||
Comment 44•17 years ago
|
||
(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 | ||
Comment 45•17 years ago
|
||
Assignee | ||
Comment 46•17 years ago
|
||
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?
Comment 47•17 years ago
|
||
(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•17 years ago
|
||
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 49•17 years ago
|
||
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?
Assignee | ||
Comment 50•17 years ago
|
||
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 51•17 years ago
|
||
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+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch][has review][needs approval]
Comment 52•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9
Reporter | ||
Comment 53•17 years ago
|
||
(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
Assignee | ||
Comment 54•17 years ago
|
||
(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•17 years ago
|
||
(For future reference for anyone reading this bug, Bryan filed bug 433109 as a followup)
Comment 56•17 years ago
|
||
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.
Description
•