Closed Bug 332314 Opened 18 years ago Closed 16 years ago

Menu items are improperly aligned when using iconic menuitems

Categories

(Toolkit :: UI Widgets, defect)

x86
Windows XP
defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: BoxerBoi76, Assigned: kliu)

References

Details

(Keywords: polish, regression)

Attachments

(6 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060330 Firefox/2.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060330 Firefox/2.0a1

"Bookmark This Page", "Bookmark All Tabs", etc. as well as "Open in Tabs" are not aligned vertically with bookmark folder names.  The padding here needs to be greater then in other sub-menus such as Tools.  See http://img86.imageshack.us/img86/7222/bookmark8rf.gif for a better idea of what i'm talking about!

Reproducible: Always

Steps to Reproduce:
1. Launch Ff
2. Click "Bookmarks"
3. Note that "Bookmark All Tabs" does not align vertically with the text of your bookmark folders.

Actual Results:  
"Bookmark All Tabs" does not align vertically with the text of your bookmark folders.

Expected Results:  
"Bookmark All Tabs" does align vertically with the text of your bookmark folders.

Please see http://img86.imageshack.us/img86/7222/bookmark8rf.gif
Attached image screensot comment #0 (obsolete) —
Attached image Screenshot 2 (obsolete) —
Another example of the issue.  It's out of alignment by four pixels and it's the same for the "Open x" and "Open in Tabs".
Keywords: regression
Attached image Screenshot + 3 pixels
I think also that it would look better with some pixels more space to the left.
For WinXP default theme it is 3 pixels however, also without ClearType.
Assignee: nobody → annie.sullivan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → Firefox 2 beta1
Flags: blocking1.9a2?
Don't care about this anymore.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → NEW
Flags: blocking1.9a2?
mass re-assign of ben's, pam's, annie's, and joe's places bugs.
Assignee: annie.sullivan → nobody
Severity: normal → minor
Priority: P3 → --
Whiteboard: Fx2-parity
Target Milestone: Firefox 2 beta1 → Firefox 3
Version: unspecified → Trunk
Attached image Image showing issue
Attachment #216886 - Attachment is obsolete: true
Keywords: polish
Whiteboard: Fx2-parity
Component: Places → Menus
From Bug 411064, Per Simon Bünzli: Feel free to file a new bug and reference bug 393804 comment #16 for a start.

"Open All in Tabs" is mis-aligned as well by 1px to the right.

~B
The first three attachments (screenshots) are obsolete.

~B
Blocks: 425582
Target Milestone: Firefox 3 → ---
Attachment #216872 - Attachment is obsolete: true
QA Contact: places → menus
Do we think we'll get this resolved before release?

~B
Component: Menus → Theme
QA Contact: menus → theme
Summary: Bookmarks sub-menu items improperly aligned → Menu items improperly aligned when using menuitem-iconic
Kai,

Gavin specifically asked for a new bug in https://bugzilla.mozilla.org/show_bug.cgi?id=393804 that I just re-opened for this exact issue and now you've marked that new bug as a duplicate.  This bug speaks only to the Bookmarks menus and not the View menu.  Perhaps they all use menuitem-iconic.

"Status Bar" and "Full Screen" are not vertically aligned with the rest of
the items under "View" in default "Minefield/Firefox" theme (3.0).  Also, the
"Character Encoding" sub menu also has alignment issues.

Reproducible: Always

Steps to Reproduce:
1) Launch current build of Minefield
2) Click "View" and observe the non-aligned items
3) Click "View-> Character Encoding" and observe the non-aligned items
Attached image Screenshot of issue...
Attached image Screenshot of issue...
(In reply to comment #12)
> Perhaps they all use menuitem-iconic.
> 

Yes.  Hence the title of this bug.  :)  There is an alignment problem in all toolkit iconic menu items, not just the stuff in View or History.
Component: Theme → XUL Widgets
Product: Firefox → Toolkit
QA Contact: theme → xul.widgets
*reads bug 411064*  With respect to the checkbox/radio variants of menuitem-iconic (what you see in the view menu), the hack in bug 411064 had a side effect of making the hack in bug 393804 superfluous (the regression that you speak of).  However, reverting the change that was made in bug 393804 would also affect other variants of menuitem-iconic (so that the problem you reported in the bookmarks menu would go from a +1px misalignment to a -1px misalignment).  IMHO, it's probably best best to keep both issues (i.e., that of the checkbox/radio variant and that of the image variant) together in the same bug, because they are related.
(In reply to comment #16)
> IMHO, it's probably best best to keep both issues (i.e., that
> of the checkbox/radio variant and that of the image variant) together in the
> same bug, because they are related.
Kai,

I agree, I was in a rush and didn't have a whole lot of time to create a new bug and did and then it was closed!  That's all.  

What is the likelihood this will be fixed correctly?

~B

