349437 - Menulist menuitem font doesn't inherit from the parent anymore

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Stefan [:stefanh]]

Bug 344570 introduced the font "-moz-pull-down-menu" in menulist menuitems. This looks nice if the parent font is set to "menu" (like Firefox pref window).

But you can't use a smaller font in the parent anymore - the default menulist item will then be of a small font and the menuitem labels will have the large "-moz-pull-down-menu" font.

Comment 1

[Stefan [:stefanh]]

Attachment: Testcase with window font set to "message-box"

Comment 2

[Stefan [:stefanh]]

Attachment: Screenshots of menulist in ct window

This actually affects firefox. Look at the menulist in the customize toolbar window (top image). Note also that the menulist is too wide, because of the large font in the menuitems.

I haven't seen any docs at apple that specifies the large system font either. 

See the bottom image for how it would look if the font was inherited. Yes, I'm on 10.3.9 - so don't worry about the distorted checkmark ;-)

Comment 3

[Stefan [:stefanh]]

Attachment: Make the font inherit again

I'm not sure if the -moz-pull-down-menu font belong in a global css file, so I propose we make the font inherit again. The only effect in Firefox pref windows would be that the menulist menuitem font is 1px smaller (same size as the parent).

Comment 4

[Asaf Romano (gone)]

So without this change, comparing to a normal native menu-list, is our menupoup font bigger or is the <menulist> font smaller?

Comment 5

[Stefan [:stefanh]]

