Context menus in about:preferences have wonky fonts

VERIFIED FIXED in Firefox 48

Status

()

defect
P5
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: ktbee, Mentored)

Tracking

(Blocks 1 bug)

Trunk
Firefox 48
Points:
5
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 verified)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

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

Comment 3

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

Comment 6

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

Comment 8

3 years ago
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+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4eea4fcaf495
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(Reporter)

Updated

3 years ago
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.