Closed Bug 1247214 Opened 5 years ago Closed 5 years ago

Context menus in about:preferences have wonky fonts

Categories

(Firefox :: Theme, defect, P5)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox47 --- affected
firefox48 --- verified

People

(Reporter: Gijs, Assigned: ktbee, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [outreachy-12] [bugday-20160727])

Attachments

(1 file, 1 obsolete file)

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? :-)
Flags: needinfo?(philipp)
See Also: → 1240439
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...
Blocks: fx-qx
Flags: needinfo?(philipp)
Priority: -- → P5
Blocks: 1248735
No longer blocks: fx-qx
Mentor: jaws
Whiteboard: [outreachy-12]
Points: --- → 5
Katie, this will be a good next bug for you.
Assignee: nobody → kbroida
Status: NEW → ASSIGNED
Sounds good!
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?
Blocks: 993369
Flags: needinfo?(richard.marti)
(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.
Flags: needinfo?(richard.marti)
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.
Attachment #8739974 - Flags: review?(jaws)
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.
Attachment #8739974 - Flags: review?(jaws) → review-
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.
Attachment #8739974 - Attachment is obsolete: true
Attachment #8740100 - Flags: review?(jaws)
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!
Attachment #8740100 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/4eea4fcaf495
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Duplicate of this bug: 1116304
Confirming fixed on Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Status: RESOLVED → VERIFIED
Whiteboard: [outreachy-12] → [outreachy-12] [bugday-20160727]
You need to log in before you can comment on or make changes to this bug.