fontconfig generic font substitutions are ignored

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kanru, Assigned: jtd)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(5 attachments, 1 obsolete attachment)

Posted file 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
See Also: → 1056479
Whiteboard: gfx-noted
See Also: → 1173826
Does this only happen on nightly? Does developer edition exhibit the same problem?
Flags: needinfo?(kchen)
(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.
Flags: needinfo?(kchen)
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.
Summary: Font fallback preferences in fontconfig is ignored → fontconfig generic font substitutions are ignored
Depends on: 1182361
Blocks: 1180560
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
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.
Attachment #8686578 - Flags: review?(cam)
Assignee: nobody → jdaggett
Maps different subfamilies of Noto Sans CJK to sans-serif for different languages, with Ubuntu used for Latin glyphs.
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.
Pardon me, attached the wrong patch file
Attachment #8686578 - Attachment is obsolete: true
Attachment #8686578 - Flags: review?(cam)
Attachment #8686883 - Flags: review?(cam)
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.
Attachment #8686883 - Flags: review?(cam) → review+
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).
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.
Attachment #8687059 - Flags: review?(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.
Attachment #8687059 - Flags: review?(m_kato) → review+
See Also: → 1224498
Blocks: 1224965
(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
https://hg.mozilla.org/mozilla-central/rev/f77f6580b31c
https://hg.mozilla.org/mozilla-central/rev/236908ce084c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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
Attachment #8686883 - Flags: approval-mozilla-aurora?
Comment on attachment 8687059 [details] [diff] [review]
patch, stub out incorrect textattr test

Approval Request Comment

- same as above -
Attachment #8687059 - Flags: approval-mozilla-aurora?
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.
Attachment #8686883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8687059 [details] [diff] [review]
patch, stub out incorrect textattr test

Aurora44+
Attachment #8687059 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1243226
You need to log in before you can comment on or make changes to this bug.