1247214 - Context menus in about:preferences have wonky fonts

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P5)

Description

[:Gijs (he/him)]

See bug 1105545 and bug 1104076. STR for this one:

1. open the options

Any of:

2a. right click the home page text box
2b. go to security > saved logins, right click the search box

On OS X, the first looks vaguely OK, though I can't shake the feeling it's not quite right. Maybe someone with better eyes for this kind of thing can point out what's wrong. On Windows 8, it seems the context menu doesn't have nearly enough padding, with the focus rectangle on "Select All" overlapping with the last 'l', and the menu generally not being as wide as menus anywhere else (seems there should be a minimum width of some sort?).

2b uses overly large fonts on both OSes. More noticeable on OS X because the actual size of the menu ends up being wrong, but even on Windows we seem to be using a much larger font than we should be.

Philipp, is this something you'd be interested in writing a patch for? :-)

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Oh yeah, this is definitely wrong on Windows (don't have an OS X machine around atm.)
I'm adding it to the QX meta bug.

I'm probably not the best person to write a patch though...

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Katie, this will be a good next bug for you.

Comment 3

[Katie Broida [:ktbee]]

Sounds good!

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Katie, I found the issue with the menu being too narrow. See the margin-start and margin-end rules at http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css?rev=2f9b5a3b2a90#111

Those rules should only be applying to labels inside of the groupbox. Due to the way that the context menu works in the preferences, the menu is a descendant of the groupbox and the menuitems in the menu use labels to display their text.

These two rules are removing the margins from the label and the menu becomes too narrow as a result. One way we could fix this would be to modify the rule so it only applies to labels that are not menuitems, like so:
> xul|groupbox xul|label:not(.menu-accel):not(.menu-text),
> xul|groupbox xul|description {
>   /* !important needed to override toolkit !important rule */
>   -moz-margin-start: 0 !important;
>   -moz-margin-end: 0 !important;
> }

This ruleset was introduced by bug 993369. Richard, since you wrote the patch for bug 993369, how do you think this should get fixed?

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> These two rules are removing the margins from the label and the menu becomes
> too narrow as a result. One way we could fix this would be to modify the
> rule so it only applies to labels that are not menuitems, like so:
> > xul|groupbox xul|label:not(.menu-accel):not(.menu-text),
> > xul|groupbox xul|description {
> >   /* !important needed to override toolkit !important rule */
> >   -moz-margin-start: 0 !important;
> >   -moz-margin-end: 0 !important;
> > }

Yes, that would also be the way I would choose to fix. I would also apply the same to https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/preferences/in-content/dialog.css#7 and https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/preferences/in-content/dialog.css#7 to not increase the menu font size in dialogs.

Comment 6

[Katie Broida [:ktbee]]

Attachment: Prevents context menus from inheriting incorrect styling from preferences pages

Thank you for your guidance on this everyone! I've written a patch based on your suggestions, let me know if I can add anything else as well.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8739974 [details] [diff] [review]
Prevents context menus from inheriting incorrect styling from preferences pages

Review of attachment 8739974 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/windows/preferences/in-content/dialog.css
@@ +4,5 @@
>  
>  %include ../../../shared/incontentprefs/dialog.inc.css
>  
> +label:not(.menu-text),
> +textbox:not(.menu-text),

Why does the :not(.menu-text) need to be applied to the textbox?

@@ +10,5 @@
>  .tab-text,
>  caption > label {
>    font-size: 1.2em;
>  }
> +

nit, please remove this extra line that got added.

::: toolkit/themes/shared/in-content/common.inc.css
@@ +109,2 @@
>  xul|groupbox xul|description {
> +   /* !important needed to override toolkit !important rule */

nit, please remove the trailing space after the comma, and undo the extra space that got added before this comment.

Comment 8

[Katie Broida [:ktbee]]

Attachment: Prevents context menus from inheriting incorrect styling from preferences pages

From looking at the context menu's styling in the Browser Toolbox, I had thought both the toolbox and label styling were increasing the menu text's font-size. However, I after removing "(.menu-text)" from "textbox" and testing my change, I found that we don't need that declaration on toolbox after all. So, I've removed those extra declarations along with the trailing spaces Jared mentioned.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8740100 [details] [diff] [review]
Prevents context menus from inheriting incorrect styling from preferences pages

Review of attachment 8740100 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4eea4fcaf495

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4eea4fcaf495

Comment 13

[Stephanie Daugherty [:sdaugherty]]

Confirming fixed on Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0