(In reply to comment #4)
> So without this change, comparing to a normal native menu-list, is our menupoup
> font bigger or is the <menulist> font smaller?
>

Both are bigger. If you mean menupopup/menulist menuitem font. That is, Apple says "System font" for a "Full-sized Popup-menu" - I'm interpreting that as the standard system font (13px Lucida Grande). Looking at various apps, I think they also use the larger menu font.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 271218 [details] [diff] [review]
Make the font inherit again

need to fix some consumers before doing the switch, as discussed.

Comment 7

[Stefan [:stefanh]]

So... I could add the -moz-pull-down-menu styling in browser/themes/pinstripe/browser/preferences/preferences.css. But fonts.xul only uses styles from toolkit/themes/pinstripe/global. Making fonts.xul using browser's preferences.css changes the layout.

Comment 8

[Stefan [:stefanh]]

Attachment: Make the font inherit and fix prefwindow font

OK, this makes the font inherit again and fixes the pref menuitems in Minefield. We now use the same styling for the advanced fonts sheet since I've forced fonts.xul to use browser's preferences.css. I think this is more consistent with the rest. The color sheet can also be styled in the same way (by including the style sheet in colors.xul and fix the margin).

I'll file (and fix) a follow-up on Thunderbird if this gets through.

Comment 9

[Stefan [:stefanh]]

Attachment: Screenshots of advanced fonts sheet before/after

This is how it looks before/after

Comment 10

[Stefan [:stefanh]]

Attachment: New version, including colors/languages sheets (checked in)

Made a new version, also covers the colors/languages sheets. No real change of the languages sheet, just need the style sheet to get the -moz-pull-down-menu font.

Comment 11

[Stefan [:stefanh]]

Attachment: Screenshot of colors sheet before/after

Beltzner, can you comment on those changes, please (this and attachment #286176 [details])?

Comment 12

[Stefan [:stefanh]]

Attachment: Thunderbird part (checked in)

Thunderbird part (needs toolkit part of attachment #286625 [details] [diff] [review]). With this patch, Thunderbird will have an inherited font in all menulists, except the preferences ones. For example, menuitem font in the account settings "Outgoing Server (SMTP):" menulist will now be the same as the label font, instead of the 14px sized Lucida Grande. This will also make content in the "Composition & Addressing" pane fit again.

Comment 13

[Stefan [:stefanh]]

Comment on attachment 286625 [details] [diff] [review]
New version, including colors/languages sheets (checked in)

Requesting additional review from beltzner, because of the ui change (see screenshots).

Comment 14

[Reed Loden [:reed]]

(In reply to comment #13)
> (From update of attachment 286625 [details] [diff] [review])
> Requesting additional review from beltzner, because of the ui change (see
> screenshots).

Try using ui-review. :)

Comment 15

[Stefan [:stefanh]]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 286625 [details] [diff] [review] [details])
> > Requesting additional review from beltzner, because of the ui change (see
> > screenshots).
> 
> Try using ui-review. :)
> 
 Oh, thanks. Hmm, I didn't noticed that there was a ui-review box.

Comment 16

[Stefan [:stefanh]]

Attachment: Calendar/Sunbird part (checked in to trunk/1.8)

Just attaching the Calendar/Sunbird part...

Comment 17

[Asaf Romano (gone)]

Comment on attachment 286625 [details] [diff] [review]
New version, including colors/languages sheets (checked in)

please re-request review after it's is uir'ed.

Comment 18

[Stefan [:stefanh]]

Comment on attachment 286625 [details] [diff] [review]
New version, including colors/languages sheets (checked in)

beltzner, the things to look at is:

attachment #269239 [details], attachment #286176 [details] and attachment #286626 [details].

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 286625 [details] [diff] [review]
New version, including colors/languages sheets (checked in)

I guess I'm OK with this. Apple seems to be using purely indentation as the way of indicating subordinate options, which would mean that we should be bothering with the keyline, either, but we use that everywhere else so I think we should handle that as a separate issue.

I wouldn't block on this sort of change, but it looks more native than what we have now.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 286625 [details] [diff] [review]
New version, including colors/languages sheets (checked in)

r=mano

Comment 21

[Stefan [:stefanh]]

Comment on attachment 286625 [details] [diff] [review]
New version, including colors/languages sheets (checked in)

Low risk, fixes a few ui-glitches in Firefox and makes our usage of menulist menuitem font more sane.

Comment 22

[Stefan [:stefanh]]

Comment on attachment 286698 [details] [diff] [review]
Thunderbird part (checked in)

Can you please look at this, Phil?

Comment 23

[Stefan [:stefanh]]

Comment on attachment 286734 [details] [diff] [review]
Calendar/Sunbird part (checked in to trunk/1.8)

This patch restores the pref menulist menuitems font-styles that you'll lose in the toolkit part of attachment #286625 [details] [diff] [review].

Comment 24

[Stefan [:stefanh]]

Ah, right. Note that Thunderbird/Sunbird patches atm assumes that attachment #286698 [details] [diff] [review] is applied.

Comment 25

[Stefan [:stefanh]]

Comment on attachment 286625 [details] [diff] [review]
New version, including colors/languages sheets (checked in)

Checking in browser/components/preferences/colors.xul;
/cvsroot/mozilla/browser/components/preferences/colors.xul,v  <--  colors.xul
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/preferences/fonts.xul;
/cvsroot/mozilla/browser/components/preferences/fonts.xul,v  <--  fonts.xul
new revision: 1.19; previous revision: 1.18
done
Checking in browser/components/preferences/languages.xul;
/cvsroot/mozilla/browser/components/preferences/languages.xul,v  <--  languages.xul
new revision: 1.8; previous revision: 1.7
done
Checking in browser/themes/pinstripe/browser/preferences/preferences.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/preferences/preferences.css,v  <--  preferences.css
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/themes/pinstripe/global/menu.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/menu.css,v  <--  menu.css
new revision: 1.13; previous revision: 1.12
done

Comment 26

[Stefan [:stefanh]]

I'm resolving this since the toolkit/FF part is done.

Comment 27

[Michael Büttner (no reviews TFN)]

Comment on attachment 286734 [details] [diff] [review]
Calendar/Sunbird part (checked in to trunk/1.8)

r=mickey.

Comment 28

[Stefan [:stefanh]]

Comment on attachment 286734 [details] [diff] [review]
Calendar/Sunbird part (checked in to trunk/1.8)

Landed this without extra empty line that I accidently added in the patch:
Checking in calendar/base/themes/pinstripe/preferences/preferences.css;
/cvsroot/mozilla/calendar/base/themes/pinstripe/preferences/preferences.css,v  <--  preferences.css
new revision: 1.5; previous revision: 1.4
done

Comment 29

[Phil Ringnalda (:philor)]

Comment on attachment 286698 [details] [diff] [review]
Thunderbird part (checked in)

r=philringnalda, thanks.

Comment 30

[Stefan [:stefanh]]

Comment on attachment 286698 [details] [diff] [review]
Thunderbird part (checked in)

Checking in mail/components/preferences/fonts.xul;
/cvsroot/mozilla/mail/components/preferences/fonts.xul,v  <--  fonts.xul
new revision: 1.13; previous revision: 1.12
done
Checking in mail/components/preferences/receipts.xul;
/cvsroot/mozilla/mail/components/preferences/receipts.xul,v  <--  receipts.xul
new revision: 1.3; previous revision: 1.2
done
Checking in mail/components/preferences/sendoptions.xul;
/cvsroot/mozilla/mail/components/preferences/sendoptions.xul,v  <--  sendoptions.xul
new revision: 1.4; previous revision: 1.3
done
Checking in mail/themes/pinstripe/mail/preferences/preferences.css;
/cvsroot/mozilla/mail/themes/pinstripe/mail/preferences/preferences.css,v  <--  preferences.css
new revision: 1.9; previous revision: 1.8
done

Comment 31

[Simon Paquet [:sipaq]]

Stefan, since we (Calendar project) develop on trunk and 1.8 branch in parallel, would it be okay to add that change to the 1.8 branch as well? Or would that have bad side-effects?

If it is okay, can you please commit the Calendar patch to the 1.8 branch as well. Thanks!

Comment 32

[Stefan [:stefanh]]

Comment on attachment 286734 [details] [diff] [review]
Calendar/Sunbird part (checked in to trunk/1.8)

(In reply to comment #31)
> Stefan, since we (Calendar project) develop on trunk and 1.8 branch in
> parallel, would it be okay to add that change to the 1.8 branch as well? Or
> would that have bad side-effects?
> 
> If it is okay, can you please commit the Calendar patch to the 1.8 branch as
> well. Thanks!

Landed on MOZILLA_1_8_BRANCH (without extra line):

Checking in calendar/base/themes/pinstripe/preferences/preferences.css;
/cvsroot/mozilla/calendar/base/themes/pinstripe/preferences/preferences.css,v  <--  preferences.css
new revision: 1.1.2.3; previous revision: 1.1.2.2
done