Closed Bug 542192 Opened 13 years ago Closed 13 years ago

Restyle Mac menus


(Toolkit :: Themes, defect)

Not set





(Reporter: mstange, Assigned: mstange)



(Keywords: polish)


(2 files, 1 obsolete file)

We need to improve the native appearance of our menus:

 - Correct checkmarks:
     Native menus come in four different font sizes: 9px, 11px, 13px and 14px.
     The three smaller ones are used for <menulist> popups, which inherit the
     menulist's font size. 14px is used for everything else.
     At the moment all sizes were using the same wrong checkmark image.
     Native behavior is to use an image for the 14px menus (different from the
     current image) and the font glyph "CHECK MARK" (✓) for menulist popups.

 - Mixing checkmarks and icons:
     Right now, using an icon for a checkable menu item replaces the checkmark.
     This has resulted in some things using bold fonts for checked items, for
     example the all tabs dropdown, where favicons replace the checkmark.
     Instead menuitems should be able to have both an icon and a checkmark.

 - More correct alignments, especially for menulists with the small 11px font

 - Proper RTL support (bug 492245)
Attached patch v1 (obsolete) — Splinter Review
This implements all of the above. It also replaces our arrow images with one image that is rotated when necessary.
Attachment #423500 - Flags: review?(dao)
Comment on attachment 423500 [details] [diff] [review]

> menu,
> menuitem {
>   -moz-appearance: menuitem;
>   -moz-box-align: center;
>   color: MenuText;
>   font: -moz-pull-down-menu;
>+  font-weight: normal !important;

What's this about?

Please write 0 instead of 0px consistently.
Attached patch v2Splinter Review
(In reply to comment #2)
> (From update of attachment 423500 [details] [diff] [review])
> > menu,
> > menuitem {
> >   -moz-appearance: menuitem;
> >   -moz-box-align: center;
> >   color: MenuText;
> >   font: -moz-pull-down-menu;
> >+  font-weight: normal !important;
> What's this about?

Both FireFTP and Adblock Plus use font-weight: bold on default context menu items. This was a counter measure against that.
But we shouldn't change our CSS because of mistakes in extensions, so I removed the line.

> Please write 0 instead of 0px consistently.

Done. I hope I got them all.
Attachment #423500 - Attachment is obsolete: true
Attachment #423522 - Flags: review?(dao)
Attachment #423500 - Flags: review?(dao)
Comment on attachment 423522 [details] [diff] [review]

>+  /* Undo content/browser/preferences/handlers.css - we don't
>+   * want icon-less labels to line up with the other labels.

Really? Would you mind attaching a screenshot for this?
Comment on attachment 423522 [details] [diff] [review]

> {
>+  height: 16px;
>+  margin: -2px 0;

Seems like you really want margin-top: -2px; margin-bottom: -2px; here.

>+  -moz-margin-end: 5px;
>+  /* Empty icons shouldn't take up room, so we need to compensate
>+   * the 5px margin-end with a negative margin-start.
>+   */
>+  -moz-margin-start: -5px;
> }
Attached image some screenshots
Comment on attachment 423522 [details] [diff] [review]

>-+  skin/classic/global/scrollbox/autorepeat-arrow-dn.gif              (scrollbox/autorepeat-arrow-dn.gif)
>-+  skin/classic/global/scrollbox/autorepeat-arrow-dn-dis.gif          (scrollbox/autorepeat-arrow-dn-dis.gif)
>-+  skin/classic/global/scrollbox/autorepeat-arrow-up.gif              (scrollbox/autorepeat-arrow-up.gif)
>-+  skin/classic/global/scrollbox/autorepeat-arrow-up-dis.gif          (scrollbox/autorepeat-arrow-up-dis.gif)

You didn't actually remove autorepeat-arrow-dn-dis.gif and autorepeat-arrow-up-dis.gif.
Attachment #423522 - Flags: review?(dao) → review+
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Depends on: 643184
Depends on: 661288
You need to log in before you can comment on or make changes to this bug.