(In reply to comment #17)
> I agree, I was in a rush and didn't have a whole lot of time to create a new
> bug and did and then it was closed!  That's all.  
> 
Sorry.  If it makes you feel better, I've also had bugs that I spent a long time filing that got duped.  :)

> What is the likelihood this will be fixed correctly?
> 
Well, to fix it correctly will require changes along the lines of what Simon suggested in bug 393804 comment 16.  It would also require testing on various Windows styles and font sizes to make sure that it all works correctly.  I'll try to get around to looking at it this evening.

There is also the quick-fix option of tweaking some pixels, but that just perpetuates the hack and would result in an incorrect alignment with anything other than an 8pt font size (and it's because of the fragility of these hacks that there are these alignment regressions in the first place, so I'd personally prefer a correct fix).
Attached file testcase
With the latest trunk build, you'll see that there are four (!) different menu alignments.  Fun stuff.
Assignee: nobody → kliu
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
I've tested this patch on Classic, Luna, Aero, on normal font size, extra-large font size, on 96 DPI, and on 120 DPI.  Did I miss anything?  ;)
Attachment #319925 - Flags: review?(zeniko)
Summary: Menu items improperly aligned when using menuitem-iconic → Menu items are improperly aligned when using iconic menuitems
Comment on attachment 319925 [details] [diff] [review]
patch

Oops, forgot that I should be getting a widget review too since I'm touching nsNativeThemeWin.cpp
Attachment #319925 - Flags: review?(roc)
Argh, accidentally attached an older version of the patch.
Attachment #319925 - Attachment is obsolete: true
Attachment #319962 - Flags: review?(zeniko)
Attachment #319925 - Flags: review?(zeniko)
Attachment #319925 - Flags: review?(roc)
Attachment #319962 - Flags: review?(roc)
(In reply to comment #20)
> I've tested this patch on Classic, Luna, Aero, on normal font size, extra-large
> font size, on 96 DPI, and on 120 DPI.  Did I miss anything?  ;)

Kai,

Did you test with different system fonts and with varying sizes of those non-standard fonts?

~B

(In reply to comment #23)
> Did you test with different system fonts and with varying sizes of those
> non-standard fonts?
> 
Because the spacing in the patch is now set using em's (except when we're using uxtheme, but that's a different story), things are now font-agnostic, and the alignment will be correct regardless of typeface, size, or DPI.**


** Well, not quite.  Because we are mixing together things with variable em-based width (radio, checkbox) and things with fixed px-based width (16x16 images), there is an unavoidable lower-bound below which the 16x16 image-based menu items will fall out of alignment.  FWIW, not even Internet Explorer's menus handle this correctly, and nobody ever uses a UI font smaller than 8pt anyway, so this is really a moot point.
Comment on attachment 319962 [details] [diff] [review]
the correct patch

Looks alright to me, thanks. You'll still need review by a Toolkit peer, though.
Attachment #319962 - Flags: review?(zeniko) → review+
Comment on attachment 319962 [details] [diff] [review]
the correct patch

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

> .menu-text,
> .menu-iconic-text {
>   font-weight: inherit;
>-  padding-left: 2px;
>-  padding-right: 2px;
>+  -moz-margin-start: 2px !important;
>+  -moz-padding-end: 2px;

What's the !important for?
Attachment #319962 - Flags: review+
(In reply to comment #27)
> What's the !important for?
> 

Margins had been set to 0 earlier using !important.  That use of !important, in turn, appears to be the result of the label rules in chrome://global/skin/formatting.css taking precedence.

<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/themes/winstripe/global/menu.css&rev=1.15&mark=73-77#73>
Whiteboard: [has patch][needs widget review]
Comment on attachment 319962 [details] [diff] [review]
the correct patch

low-risk Windows-only patch to correct menu alignment problems
Attachment #319962 - Flags: approval1.9?
Whiteboard: [has patch][needs widget review] → [has patch][has reviews]
Comment on attachment 319962 [details] [diff] [review]
the correct patch

a1.9+=damons
Attachment #319962 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
mozilla/widget/src/windows/nsNativeThemeWin.cpp 	3.127
mozilla/toolkit/themes/winstripe/global/menu.css 	1.16 
Status: ASSIGNED → RESOLVED
Closed: 18 years ago16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: --- → mozilla1.9
Attached image Still off in right-click context menus (obsolete) —
This still is not right in context menus.  Here is the code I use with a 16x16 pixel icon.

#uctb-contextentry { 
   list-style-image: url("chrome://undoclosedtabsbutton/skin/toolbar-button-small-active.png") !important;
   -moz-binding: url("chrome://global/content/bindings/menu.xml#menuitem-iconic") !important;
   -moz-appearance: none !important;
}
(In reply to comment #33)
> This still is not right in context menus.
>
Gecko makes no distinction between a context menu or a regular menu when rendering; the only distinction is the anchoring position when the menu is shown.

>    -moz-appearance: none !important;
>
This is the cause of your problem.  The -moz-appearance for <menuitem>s should not be set to none, as doing so will cause Gecko to bypass some of the metrics set by the widget code.  If you must use "-moz-appearance: none" for <menuitem>s, then it is your responsibility to provide the necessary CSS fallback code to make up for things that would otherwise have been handled by the widget code.  Also, using "-moz-appearance: none" for <menuitem>s on Vista Aero would completely screw up the native menu rendering used for Vista, so this is something that really should never be used <menuitem>s.
Comment on attachment 320210 [details]
Still off in right-click context menus

Thank Kai, that fixed it.  Sorry for the spam then!
Verified fixed on XP default fonts and sizes
Attachment #320210 - Attachment is obsolete: true
Same for me. Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050920 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Blocks: 436779
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: