Closed Bug 1533654 Opened 6 months ago Closed 6 months ago

urlbar changes to use serif font

Categories

(Core :: CSS Parsing and Computation, defect, P2)

67 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 + fixed
firefox68 --- fixed

People

(Reporter: lilydjwg, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

update my firefox nightly

Actual results:

The urlbar uses serif font.

Expected results:

The urlbar should use sans font as before.

mozregression finds the following commit:
55:29.73 INFO: Last good revision: 1ae26ce1cf090db6b0b19ea7d7eccd42373dd5fa
55:29.73 INFO: First bad revision: 0d33ffb859fa4e1ea031c3b2cb3274d71d66a0d5
55:29.73 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1ae26ce1cf090db6b0b19ea7d7eccd42373dd5fa&tochange=0d33ffb859fa4e1ea031c3b2cb3274d71d66a0d5

Flags: needinfo?(emilio)
Blocks: 1215878
Keywords: regression
Assignee: nobody → emilio

Hmm, I cannot repro this. Which locale are you on? Anything different from your setup?

Can you go to about:config and paste all the prefs that start with font.default?

Flags: needinfo?(lilydjwg)

Moving to CSS parsing since it was a CSS parsing change, can't wait to see what's the root cause of this.

Status: UNCONFIRMED → NEW
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Priority: -- → P2
Product: Firefox → Core

[Tracking Requested - why for this release]: It's not clear to me what combination of prefs / language can cause this, but I wouldn't want to ship this regression.

I ran mozregression so there shouldn't be many pref differences.

My locale is zh_CN.UTF-8, nightly is en_US or zh_CN. The prefs you asked for:

font.default.ar sans-serif
font.default.el serif
font.default.he sans-serif
font.default.ja sans-serif
font.default.ko sans-serif
font.default.th sans-serif
font.default.x-armn serif
font.default.x-beng serif
font.default.x-cans serif
font.default.x-cyrillic serif
font.default.x-devanagari serif
font.default.x-ethi serif
font.default.x-geor serif
font.default.x-gujr serif
font.default.x-guru serif
font.default.x-khmr serif
font.default.x-knda serif
font.default.x-math serif
font.default.x-mlym serif
font.default.x-orya serif
font.default.x-sinh serif
font.default.x-tamil serif
font.default.x-telu serif
font.default.x-tibt serif
font.default.x-unicode serif
font.default.x-western serif
font.default.zh-CN sans-serif
font.default.zh-HK sans-serif
font.default.zh-TW sans-serif

I'm on Arch Linux and sure that it reproduces with newer versions but not older versions when my system doesn't change. Also:

$ fc-match serif
DejaVuSerif.ttf: "DejaVu Serif" "Book"
$ fc-match sans
DejaVuSans.ttf: "DejaVu Sans" "Book"

I notice that in about:config, the text is in sans except the input box is in serif. (Both are in sans in older versions.) Input boxes on DuckDuckGo and the new tab page are not affected (in sans as before).

BTW, there is a user in our Arch Linux CN group reporting that his nightly shows Chinese in serif and English in sans in the urlbar, but details are unknown.

Flags: needinfo?(lilydjwg)

This happens for me with the en_US locale too: LANG=en_US.UTF-8 LANGUAGE=en mozregression --launch 2019-3-7.

There are lots of special handling of font everywhere... but after inspecting the code I couldn't really figure out any path which may actually make a difference. (There are many times I thought I found it but turned out not to be the case...)

Ok, so I still haven't been able to reproduce, but I'm moderately sure the problematic rule is:

https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/toolkit/themes/linux/global/textbox.css#31

Now, what I don't understand, is why explicitly inheriting that makes a difference. That's clearly a bug.

What does locale | grep LC_TIME show in your terminal?

Flags: needinfo?(lilydjwg)

It's LC_TIME="zh_CN.UTF-8".

Flags: needinfo?(lilydjwg)

I find that with about:config there is such a rule at chrome://global/skin/in-content/common.css:380

html|input[type="email"], html|input[type="tel"], html|input[type="text"], html|input[type="number"], html|textarea {
font-family: inherit;
font-size: inherit;
padding: 5px 8px;
}

When I disable the "font-family: inherit;" rule, the text becomes in sans. The computed "font-family" style is always "文泉驿正黑" (a Chinese font; this is the second font I set for both serif and sans-serif in fontconfig) but the rendering font is DejaVu Sans / Serif.

I see, that makes sense (re. the disabling the inherit rule). That's because there's an input { font: -moz-field; } in forms.css.

Does opening this test-case as HTML show any sheriff font? If so, which ones?

<!doctype html>
<div id="parent" style="font: message-box">
  Parent family.
  <div id="child-inherit" style="font: -moz-field; font-size: inherit; font-family: inherit;">
    Inherited family.
  </div>
  <div id="child" style="font: -moz-field;">
    Non-inherited family.
  </div>
</div>

I switched my locale to zn_CH (which is actually hard, because I know no chinese at all!) and downloaded a bunch of chinese fonts but still nothing interesting.

(for trying the test-case above if you don't mind, thanks for bearing with me!)

Flags: needinfo?(lilydjwg)

Also, do you know in which package could I download the font you mention in comment 10? Presumably setting that as the default system font in the Gnome settings could display the bug.

And thanks so much for bearing with me on this!

Flags: needinfo?(emilio)

Err, didn't meant to drop the ni?

Flags: needinfo?(emilio)

All text in the testcase are in sans.
The font is in the wqy-zenhei package for Arch Linux.

I find something interesting: when I rename my fonts.conf to make it uneffective, the text in F12 console tab (both the text I type and the output it gives out) is in serif, but with my fonts.conf it's in sans...Without my fonts.conf the default font for sans is 文泉驿正黑 (a sans Chinese font) and for serif is 宋体 (a serif Chinese font). The urlbar is still in sans.

Flags: needinfo?(lilydjwg)

Found it! The culprit is the emojione-color-font package. It contains such a section:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE fontconfig SYSTEM "fonts.dtd">
<fontconfig>
<match target="pattern">
<!-- If the requested font is sans-serif -->
<test qual="any" name="family">
<string>sans-serif</string>
</test>
<!-- Make Bitstream Vera Sans the first result -->
<edit name="family" mode="prepend_first">
<string>Bitstream Vera Sans</string>
</edit>
</match>
</fontconfig>

With the ttf-bitstream-vera package installed this issue reproduces.

Wohoo! With that bit in fonts.confg I can repro the regression, and thus I can look into this. Thank you!

Hi, does it also affect Firefox 66 or only 67?

Flags: needinfo?(lilydjwg)

This affects only recent builds of nightly (e.g. 2019-03-07 is bad and 2019-03-02 is good). In comment 0 I've given the result of mozregression.

Flags: needinfo?(lilydjwg)

I've tried firefox-beta 66.0b13 (build in 2019-03-05) and it's not affected.

Thank you, good to know that our upcomig release is not affected!

This is the low-risk fix for this issue, that we should get into 67.

What's going on here is that font-family is tracked via the font list, from the
POV of the style system the font list is generally just the
RefPtr<SharedFontList>, but in Gecko there's also mDefaultGenericId.

The way we end up with the right mDefaultGenericId is fishy at best, bogus at
worst. I left various fixmes over time related to a bunch of this code.

After my patch, we end up with a mDefaultGenericId of serif, rather than the
right one (none).

The parent font always has none because nsLayoutUtils::ComputeSystemFont always
sets it to none if the font is known.

Before my patch, PrefillDefaultForGeneric with aGenericId of none (from the
parent), which makes it the default generic id for the current language, serif
in this case.

Before my optimization, apply_declaration_ignoring_phase called
copy_font_family_from, which resets both the font list and the default
generic.

This patch achieves the same effect by not having the first mutation in the
first place.

This code is still terribly fishy in any case, all the _skip_font_family stuff
is just ridiculous. I'll try to clean up a bit after this, but for 68.

PSA: Sean, just wanted to confirm that you're cool with landing this one-liner regression-fix during the soft-freeze.

Alternative is backing out the optimization introduced in bug 1215878, but given how simple the fix is I don't think it's worth it.

Flags: needinfo?(emilio) → needinfo?(svoisen)

I would actually suggest we back out that optimization for now and give it more time to test in the next cycle. This bug is a good example that the optimization may not be as trivial as it looks like.

Sure, I'm ok with that as well, but then I'd prefer waiting until the merge and uplift that backout instead, rather than backing out on trunk just to reland it a few days later.

Blocks: 1534494

The plan in comment 26 makes sense if you're OK with it: uplift the backout to 67 in the beta cycle.

@emilio: Just confirming that the backout doesn't change the fact that revert will continue to ride in 67, correct?

Flags: needinfo?(svoisen) → needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)

Sure, I'm ok with that as well, but then I'd prefer waiting until the merge and uplift that backout instead, rather than backing out on trunk just to reland it a few days later.

That sounds good to me.

Yeah, that's right.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/299e74655a6b
Don't call PrefillDefaultForGeneric when inheriting font-family. r=heycam

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1215878
  • User impact if declined: With some font configurations where system fonts can cause fallback, you may end up using the wrong default generic font.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Reverts an optimization that had this side-effect. We decided to do this for beta.
  • String changes made/needed: none
  • Do you want to request approval of these patches as well?: on
Attachment #9051726 - Flags: approval-mozilla-beta?
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Comment on attachment 9051726 [details] [diff] [review]
Back out the second patch of bug 1215878 from beta for causing this bug.

Minimal patch reverting an optimization that caused a regression in our primary UI. Uplift approved for 67 beta 4. Thanks!
Attachment #9051726 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.