Closed Bug 1419867 Opened 3 years ago Closed 3 years ago

"Fonts for" drop-down is bold

Categories

(Firefox :: Preferences, defect, P4)

58 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 59
Tracking Status
firefox58 --- wontfix
firefox59 --- verified

People

(Reporter: mstanke, Assigned: emilio)

References

Details

(Keywords: nightly-community)

Attachments

(3 files)

Attached image 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
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?
Flags: needinfo?(jhofmann)
Thanks a lot for the report btw!
If the caption is just for styling purposes, I'd take a patch that removes it and retains approximately the same look :)
Flags: needinfo?(jhofmann)
(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.
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 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.
Attachment #8931407 - Flags: review?(jhofmann) → review+
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.
Attachment #8931407 - Flags: review?(dao+bmo)
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Priority: -- → P4
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?
Attachment #8931407 - Flags: review?(dao+bmo)
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 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.
Attachment #8931407 - Flags: review?(dao+bmo)
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...
Attachment #8931407 - Flags: review?(dao+bmo) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ea36d9fc714f
prevent menulist labels from getting bolder when they're under a caption. r=dao,johannh
https://hg.mozilla.org/mozilla-central/rev/ea36d9fc714f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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
(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.
(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
QA Whiteboard: [good first verify]
 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
QA Whiteboard: [good first verify] → [testday-20180216]
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.
Status: RESOLVED → VERIFIED
OS: Linux → All
Hardware: x86_64 → All
You need to log in before you can comment on or make changes to this bug.