1419867 - "Fonts for" drop-down is bold

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P4)

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Attachment: screenshot

STR:
1. Go to Preferences.
2. Find font settings.
3. Click Advanced.
4. See the first "Fonts for" drop-down (it's bold, and also misaligned with all the other below).

I haven't checked any other preferences, but I guess more of them may be affected. The cause is in 'font-weight: bold' set to the whole <caption>, which is parent element of the <menulist> (or that the <menulist> is child of the <caption> at all).

mozregression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=13763ec9100053bc33afc138cb2bdabafbf299d9&tochange=66212b8b4315fafb91f0178c39962a995114b7a7

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Hmmpf, so this is creating a caption with way more than a label, that is annoying.

This didn't match because of the <hbox>, but now it does... It's not clear to me why that even uses a caption. Other fields in that form definitely don't. I don't know whether I should just add a rule to go back to the earlier behavior, or just remove the caption... I think I'd go for the second, but Johann, wdyt?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Thanks a lot for the report btw!

Comment 3

[Johann Hofmann [:johannh]]

If the caption is just for styling purposes, I'd take a patch that removes it and retains approximately the same look :)

Comment 4

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #3)
> If the caption is just for styling purposes, I'd take a patch that removes
> it and retains approximately the same look :)

It might also have accessibility implications. Not sure if it does in this case, but worth checking.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

Review commit: https://reviewboard.mozilla.org/r/202554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202554/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Before (left) and after (right) with the proposed patch.

This patch was a very local change. I don't have a strong preference for any of both, but I have a terrible eye for design so... :)

Anyway, here's the comparison without / with patch applied. Let me know if this works for you, or I can try to be more sophisticated.

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8931407 [details]
Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

https://reviewboard.mozilla.org/r/202554/#review207888

Works for me.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8931407 [details]
Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

https://reviewboard.mozilla.org/r/202554/#review207894

::: browser/components/preferences/fonts.xul:45
(Diff revision 1)
>  
>      <!-- Fonts for: [ Language ] -->
>      <groupbox>
> -      <caption>
> -        <hbox align="center">
> +      <hbox>
> +        <caption align="center">
>            <label accesskey="&fonts.accesskey;" control="selectLangs">&fonts.label;</label>

So the caption for the groupbox will now be "Fonts for", which doesn't make sense. Again, I'm not sure how exactly screen readers handle this. Maybe :MarcoZ can shed some light on this.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8931407 [details]
Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202554/diff/1-2/

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8931407 [details]
Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

https://reviewboard.mozilla.org/r/202554/#review209006

::: commit-message-a96f7:10
(Diff revision 2)
> +
> +Instead clean the negatives a bit avoiding :not, and do the (imo) cleaner fix.
> +
> +This makes it look closer to how it looked before my patch, modulo the "Fonts
> +for" text, which now has the same size as the other label in the dialog, which I
> +think is nice.

It's part of the caption so I think it should probably inherit the size from there. More fundamentally, I think the in-content stylesheets overuse the rem unit. :( They should rely on inheritence and use context-sensitive units more often.

::: browser/themes/linux/preferences/in-content/dialog.css:7
(Diff revision 2)
>     - License, v. 2.0. If a copy of the MPL was not distributed with this file,
>     - You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  %include ../../../shared/incontentprefs/dialog.inc.css
>  
> -label:not(.menu-text),
> +label,

Can we remove this?

::: browser/themes/linux/preferences/in-content/preferences.css:14
(Diff revision 2)
>  window * {
>    font-size: 1.11rem;
>  }
>  
> -caption label:not(.dialogTitle) {
> +caption label {
>    font-size: 1.27rem;

Can this selector be changed to just caption?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8931407 [details]
Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

https://reviewboard.mozilla.org/r/202554/#review209006

> It's part of the caption so I think it should probably inherit the size from there. More fundamentally, I think the in-content stylesheets overuse the rem unit. :( They should rely on inheritence and use context-sensitive units more often.

Yeah, I agree with this, but this seems like a bigger change than what I planned...

> Can we remove this?

We cannot, because otherwise the `html *, page *, window *` rule in preferences.css wins, and clobbers the inherited value from the caption.

> Can this selector be changed to just caption?

No, because of the above :/. And removing that selector requires fixup in a couple other places which is a change I'm less confident with.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8931407 [details]
Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

Re-requesting review because of the above, but let me know if you still want me to do other changes and I'm happy to.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8931407 [details]
Bug 1419867: prevent menulist labels from getting bolder when they're under a caption.

https://reviewboard.mozilla.org/r/202554/#review209414

I guess I'll have to clean this up myself in a followup...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea36d9fc714f
prevent menulist labels from getting bolder when they're under a caption. r=dao,johannh

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ea36d9fc714f

Comment 16

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

The dropdown is still misaligned with the other ones below, but it's not bold anymore, only the label is.

Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0 ID:20171130101246

Comment 17

[Dão Gottwald [:dao]]

(In reply to Michal Stanke (Mozilla.cz) [:MikkCZ][:mstanke] (use needinfo) from comment #16)
> The dropdown is still misaligned with the other ones below

It's part of the caption, not part of the grid. It's not supposed to line up with the other fields.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #13)
> Comment on attachment 8931407 [details]
> Bug 1419867: prevent menulist labels from getting bolder when they're under
> a caption.
> 
> https://reviewboard.mozilla.org/r/202554/#review209414
> 
> I guess I'll have to clean this up myself in a followup...

Filed bug 1422100

Comment 19

[Saheda Reza [:Antora]]

 Although it is Linux Platform bug but I have found this bug on Windows & also
reproduced this bug on nightly according to (2017-11-22)


Fixing bug is verified on Latest Beta--
Build ID 	20180215111455
User Agent 	Mozilla/5.0 (Windows NT 6.1; rv:59.0) Gecko/20100101 Firefox/59.0 

After fixing, this issue is Solved for windows .

Tested OS-- Windows7 32bit

Comment 20

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Thank you :Antora for your testing efforts! 
I can also confirm that the initial issue was reproducible on 59.0a1 (2017-11-22)and now is fixed on macOS 10.13.2 and Ubuntu 16.04 x64, using 59.0b11 (20180219114835). Based on comment 19, I will mark this as verified fixed.