Menu items are improperly aligned when using iconic menuitems

VERIFIED FIXED in mozilla1.9

Status

()

Toolkit
XUL Widgets
--
minor
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: Bryan, Assigned: Kai Liu)

Tracking

({polish, regression})

Trunk
mozilla1.9
x86
Windows XP
polish, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 2

12 years ago
Created attachment 216886 [details]
Screenshot 2

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".
(Reporter)

Updated

12 years ago
Keywords: regression
Created attachment 216902 [details]
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

Updated

12 years ago
Flags: blocking1.9a2?
(Reporter)

Comment 4

12 years ago
Don't care about this anymore.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → NEW

Updated

11 years ago
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
(Reporter)

Comment 6

10 years ago
Created attachment 316704 [details]
Image showing issue
Attachment #216886 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Keywords: polish
Whiteboard: Fx2-parity
(Reporter)

Updated

10 years ago
Component: Places → Menus
(Reporter)

Comment 7

10 years ago
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
(Reporter)

Comment 8

10 years ago
The first three attachments (screenshots) are obsolete.

~B
(Reporter)

Updated

10 years ago
Blocks: 425582

Updated

10 years ago
Target Milestone: Firefox 3 → ---

Updated

10 years ago
Attachment #216872 - Attachment is obsolete: true
QA Contact: places → menus
(Reporter)

Comment 9

10 years ago
Do we think we'll get this resolved before release?

~B
(Assignee)

Updated

10 years ago
Duplicate of this bug: 431791
(Assignee)

Updated

10 years ago
Component: Menus → Theme
QA Contact: menus → theme
Summary: Bookmarks sub-menu items improperly aligned → Menu items improperly aligned when using menuitem-iconic
(Assignee)

Updated

10 years ago
Duplicate of this bug: 432357
(Reporter)

Comment 12

10 years ago
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
(Reporter)

Comment 13

10 years ago
Created attachment 319488 [details]
Screenshot of issue...
(Reporter)

Comment 14

10 years ago
Created attachment 319489 [details]
Screenshot of issue...
(Assignee)

Comment 15

10 years ago
(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
(Assignee)

Comment 16

10 years ago
*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.
(Reporter)

Comment 17

10 years ago
(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

(Assignee)

Comment 18

10 years ago
(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).
(Assignee)

Comment 19

10 years ago
Created attachment 319924 [details]
testcase

With the latest trunk build, you'll see that there are four (!) different menu alignments.  Fun stuff.
Assignee: nobody → kliu
Status: NEW → ASSIGNED
(Assignee)

Comment 20

10 years ago
Created attachment 319925 [details] [diff] [review]
patch

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)
(Assignee)

Updated

10 years ago
Summary: Menu items improperly aligned when using menuitem-iconic → Menu items are improperly aligned when using iconic menuitems
(Assignee)

Comment 21

10 years ago
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)
(Assignee)

Comment 22

10 years ago
Created attachment 319962 [details] [diff] [review]
the correct patch

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)
(Assignee)

Updated

10 years ago
Attachment #319962 - Flags: review?(roc)
(Reporter)

Comment 23

10 years ago
(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

(Assignee)

Comment 24

10 years ago
(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.
Duplicate of this bug: 432838

Comment 26

10 years ago
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+
(Assignee)

Comment 28

10 years ago
(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>
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs widget review]
(Assignee)

Comment 29

10 years ago
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?
(Assignee)

Updated

10 years ago
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+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
mozilla/widget/src/windows/nsNativeThemeWin.cpp 	3.127
mozilla/toolkit/themes/winstripe/global/menu.css 	1.16 
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: --- → mozilla1.9
Duplicate of this bug: 312781

Comment 33

10 years ago
Created attachment 320210 [details]
Still off in right-click context menus

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;
}
(Assignee)

Comment 34

10 years ago
(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 35

10 years ago
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
(Assignee)

Updated

10 years ago
Blocks: 436779
You need to log in before you can comment on or make changes to this bug.