1173260 - fontconfig generic font substitutions are ignored

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: test.html

The system is Debian GNU/Linux 8 Jessie; my personal fontconfig config:

<?xml version="1.0"?>
<!DOCTYPE fontconfig SYSTEM "fonts.dtd">
<fontconfig>
  <match>
    <test name="family"><string>sans-serif</string></test>
    <edit name="family" mode="prepend" binding="strong">
      <string>DejaVu Sans</string>
      <string>Source Han Sans</string>
      <string>WenQuanYi Micro Hei</string>
    </edit>
  </match>
  <match>
    <test name="family"><string>serif</string></test>
    <edit name="family" mode="prepend" binding="strong">
      <string>DejaVu Serif</string>
      <string>AR PL UMing TW</string>
      <string>WenQuanYi Bitmap Song</string>
    </edit>
  </match>
  <match>
    <test name="family"><string>monospace</string></test>
    <edit name="family" mode="prepend" binding="strong">
      <string>DejaVu Sans Mono</string>
      <string>Source Han Sans</string>
      <string>WenQuanYi Micro Hei Mono</string>
    </edit>
  </match>
  <match>
    <test name="family" compare="contains"><string>Source Han Sans</string></test>
    <edit name="family" mode="prepend" binding="strong">
      <string>Source Han Sans TW</string>
      <string>Source Han Sans CN</string>
      <string>Source Han Sans JP</string>
    </edit>
  </match>
</fontconfig>

$ fc-match sans-serif:lang=zh -s
DejaVuSans.ttf: "DejaVu Sans" "Book"
DejaVuSans-Bold.ttf: "DejaVu Sans" "Bold"
DejaVuSans-Oblique.ttf: "DejaVu Sans" "Oblique"
DejaVuSans-BoldOblique.ttf: "DejaVu Sans" "Bold Oblique"
SourceHanSansTW-Medium.otf: "Source Han Sans TW" "Medium"
SourceHanSansCN-Medium.otf: "Source Han Sans CN" "Medium"
SourceHanSansJP-Medium.otf: "Source Han Sans JP" "Medium"
wqy-microhei.ttc: "WenQuanYi Micro Hei" "Regular"

I expect the font selected by default is "Source Han Sans TW", but in Nightly the font selected is still "WenQuanYi Micro Hei"

Probably caused by the reason described in bug 1056479 comment 42

Comment 1

[Mason Chang [Inactive] [:mchang]]

Does this only happen on nightly? Does developer edition exhibit the same problem?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Mason Chang [:mchang] from comment #1)
> Does this only happen on nightly? Does developer edition exhibit the same
> problem?

I tried Developer Edition version 40.0a2 and it doesn't exhibit this problem.

Comment 3

[John Daggett (:jtd)]

This is caused by the limitation that Karl noted in bug 1056479, comment 42, namely that the fontconfig platform fontlist code maps each generic to a single family rather than a set of families. So the multiple substitutions specified fontconfig config file will be ignored. The solution here would be to rejigger the generics handling code to take a list of fonts rather than a single one.

Comment 4

[John Daggett (:jtd)]

The streamlining of generic font lookups (bug 1182361) has landed so we can tackle generic substitutions better by using a derived version of the AddGenericFonts method in gfxFcPlatformFontList:

gfxFcPlatformFontList::AddGenericFonts

  if (<generic, langGroup> ==> serif/sans-serif/monospace)
    GetPrefFontsLang
  else
    gfxPlatformFontList::GetPrefFontsLangGroup


// use fontconfig to do <generic, lang> ==> list of families lookup
gfxFcPlatformFontList::GetPrefFontsLang


gfxFcPlatformFontList::FindFamily

  if ("special generic")
    GetPrefFontsLang

Comment 5

[John Daggett (:jtd)]

Attachment: patch, allow generics to map to multiple font families

Allow for generics to map to a maximum of three font families.
Fontconfig typically substitutes a long laundry list of fonts for
generics like serif and sans-serif. Users can configure substitutions
for a given generic name so allow several font families to be
included when mapping generic font types to actual font families.

For each generic/lang pair, the code looks up the fonts in the pref and
uses fontconfig to look them up if don't exist or indicate "use
fontconfig". So if font.name.serif.ar == "serif", look up fonts via
fontconfig and store it in a hash, such that for each generic/lang pair,
the fonts are only looked up once.

Comment 6

[John Daggett (:jtd)]

Attachment: test fontconfig config file for mapping multiple font families to generics

Maps different subfamilies of Noto Sans CJK to sans-serif for different languages, with Ubuntu used for Latin glyphs.

Comment 7

[John Daggett (:jtd)]

Attachment: testcase, sans-serif use with a mixture of lang tagging

Testcase steps

1. Download Noto Sans CJK super OTC font into ~/.fonts directory
https://noto-website-2.storage.googleapis.com/pkgs/NotoSansCJK.ttc.zip

2. Download attached noto-sans-cjk-latin.conf file. Rename to ~/.fonts.conf

