1104076 - Items in subdialogs dropdowns are huge

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[u428464]

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.

Comment 1

[u428464]

Attachment: Fonts dropdown issue

Comment 2

[u428464]

Seems to be related to all in-content subdialogs since the "add languages" dropdown is also affected.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: bug1104076.patch

Reduce the font-size.

Comment 4

[Dão Gottwald [: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.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: bug1104076.patch

I use now height: 1.5em;. This gives on my PC 24px.

Comment 6

[Dão Gottwald [: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.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: bug1104076.patch

(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.

Comment 8

[Dão Gottwald [: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.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: bug1104076.patch

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.

Comment 10

[Dão Gottwald [: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.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: bug1104076.patch

Yes, 1em does the thing. I'm now using padding instead of height.

Comment 12

[Dão Gottwald [: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?

Comment 13

[u428464]

I don't know if the patch also covers this : but the "sort categorie" fonts in subdialogs are also too big.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: bug1104076.patch

Patch with relative units padding.

Comment 15

[Richard Marti (:Paenglab)]

(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.

Comment 16

[u428464]

(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.

Comment 17

[Richard Marti (:Paenglab)]

Yes, that should be the treecols. I'm not seeing this issue. Maybe file a new bug for this.

Comment 18

[u428464]

Attachment: Treecols font

Just to be sure are the treecols fonts supposed to be this size ?

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8528580 [details] [diff] [review]
bug1104076.patch

Thanks!

Comment 20

[Richard Marti (:Paenglab)]

(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.

Comment 21

[u428464]

(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.

Comment 22

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/0323dcdc16f9

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0323dcdc16f9

Comment 25

[Camelia Badau [:cbadau], Desktop Test Engineering]

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).