Closed
Bug 1247214
Opened 9 years ago
Closed 9 years ago
Context menus in about:preferences have wonky fonts
Categories
(Firefox :: Theme, defect, P5)
Firefox
Theme
Tracking
()
VERIFIED
FIXED
Firefox 48
People
(Reporter: Gijs, Assigned: ktbee, Mentored)
References
Details
(Whiteboard: [outreachy-12] [bugday-20160727])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Comment 1•9 years ago
|
||
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...
Updated•9 years ago
|
Updated•9 years ago
|
Mentor: jaws
Whiteboard: [outreachy-12]
Updated•9 years ago
|
Points: --- → 5
Comment 2•9 years ago
|
||
Katie, this will be a good next bug for you.
Assignee: nobody → kbroida
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Sounds good!
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
(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•9 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 7•9 years ago
|
||
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•9 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 9•9 years ago
|
||
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+
Updated•9 years ago
|
Keywords: checkin-needed
Comment 10•9 years ago
|
||
Keywords: checkin-needed
Comment 11•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 13•8 years ago
|
||
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.
Description
•