3. Rebuild font caches
sudo fc-cache -rf

4. Run Firefox with the attached testcase.

Result: Latin glyphs are in Ubuntu (compare to the Ubuntu line below) and CJK text shows in Noto Sans CJK.

Comment 8

[John Daggett (:jtd)]

Attachment: patch, allow generics to map to multiple font families (really)

Pardon me, attached the wrong patch file

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8686883 [details] [diff] [review]
patch, allow generics to map to multiple font families (really)

Review of attachment 8686883 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments below addressed.

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +1462,5 @@
> +    // non-default settings exist
> +    NS_ConvertASCIItoUTF16 genericToLookup(generic);
> +    if ((!mAlwaysUseFontconfigGenerics && aLanguage) ||
> +        aLanguage == nsGkAtoms::x_math) {
> +        nsIAtom *langGroup = GetLangGroup(aLanguage);

"*" next to type.

@@ +1478,5 @@
> +                !fontlistValue.EqualsLiteral("sans-serif") &&
> +                !fontlistValue.EqualsLiteral("monospace")) {
> +                usePrefFontList = true;
> +            } else {
> +                // either serif, sans-serif or monospace was specified

Nit: "either" with three items is not correct. :-)

@@ +1494,5 @@
> +
> +    PrefFontList* prefFonts = FindGenericFamilies(genericToLookup, aLanguage);
> +    NS_ASSERTION(prefFonts, "null generic font list");
> +    if (!prefFonts->IsEmpty()) {
> +        aFamilyList.AppendElements(*prefFonts);

No need for the if statement; if prefFonts is empty, the AppendElements call won't add anything.

@@ +1546,5 @@
>      return sCairoFTLibrary;
>  }
>  
> +// a given generic will map to at most this many families
> +const uint32_t kMaxGenericFamilies = 3;

I'm wary of hard coding 3 here, although I appreciate you want to avoid the many families that distributions list.  As discussed on IRC, can you file a followup to make this pref controllable?  Otherwise there might be users who really do want say 4 families (they might be a polyglot!) and would be unable to use them here.

@@ +1617,5 @@
>              NS_ConvertUTF8toUTF16 mappedGenericName(ToCharPtr(mappedGeneric));
> +            gfxFontFamily* genericFamily =
> +                gfxPlatformFontList::FindFamily(mappedGenericName);
> +            if (genericFamily && !prefFonts->Contains(genericFamily)) {
> +                printf("generic %s ==> %s\n", genericLang.get(), (const char*)mappedGeneric);

Don't forget to re-comment this out.

@@ +1630,5 @@
> +    return prefFonts;
> +}
> +
> +void
> +gfxFcPlatformFontList::CheckPrefFontLists()

Maybe use a slightly more descriptive name, such as CheckPrefFontListsForGenerics?  It might even be clearer for this method to just return a bool, and for the caller to do:

  mAlwaysUseFontconfigGenerics = PrefFontListsUseOnlyGenerics();

or something like that.  Up to you.

@@ +1633,5 @@
> +void
> +gfxFcPlatformFontList::CheckPrefFontLists()
> +{
> +    mAlwaysUseFontconfigGenerics = true;
> +    static const char kFontNamePrefix[] = "font.name.";

Can you pull this constant out of the method so that we can use it in AddGenericFonts above, too?

@@ +1642,5 @@
> +    if (NS_SUCCEEDED(rv) && count) {
> +        for (size_t i = 0; i < count; i++) {
> +            // check to see if *any* font pref is set to something other than
> +            // one of the default generic keywords, "serif", "sans-serif", or
> +            // "monospace" (example: font.name.ar.serif ==> "serif")

So this comment sounds a bit misleading as it doesn't check for font.name.ar.serif=sans-serif, for example, and as discussed on IRC this is fine (it's an uncommon case and would just shunt us to the slightly slower path anyway, but still be correct).  Can you mention that case here?  And also the case where you have a generic in a font.name-list pref, which we also don't bother checking?

@@ +1643,5 @@
> +        for (size_t i = 0; i < count; i++) {
> +            // check to see if *any* font pref is set to something other than
> +            // one of the default generic keywords, "serif", "sans-serif", or
> +            // "monospace" (example: font.name.ar.serif ==> "serif")
> +            nsDependentCString prefName(names[i]);

At this point we know that the pref name starts with "font.name.", so we may as well make prefName bound to (names[i] + ArrayLength(kFontNamePrefix) - 1) and skip tokenizing the first two tokens.

@@ +1649,5 @@
> +            for (int a = 0; a < 2; a++) {
> +                if (tokenizer.hasMoreTokens()) {
> +                    tokenizer.nextToken();
> +                }
> +            }

It looks like nextToken() is safe to call even at the end of tokenization, and will just return an empty string, so that might let you simplify the majority of the function to:

  ...
  const nsDependentCSubstring& generic = tokenizer.nextToken();
  const nsDependentCSubstring& langGroup = tokenizer.nextToken();
  nsAdoptingCString fontPrefValue = Preferences::GetCString(names[i]);

  if (!langGroup.EqualsLiteral("x-math") && !generic.Equals(fontPrefValue))) {
    mAlwaysUseFontconfigGenerics = false;
    break;
  }

If the tokenization fails due to the pref name being something like "font.name.foo", then there is still a chance we set mAlwaysUseFontconfigGenerics to false, but as that behaviour's still correct I probably wouldn't bother checking for all the different ways the pref name might be malformed.

(After all, we could also iterate over invalid lang groups or invalid generic names and set mAlwaysUseFontconfigGenerics to false in response.)

@@ +1656,5 @@
> +            if (!tokenizer.hasMoreTokens()) {
> +                continue;
> +            }
> +            const nsDependentCSubstring& genericToken = tokenizer.nextToken();
> +            nsAutoCString generic(genericToken);

Hmm, is it necessary to make a copy of that string?  Does the Equals() call below not work without it?

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +865,5 @@
>                                       nsIAtom* aLanguage,
>                                       nsTArray<gfxFontFamily*>& aFamilyList)
>  {
>      // map lang ==> langGroup
> +    nsIAtom *langGroup = GetLangGroup(aLanguage);

Nit: move "*" next to type while touching this line.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8686883 [details] [diff] [review]
patch, allow generics to map to multiple font families (really)

Review of attachment 8686883 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +1650,5 @@
> +                if (tokenizer.hasMoreTokens()) {
> +                    tokenizer.nextToken();
> +                }
> +            }
> +            NS_ASSERTION(tokenizer.hasMoreTokens(),

NS_WARNING would be better (and below); NS_ASSERTION should be for issues with code, not user supplied data (like the prefs file).

Comment 11

[John Daggett (:jtd)]

Attachment: patch, stub out incorrect textattr test

For generics not defined in a pref (e.g. font.name.cursive.ja), the current code simply returns an empty fontlist, so effectively 'cursive' never maps to anything under Linux unless the user has explicitly added a pref. The patch here will always call through to fontconfig for any generic, including 'cursive' and 'fantasy'. The textattrs code expects the fallback font instead ("DejaVu Serif"), which is explicitly hard-coded in attributes.js (!!!).

There are still failures if the right font on tryserver machines is inserted, so for now I'm just going to conditionalize these tests and file a bug on this.

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8687059 [details] [diff] [review]
patch, stub out incorrect textattr test

Review of attachment 8687059 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/textattrs/test_general.html
@@ +477,5 @@
>  
>        attrs = { "font-family": kAbsentFontFamily };
>        testTextAttrs(ID, 18, attrs, defAttrs, 18, 22);
>  
> +      // xxx - this fails with 'cursive' fontconfig lookup

It is better if adding this bug number in this comment.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f77f6580b31c
https://hg.mozilla.org/integration/mozilla-inbound/rev/236908ce084c

Comment 14

[John Daggett (:jtd)]

(In reply to Cameron McCormack (:heycam) (away Nov 23 – Dec 4) from comment #9)

> >      return sCairoFTLibrary;
> >  }
> >  
> > +// a given generic will map to at most this many families
> > +const uint32_t kMaxGenericFamilies = 3;
> 
> I'm wary of hard coding 3 here, although I appreciate you want to avoid the
> many families that distributions list.  As discussed on IRC, can you file a
> followup to make this pref controllable?  Otherwise there might be users who
> really do want say 4 families (they might be a polyglot!) and would be
> unable to use them here.

Bug 1224965

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f77f6580b31c
https://hg.mozilla.org/mozilla-central/rev/236908ce084c

Comment 16

[John Daggett (:jtd)]

Comment on attachment 8686883 [details] [diff] [review]
patch, allow generics to map to multiple font families (really)

Approval Request Comment
[Feature/regressing bug #]: This is one of the bugs that need to be fixed to enable the new fontconfig back end for beta/release builds. Enabling the new fontconfig back end will allow us to release unicode-range support across all platforms (bugs 1180560, 1119062).
[User impact if declined]: unicode-range support can't be enabled until FF45
[Describe test coverage new/current, TreeHerder]: landed on trunk last week, no issues reported
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 17

[John Daggett (:jtd)]

Comment on attachment 8687059 [details] [diff] [review]
patch, stub out incorrect textattr test

Approval Request Comment

- same as above -

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686883 [details] [diff] [review]
patch, allow generics to map to multiple font families (really)

This has been on Nightly for a few days, seems safe to uplift to Aurora44.

Comment 19

[John Daggett (:jtd)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/14999fc92ea9
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d66f1f8df56

Comment 20

[Carsten Book [:Tomcat]]

(In reply to John Daggett (:jtd) from comment #19)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/14999fc92ea9
> https://hg.mozilla.org/releases/mozilla-aurora/rev/8d66f1f8df56

setting flags

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687059 [details] [diff] [review]
patch, stub out incorrect textattr test

Aurora44+