Closed Bug 1104076 Opened 9 years ago Closed 9 years ago

Items in subdialogs dropdowns are huge

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 36

People

(Reporter: u428464, Assigned: Paenglab)

References

Details

(Whiteboard: regression)

Attachments

(3 files, 5 obsolete files)

When I want to select a font in the "advanced" section of "content" the fonts names in the dropdown are huge, much bigger than the current font. I don't see this issue in other preferences dropdowns.
Attached image Fonts dropdown issue
Seems to be related to all in-content subdialogs since the "add languages" dropdown is also affected.
Summary: Fonts names in content/advanced are huge → Items in subdialogs dropdowns are huge
Blocks: 1089814
Blocks: 1062127
No longer blocks: 1089814
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: regression
OS: Windows 7 → All
Attached patch bug1104076.patch (obsolete) — Splinter Review
Reduce the font-size.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8527925 - Flags: review?(dao)
Comment on attachment 8527925 [details] [diff] [review]
bug1104076.patch

The height shouldn't be hardcoded to 25px. Also, I think 1em (rather than 1rem) should do the right thing here. If it doesn't I'd like to know why.
Attachment #8527925 - Flags: review?(dao) → review-
Attached patch bug1104076.patch (obsolete) — Splinter Review
I use now height: 1.5em;. This gives on my PC 24px.
Attachment #8527925 - Attachment is obsolete: true
Attachment #8528433 - Flags: review?(dao)
Comment on attachment 8528433 [details] [diff] [review]
bug1104076.patch

>+menulist > menupopup menu,
>+menulist > menupopup menuitem {

Please use the child selector, e.g. menulist > menupopup > menuitem should work, I think.

I'm not quite sure why this selector is targeting menu elements too. Do we support nested popups for menulists?

>+  height: 1.5em;

Why do you need to specify a height at all? It's not clear to me what you're trying to fix here.
Attached patch bug1104076.patch (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8528433 [details] [diff] [review]
> bug1104076.patch
> 
> >+menulist > menupopup menu,
> >+menulist > menupopup menuitem {
> 
> Please use the child selector, e.g. menulist > menupopup > menuitem should
> work, I think.
> 
> I'm not quite sure why this selector is targeting menu elements too. Do we
> support nested popups for menulists?

Yes they are all simple menulists, so menulist > menupopup > menuitem is enough.

> >+  height: 1.5em;
> 
> Why do you need to specify a height at all? It's not clear to me what you're
> trying to fix here.

Because http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#296 sets 30px and this is to tall with the smaller font-size.
Attachment #8528433 - Attachment is obsolete: true
Attachment #8528433 - Flags: review?(dao)
Attachment #8528507 - Flags: review?(dao)
(In reply to Richard Marti (:Paenglab) from comment #7)
> > >+  height: 1.5em;
> > 
> > Why do you need to specify a height at all? It's not clear to me what you're
> > trying to fix here.
> 
> Because
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> content/common.inc.css#296 sets 30px and this is to tall with the smaller
> font-size.

Seems like we should remove that (maybe replace it with something saner if needed). In fact, isn't font-size: 1.25rem in that rule what's causing this bug in the first place? The hardcoded line-height looks wrong too, by the way.
Attached patch bug1104076.patch (obsolete) — Splinter Review
I changed the height: 30px; to 2em. The taller font-size in the normal in-content pages are from style specs. The specs also want for in-content dialogs smaller fonts and this needs the code for smaller fonts in dialog.css.

I also removed the line-height.
Attachment #8528507 - Attachment is obsolete: true
Attachment #8528507 - Flags: review?(dao)
Attachment #8528530 - Flags: review?(dao)
Comment on attachment 8528530 [details] [diff] [review]
bug1104076.patch

>--- a/toolkit/themes/shared/in-content/common.inc.css
>+++ b/toolkit/themes/shared/in-content/common.inc.css
>@@ -287,18 +287,17 @@ xul|button[type="menu"] > xul|menupopup 
> }
> 
> xul|menulist > xul|menupopup xul|menu,
> xul|menulist > xul|menupopup xul|menuitem,
> xul|button[type="menu"] > xul|menupopup xul|menu,
> xul|button[type="menu"] > xul|menupopup xul|menuitem {
>   -moz-appearance: none;
>   font-size: 1.25rem;
>-  line-height: 22px;
>-  height: 30px;
>+  height: 2em;
>   color: #333;
>   -moz-padding-start: 10px;
>   -moz-padding-end: 30px;
> }

Seems like font-size: 1em should be used here, and then you can stop setting it in dialog.inc.css.
We also shouldn't set a height at all but instead use vertical padding.
Attachment #8528530 - Flags: review?(dao) → review-
Attached patch bug1104076.patch (obsolete) — Splinter Review
Yes, 1em does the thing. I'm now using padding instead of height.
Attachment #8528530 - Attachment is obsolete: true
Attachment #8528551 - Flags: review?(dao)
Would it be possible to use a relative unit for the padding (i.e. em) so that you don't have to override the value in dialog.inc.css?
I don't know if the patch also covers this : but the "sort categorie" fonts in subdialogs are also too big.
Attached patch bug1104076.patchSplinter Review
Patch with relative units padding.
Attachment #8528551 - Attachment is obsolete: true
Attachment #8528551 - Flags: review?(dao)
Attachment #8528580 - Flags: review?(dao)
(In reply to Guillaume C. [:ge3k0s] from comment #13)
> I don't know if the patch also covers this : but the "sort categorie" fonts
> in subdialogs are also too big.

Do you mean the treecols? If yes, are you using a Nightly or build with bug 1087618 applied? If the latter I'm seeing this only with the patch of bug 1087618 applied.
(In reply to Richard Marti (:Paenglab) from comment #15)
> (In reply to Guillaume C. [:ge3k0s] from comment #13)
> > I don't know if the patch also covers this : but the "sort categorie" fonts
> > in subdialogs are also too big.
> 
> Do you mean the treecols? If yes, are you using a Nightly or build with bug
> 1087618 applied? If the latter I'm seeing this only with the patch of bug
> 1087618 applied.

Just to be sure as an example are treecols used to sort by name or by password (when using the passwords subdialogs) ? If so yes that's what I'm talking about. 

I'm using regular Nightly without the patch you mentioned.
Yes, that should be the treecols. I'm not seeing this issue. Maybe file a new bug for this.
Attached image Treecols font
Just to be sure are the treecols fonts supposed to be this size ?
Comment on attachment 8528580 [details] [diff] [review]
bug1104076.patch

Thanks!
Attachment #8528580 - Flags: review?(dao) → review+
Keywords: checkin-needed
(In reply to Guillaume C. [:ge3k0s] from comment #18)
> Created attachment 8529030 [details]
> Treecols font
> 
> Just to be sure are the treecols fonts supposed to be this size ?

Please file a bug for this treecols.
(In reply to Richard Marti (:Paenglab) from comment #20)
> (In reply to Guillaume C. [:ge3k0s] from comment #18)
> > Created attachment 8529030 [details]
> > Treecols font
> > 
> > Just to be sure are the treecols fonts supposed to be this size ?
> 
> Please file a bug for this treecols.

Filed bug 1105254.
https://hg.mozilla.org/mozilla-central/rev/0323dcdc16f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 37.0a1 (buildID: 20141204030201) and latest Aurora 36.0a2 (buildID: 20141204004001).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.