1167284 - complete GetStandardFamilyName() for gfx.font_rendering.fontconfig.fontlist.enabled

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Karl Tomlinson (:karlt)]

Currently this depends on LocalizedName, which is not implemented.

https://bugzilla.mozilla.org/show_bug.cgi?id=1056479#c78

Comment 1

[John Daggett (:jtd)]

Attachment: patch, implement localized font family lookup

Use the fontconfig default lang to lookup the localized font family name, if one exists. Tested with several CJK fonts available with vanilla Ubuntu install.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8633905 [details] [diff] [review]
patch, implement localized font family lookup

The name returned by GetStandardFamilyName() "MUST be in the result of
GetFontList()".  This approach doesn't necessarily provide that.  That can be
provided by using FcFontList() or FcFontSetList() without depending on the
details of fontconfig implementation.

Missing from the approach in this patch are approximate language
matching and checking that the names represent the same set of fonts.

We already have an implementation of GetStandardFamilyName (minus FC_SCALABLE
handling) so a complete rewrite is not necessary.  Neither this report nor the
commit message describe reasons for the change in approach.

This is not a frequently used method and so need not be optimized through
compromise of correctness.  I expect the existing method can be optimized
without compromise, but still it provides a place from which to start.

Comment 3

[John Daggett (:jtd)]

Attachment: patch v2 - use FcFontList to determine localized name

Use a FcFontList call to determine the localized family name for a given font family.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8642828 [details] [diff] [review]
patch v2 - use FcFontList to determine localized name

>+        if (family) {
>+            AppendUTF8toUTF16(family, aLocalizedName);
>+            return;

This is still missing the check that the returned name represents the same set
of fonts.  If you are keen to avoid using the existing code for this, then I'd
be happy here with a simple check with gfxPlatformFontList::FindFamily() that
looking up |family| here returns |this|, and we can consider it FindFamily's
responsibility to get this right.

GetStandardFamilyName() should be using gfxPlatformFontList::FindFamily()
instead of using FcConfigSubstitute() because GetStandardFamilyName() is about
actual installed fonts.

>+    // didn't find localized name, use superclass method
>+    gfxFontFamily::LocalizedName(aLocalizedName);

The superclass gfxFontFamily::LocalizedName() says "subclasses should
override" and won't necessarily return a family name in GetFontList().
Instead ensure that GetStandardFamilyName() returns an empty
string.

"If the name doesn't in the system, aFamilyName will be empty string, but not
failed", so the either the implementation should comply with the documentation
or the documentation should be updated.

Comment 5

[John Daggett (:jtd)]

Attachment: patch v3 - use FcFontList to determine localized name

(In reply to Karl Tomlinson (ni?:karlt) from comment #4)

> >+        if (family) {
> >+            AppendUTF8toUTF16(family, aLocalizedName);
> >+            return;
> 
> This is still missing the check that the returned name represents the
> same set of fonts.  If you are keen to avoid using the existing code
> for this, then I'd be happy here with a simple check with
> gfxPlatformFontList::FindFamily() that looking up |family| here
> returns |this|, and we can consider it FindFamily's responsibility to
> get this right.
> 
> GetStandardFamilyName() should be using
> gfxPlatformFontList::FindFamily() instead of using
> FcConfigSubstitute() because GetStandardFamilyName() is about actual
> installed fonts.

I don't quite see how the scenario you describe is possible. The new code explicitly enumerates all localized family names within the patterns for a given family in gfxFcPlatformFontList::AddFontSetFamilies. What's the set of steps such that a localized name extracted from the font patterns of a given font family would somehow map to a different family?

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8644075 [details] [diff] [review]
patch v3 - use FcFontList to determine localized name

(In reply to John Daggett (:jtd) from comment #5)
> I don't quite see how the scenario you describe is possible. The new code
> explicitly enumerates all localized family names within the patterns for a
> given family in gfxFcPlatformFontList::AddFontSetFamilies. What's the set of
> steps such that a localized name extracted from the font patterns of a given
> font family would somehow map to a different family?

Consider the Noto Sans family of fonts as listed in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1165179#c1

The Noto Sans Khmer family is a subset of the Noto Sans family.
For these families, faces in both have the name Noto Sans before Noto Sans
Khmer.

If we can assume that gfxPlatformFontList::FindFamily("Noto Sans Khmer")
should return a family containing only faces that have Noto Sans Khmer as one
of the family names, then the gfxFamily can find the localized name.  I don't
think FindFamily() currently provides this, but I assume that is what it is
meant to do, so we can leave fixing that to another bug.  If not, then the
implementation should be on gfxFcPlatformFontList.

FcFontList() returns a list of patterns, with each element containing a list
of family names.  In the case of GetStandardFamilyName("Noto Sans Khmer"), the
first family name in every element of the list will be Noto Sans.
GetStandardFamilyName() should return an empty string because Noto Sans Khmer
is not in the list of families returned by GetFontList() and Noto Sans is a
different family.

(I think it would actually be better if GetFontList() included Noto Sans
Khmer, in which case GetStandardFamilyName() would return Noto Sans Khmer
instead of the empty string.  However, the UI code needs
GetStandardFamilyName() to return empty string if the family is not one of
those from GetFontList(), so this is how the behavior should be until
GetFontList() changes.  I would recommend minimizing the scope of changes but
focus on those necessary to ship unicode-range without regressions.  This can
be done simply by using an equivalent implementation to gfxFontconfigUtils,
but with FC_SCALABLE support.)

Now consider the possibility of a different family "Foo Sans", where "Foo Sans
Khmer" is a subset, but in this scenario faces in both have the name "Foo Sans
Khmer" before "Foo Sans".

In this case, Foo Sans Khmer is one of the families returned from
GetFontList() and so GetStandardFamilyName("Foo Sans Khmer") should return Foo
Sans Khmer.

>+    gfxFontFamily* family = FindFamily(aFontName);
>     if (family) {
>         family->LocalizedName(aFamilyName);
>+        // check that the localized name maps to the same family
>+        gfxFontFamily* localizedFamily = FindFamily(aFamilyName);
>+        if (family != localizedFamily) {

In the case of GetStandardFamilyName("Foo Sans"), some of the patterns from
FcFontList have Foo Sans as the first name, but some have Foo Sans Khmer as
the first name.  

So this same-family test should be performed for each of the patterns from
FcFontList() until one matches.  Please see the existing implementation in
gfxFontconfigUtils.

I'm not comfortable with using FcConfigSubstitute() through
gfxFcPlatformFontList::FindFamily(), as the purpose is to identify equivalent
names.  FcConfigSubstitute() is unnecessary and has the potential to transform
names.  (Second paragraph of comment 4.)

>+            return false;
>+        }
>         return true;
>     }
> 
>     return false;

"If the name doesn't in the system, aFamilyName will be empty string, but not
failed", so the either the implementation should comply with the return values in the documentation or the documentation should be updated.

Comment 7

[John Daggett (:jtd)]

Attachment: patch v4 - use FcFontList to determine localized name

(In reply to Karl Tomlinson (ni?:karlt) from comment #6)

Revised based on review comments.

> The Noto Sans Khmer family is a subset of the Noto Sans family.
> For these families, faces in both have the name Noto Sans before Noto Sans
> Khmer.

No, all the Noto Sans XXX fonts represent their own distinct families. The initial version of the two Noto Sans Khmer fonts shipped with a nameID 16 family name of "Noto Sans" but this has been corrected and removed in version 1.01. The family name is "Noto Sans Khmer".

I don't think there's really an issue here in terms of the code, simply a font data error that has been since corrected.

I think we should get this landed. If there's a real situation where we still run into problems, we can fix those on follow-on bugs.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8671739 [details] [diff] [review]
patch v4 - use FcFontList to determine localized name

>+    gfxFontFamily* family = FindFamily(aFontName);

I'm not comfortable with using FcConfigSubstitute() through
gfxFcPlatformFontList::FindFamily(), as the purpose is to identify equivalent
names.  FcConfigSubstitute() is unnecessary and has the potential to transform
names.

>+        // check that the localized name maps to the same family
>+        gfxFontFamily* localizedFamily =
>+            gfxPlatformFontList::FindFamily(aFamilyName);
>+        if (family != localizedFamily) {
>+            aFamilyName.Truncate();
>+        }

This same-family test should be performed for each of the patterns from
FcFontList() until one matches.  i.e. LocalizedName() should be checking
that the family name it is considering returning describes the same family.

(In reply to John Daggett (:jtd) from comment #7)
> I think we should get this landed. If there's a real situation where we
> still run into problems, we can fix those on follow-on bugs.

Font faces each have their own family list, so there is nothing to provide
that two intersecting families will be the same family.

There is no need to compromise here as we already have code to detect these
cases.  There is no need to rewrite, but, if you do, please handle the same
cases.

Comment 9

[John Daggett (:jtd)]

Attachment: patch v5 - use FcFontList to determine localized name

> >+    gfxFontFamily* family = FindFamily(aFontName);
> 
> I'm not comfortable with using FcConfigSubstitute() through
> gfxFcPlatformFontList::FindFamily(), as the purpose is to identify
> equivalent names.  FcConfigSubstitute() is unnecessary and has the
> potential to transform names.

Fixed.

> Font faces each have their own family list, so there is nothing to
> provide that two intersecting families will be the same family.
> 
> There is no need to compromise here as we already have code to detect
> these cases.  There is no need to rewrite, but, if you do, please
> handle the same cases.

Karl, I'm struggling to understand what you referring to here. The platform fontlist code *does* have a restriction that any given font maps to one and only one font family. This is generally how fonts are designed, they are organized within a single family, not within overlapping groups. For historical reasons related to GDI, there are fonts that ship with one name for GDI and another name for general use (i.e. nameID=1 vs. nameID=16). But the fontconfig API automatically prioritizes the larger grouping (i.e. nameID=16) when present.

Can you give me an example of a font family that doesn't work the same way under the old code vs. the new code? Or describe a scenario where the complex handling in gfxFontconfigUtils::GetStandardFamilyName is solving something not solved by the current code + patch? The Noto Sans Khmer example isn't really valid since the use of 'Noto Sans' as a family name was a font bug that has been fixed.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8676070 [details] [diff] [review]
patch v5 - use FcFontList to determine localized name

(In reply to John Daggett (:jtd) from comment #9)
> The platform
> fontlist code *does* have a restriction that any given font maps to one and
> only one font family.

That does not provide that the a first family from the first font from
FcFontList() is the same family that the fontlist is using.

> This is generally how fonts are designed, they are
> organized within a single family, not within overlapping groups. For
> historical reasons related to GDI, there are fonts that ship with one name
> for GDI and another name for general use (i.e. nameID=1 vs. nameID=16). But
> the fontconfig API automatically prioritizes the larger grouping (i.e.
> nameID=16) when present.

A family of common name ID 16 is typically a superset of a family of common
name ID 21 (WWS Family), but fontconfig puts name ID 21 before name ID 16,
when language sorting doesn't change that.

> Can you give me an example of a font family that doesn't work the same way
> under the old code vs. the new code? Or describe a scenario where the
> complex handling in gfxFontconfigUtils::GetStandardFamilyName is solving
> something not solved by the current code + patch? The Noto Sans Khmer
> example isn't really valid since the use of 'Noto Sans' as a family name was
> a font bug that has been fixed.

The general rule, especially when dealing with factors beyond our control, is
that if there is nothing to guarantee that an assumption is valid, then it is
probably invalid.

The Minion Pro example in https://www.microsoft.com/typography/otspec/name.htm
is one example where the name ID 21/16 situation makes this equivalent to the
Foo Sans Khmer example in comment 6.

For a different kind of issue consider the fact that different name IDs don't
necessarily have the same set of languages.

  Fonts will often have only an "en" family but subfamilies in various
  languages.  Consider, for example, adding Georgia Oblique to the Georgia
  family.  The face may have a subfamily for each of a number of languages, as
  with existing faces in the family.  So as not to confuse GDI, either the
  Oblique or Italic faces need a different name ID 4.  This might be generated
  by combining preferred family with subfamily, which generates name ID 4 in
  several languages.

  FcFontList will put the family with matching language first and it will be
  name ID 4 if there are no name ID 16 entries of the same language.

Another kind of issue may result if/when the next kind of family name id is
added.  Another kind of issue may result from fontconfig rules with similar
goals to those that motivated the Droid Sans situation.

Comment 11

[John Daggett (:jtd)]

Attachment: fc-list for Minion Pro family (installed from Font Folio 11 set)

(In reply to Karl Tomlinson (ni?:karlt) from comment #10)

> > Can you give me an example of a font family that doesn't work the same way
> > under the old code vs. the new code? Or describe a scenario where the
> > complex handling in gfxFontconfigUtils::GetStandardFamilyName is solving
> > something not solved by the current code + patch? The Noto Sans Khmer
> > example isn't really valid since the use of 'Noto Sans' as a family name was
> > a font bug that has been fixed.
> 
> The general rule, especially when dealing with factors beyond our control, is
> that if there is nothing to guarantee that an assumption is valid, then it is
> probably invalid.
> 
> The Minion Pro example in
> https://www.microsoft.com/typography/otspec/name.htm
> is one example where the name ID 21/16 situation makes this equivalent to the
> Foo Sans Khmer example in comment 6.
> 
> For a different kind of issue consider the fact that different name IDs don't
> necessarily have the same set of languages.
> 
>   Fonts will often have only an "en" family but subfamilies in various
>   languages.  Consider, for example, adding Georgia Oblique to the Georgia
>   family.  The face may have a subfamily for each of a number of languages,
> as
>   with existing faces in the family.  So as not to confuse GDI, either the
>   Oblique or Italic faces need a different name ID 4.  This might be
> generated
>   by combining preferred family with subfamily, which generates name ID 4 in
>   several languages.
> 
>   FcFontList will put the family with matching language first and it will be
>   name ID 4 if there are no name ID 16 entries of the same language.
> 
> Another kind of issue may result if/when the next kind of family name id is
> added.  Another kind of issue may result from fontconfig rules with similar
> goals to those that motivated the Droid Sans situation.

Karl, fontconfig is already doing the right thing here, it's prioritizing the "right" family name, the one that groups together the largest set of faces. If you install the slew of fonts in the Minion Pro family the font prefs menu lists *just* "Minion Pro" in *both* the old and new code. There's no issue for this font family.

I think we should land this patch, as it *does* fix a problem, getting the correct name for a font family when localized font names should be displayed. If there are still problems that this fix doesn't cover we should put together a set of steps to reproduce that problem with a real font family and log it as a separate bug.

Comment 12

[Karl Tomlinson (:karlt)]

I don't know what the question is.

You have proposed changing the algorithm that we ship for doing this.
I don't think that change is for the better.

(In reply to John Daggett (:jtd) from comment #11)
> Karl, fontconfig is already doing the right thing here, it's prioritizing
> the "right" family name, the one that groups together the largest set of
> faces.

It is not trying to group the largest set of fonts.
It is prioritizing name ID 21:
http://cgit.freedesktop.org/fontconfig/tree/src/fcfreetype.c?id=d162a4a83d6bf2182e288e0bc0b4d3ae2f78f040#n1137

> If you install the slew of fonts in the Minion Pro family the font
> prefs menu lists *just* "Minion Pro" in *both* the old and new code. There's
> no issue for this font family.

Are you sure you have a font version with name ID 21 and fontconfig compiled
against freetype headers with TT_NAME_ID_WWS_FAMILY?

> I think we should land this patch, as it *does* fix a problem, getting the
> correct name for a font family when localized font names should be
> displayed.

It only fixes that problem because you refused to keep the implementation of
GetStandardFamilyName() and wanted to land a stub replacement in bug
1056479 for nightly users.  After being pestered to allow something to land, I
conceded to this being addressed in a follow-up.  This bug is that follow-up
and the patch here is not suitable for replacing the code you want to replace.

Comment 13

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #12)
> I don't know what the question is.
> 
> You have proposed changing the algorithm that we ship for doing this.
> I don't think that change is for the better.

Yes, it's not existing code but I don't think that's completely relevant. I don't think we have a requirement in Gecko to preserve all features and functionality if it's not really serving a purpose.

So my question is what about this patch is a problem for actual users? What doesn't work correctly?

> > > Karl, fontconfig is already doing the right thing here, it's prioritizing
> > > the "right" family name, the one that groups together the largest set of
> > > faces.
> > 
> > It is not trying to group the largest set of fonts.
> > It is prioritizing name ID 21:
> > http://cgit.freedesktop.org/fontconfig/tree/src/fcfreetype.
> > c?id=d162a4a83d6bf2182e288e0bc0b4d3ae2f78f040#n1137
> > 
> > > If you install the slew of fonts in the Minion Pro family the font
> > > prefs menu lists *just* "Minion Pro" in *both* the old and new code. There's
> > > no issue for this font family.
> > 
> > Are you sure you have a font version with name ID 21 and fontconfig compiled
> > against freetype headers with TT_NAME_ID_WWS_FAMILY?

Yes, you're right, it prioritizes ID 21 > ID 16 > ID 1 family names. But whatever name is prioritized, it will be prioritized the same way, you'll see the same name appearing in both old/new code, no? Are you saying that somehow newer fonts with ID 21 names that are used on platforms with the very latest fontconfig builds will behave differently?

Comment 14

[John Daggett (:jtd)]

I just tested ID 21 names, using the latest version of Fedora and the Sitka font family from Windows 10. These have all flavors of family names, ID 1, 16, 21. Using both the existing gfxPangoFontGroup code and the new fontconfig platform fontlist, the list of pref fonts so the identical ID 21 names:

Sitka Banner
Sitka Display
Sitka Heading
Sitka Small
Sitka Subheading
Sitka Text

The fontconfig version: 2.11.93

fc-list : | grep Sitka output:

/home/jd/.fonts/Sitka.ttc: Sitka Banner,Sitka:style=Regular,Banner
/home/jd/.fonts/SitkaB.ttc: Sitka Display,Sitka:style=Bold,Display Bold
/home/jd/.fonts/Sitka.ttc: Sitka Small,Sitka:style=Regular,Small
/home/jd/.fonts/SitkaZ.ttc: Sitka Subheading,Sitka:style=Bold Italic,Subheading Bold Italic
/home/jd/.fonts/Sitka.ttc: Sitka Subheading,Sitka:style=Regular,Subheading
/home/jd/.fonts/SitkaI.ttc: Sitka Text,Sitka:style=Italic,Text Italic
/home/jd/.fonts/SitkaI.ttc: Sitka Small,Sitka:style=Italic,Small Italic
/home/jd/.fonts/SitkaI.ttc: Sitka Heading,Sitka:style=Italic,Heading Italic
/home/jd/.fonts/SitkaZ.ttc: Sitka Small,Sitka:style=Bold Italic,Small Bold Italic
/home/jd/.fonts/SitkaZ.ttc: Sitka Heading,Sitka:style=Bold Italic,Heading Bold Italic
/home/jd/.fonts/SitkaZ.ttc: Sitka Display,Sitka:style=Bold Italic,Display Bold Italic
/home/jd/.fonts/SitkaB.ttc: Sitka Text,Sitka:style=Bold,Text Bold
/home/jd/.fonts/SitkaB.ttc: Sitka Heading,Sitka:style=Bold,Heading Bold
/home/jd/.fonts/SitkaZ.ttc: Sitka Banner,Sitka:style=Bold Italic,Banner Bold Italic
/home/jd/.fonts/Sitka.ttc: Sitka Heading,Sitka:style=Regular,Heading
/home/jd/.fonts/Sitka.ttc: Sitka Display,Sitka:style=Regular,Display
/home/jd/.fonts/SitkaB.ttc: Sitka Small,Sitka:style=Bold,Small Bold
/home/jd/.fonts/Sitka.ttc: Sitka Text,Sitka:style=Regular,Text
/home/jd/.fonts/SitkaB.ttc: Sitka Subheading,Sitka:style=Bold,Subheading Bold
/home/jd/.fonts/SitkaI.ttc: Sitka Display,Sitka:style=Italic,Display Italic
/home/jd/.fonts/SitkaI.ttc: Sitka Banner,Sitka:style=Italic,Banner Italic
/home/jd/.fonts/SitkaB.ttc: Sitka Banner,Sitka:style=Bold,Banner Bold
/home/jd/.fonts/SitkaZ.ttc: Sitka Text,Sitka:style=Bold Italic,Text Bold Italic
/home/jd/.fonts/SitkaI.ttc: Sitka Subheading,Sitka:style=Italic,Subheading Italic

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #13)
> So my question is what about this patch is a problem for actual users? What
> doesn't work correctly?

Of the examples in comment 6 and comment 10, the Minion Pro example in the
naming table spec is a good one to focus on.

> it prioritizes ID 21 > ID 16 > ID 1 family names. But
> whatever name is prioritized, it will be prioritized the same way, you'll
> see the same name appearing in both old/new code, no?

The implementation of GetStandardFamilyName("Minion Pro") proposed here
returns an empty string if the first pattern from FcFontList has "Minion Pro
Caption" as the first name.  The existing code returns "Minion Pro".

This is equivalent to the GetStandardFamilyName("Foo Sans") example from
comment 6.

> Are you saying that
> somehow newer fonts with ID 21 names that are used on platforms with the
> very latest fontconfig builds will behave differently?

I haven't checked how recent the ID 21 handling in fontconfig is, so I don't
know exactly which versions will demonstrate the problem with Name ID 21.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #14)
> Using both the existing gfxPangoFontGroup code and the new
> fontconfig platform fontlist, the list of pref fonts so the identical ID 21
> names:
> 
> Sitka Banner
> Sitka Display
> Sitka Heading
> Sitka Small
> Sitka Subheading
> Sitka Text

This is an interesting example, but differs from the Minion Pro example in the
spec in that all fonts in the family have an ID 21 name.  All of these fonts
are part of the "Sitka" Preferred/Typographic Family but "Sitka" is never a
"canonical" name and so gfxFcPlatformFontList::AddFontSetFamilies() adds no
"Sitka" family to mFontFamilies.

gfxFcPlatformFontList::GetFontList() and gfxFontconfigUtils::GetFontList() use
a similar algorithm and so I expect the list of fonts available in the
preferences to be the same.

GetStandardFamilyName("Sitka") will probably return "Sitka Banner" (guessing
from the fc-list output) with the implementation proposed here.  The existing
code would return an empty string.  That's not the bug I'm focusing on here
though as that is a bug with the use of AddOtherFamilyName() from
gfxFcPlatformFontList::AddFontSetFamilies().

Here, let's focus on the Minion Pro example, where gfxFcPlatformFontList has a
"Minion Pro" family from the fonts without Name ID 21.

Comment 17

[John Daggett (:jtd)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #15)

> The implementation of GetStandardFamilyName("Minion Pro") proposed here
> returns an empty string if the first pattern from FcFontList has "Minion Pro
> Caption" as the first name.  The existing code returns "Minion Pro".

But under what possible scenario would the first name *ever* be "Minion Pro Caption"? For the Minion Pro family, all patterns have "Minion Pro" as the first name. That's what I'm confused about here, I don't see real world behavior affected by whatever distinction you're making.

In both the old code and new code, only "Minion Pro" ever shows up in the prefs fontlist and only "Minion Pro" is ever returned by GetStandardFamilyName. The name "Minion Pro Caption" never appears anywhere.

I think the right way to think about the different "levels" of family names (i.e. ID 21 > ID 16 > ID 1) is to think of them as historical artifacts rather than as an intended feature allowing family name groups with multiple heirarchies.

If your argument is that content should match ID 1 family names (e.g. "Minion Pro Caption"), that's a bit of a different discussion. On Linux I don't see the need to support that sort of name mapping because the restrictions imposed by GDI have not existed for a while on Linux (at least from my cursory inspection of fontconfig version history). So I think we should limit support to the intended groupings and omit support for name groupings that exist due to history.

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #17)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #15)
> 
> > The implementation of GetStandardFamilyName("Minion Pro") proposed here
> > returns an empty string if the first pattern from FcFontList has "Minion Pro
> > Caption" as the first name.  The existing code returns "Minion Pro".
> 
> But under what possible scenario would the first name *ever* be "Minion Pro
> Caption"? For the Minion Pro family, all patterns have "Minion Pro" as the
> first name.

It depends what you mean by the "Minion Pro" family.

For the gfxFontconfigFontFamily, all patterns have "Minion Pro" as the first
name.

For the "extended, non-WWS family" in the spec example, "Minion Pro Caption"
and "Minion Pro Semibold Italic Caption" have "Minion Pro Caption" as the
first name.  Patterns for these faces are in the FcFontSet from FcFontList()
because the FcFontList() search goes beyond the gfxFontconfigFontFamily and
these faces are part of the "Minion Pro" "extended, non-WWS family".

FcFontSetList() may be helpful here.

> If your argument is that content should match ID 1 family names (e.g.
> "Minion Pro Caption"), that's a bit of a different discussion.

I agree that is a different discussion.

Comment 19

[John Daggett (:jtd)]

Attachment: family names for Minion Pro opticals

I purchased the full optical faces version of Adobe's Minion Pro for testing. Attached is the full set of family name ID's as they exist in these fonts. They correspond to the same names listed in the OpenType spec description of the name table:

https://www.microsoft.com/typography/otspec/name.htm

    Minion Pro Regular:
    Name ID 1: Minion Pro
    Name ID 2: Regular

    Minion Pro Italic:
    Name ID 1: Minion Pro
    Name ID 2: Italic

    Minion Pro Semibold:
    Name ID 1: Minion Pro SmBd
    Name ID 2: Regular
    Name ID 16: Minion Pro
    Name ID 17: Semibold

    Minion Pro Semibold Italic:
    Name ID 1: Minion Pro SmBd
    Name ID 2: Italic
    Name ID 16: Minion Pro
    Name ID 17: Semibold Italic

    Minion Pro Caption:
    Name ID 1: Minion Pro Capt
    Name ID 2: Regular
    Name ID 16: Minion Pro
    Name ID 17: Caption
    Name ID 21: Minion Pro Caption
    Name ID 22: Regular

    Minion Pro Semibold Italic Caption:
    Name ID 1: Minion Pro SmBd Capt
    Name ID 2: Italic
    Name ID 16: Minion Pro
    Name ID 17: Semibold Italic Caption
    Name ID 21: Minion Pro Caption
    Name ID 22: Semibold Italic

Testing with Minion Pro optical fonts installed locally under Ubuntu 14.04, *both* the old and the new code display the same set of Minion Pro families:

  Minion Pro
  Minion Pro Caption
  Minion Pro Display
  Minion Pro Subhead

This makes sense, the faces of the family for captions needs to be separated from the normal text faces, since CSS has no way to distinguish between "regular" faces if all these fonts were lumped under "Minion Pro".

I think unless there's a demonstated problem that the current patch doesn't address, I think we should land this and move on.

Comment 20

[John Daggett (:jtd)]

Please let me know if your results are different from mine for the Minion Pro fonts.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to John Daggett (:jtd) from comment #19)
> Testing with Minion Pro optical fonts installed locally under Ubuntu 14.04,
> *both* the old and the new code display the same set of Minion Pro families:

gfxFcPlatformFontList::GetFontList() and gfxFontconfigUtils::GetFontList() use
a similar algorithm and so I expect the list of fonts available in the
preferences to be the same.

The issue is not with the list of available fonts but with the result of
GetStandardFamilyName("Minion Pro") depending on the order of patterns
returned from GetFontList().  AFAIK we have no guarantees re the ordering of
these patterns, and so what is returned on one system may be different from
what is returned on another.  How can you be sure that the Caption, Display,
or Subhead patterns won't be earlier in the list than plain "Minion Pro"?

Comment 22

[John Daggett (:jtd)]

Attachment: patch v6 - use FcFontList to determine localized name

Ok, now I understand the scenario you're concerned about. If fonts with canonical family names of both "Minion Pro" and "Minion Pro Caption" exist, then FcFontList will return a list for "Minion Pro" that will include the faces from "Minion Pro Caption". Thus, if the ordering of the patterns in the font set happens to have one of those faces as the first face, the code in LocalizedName would return "Minion Pro Caption" for the "Minion Pro" family.

Patch revised to mimic the logic of the code in gfxFontconfigUtils::GetStandardFamilyName, without the leak of candidateFS.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 8682951 [details] [diff] [review]
patch v6 - use FcFontList to determine localized name

(In reply to John Daggett (:jtd) from comment #22)
> without the leak of candidateFS.

Nice, thanks :)

>+    // See if there is a font face with first family equal to the given family.

Please reference GetSystemFontList() or GetFontList() here, as that will help
explain the need for consistency with that list.

Is there a need to have a gfxFontconfigFontFamily::LocalizedName()
implementation?  If not please move this to GetStandardFamilyName and skip the
gfxPlatformFontList::FindFamily() look-ups.

The idea here to was to return the family name passed to
GetStandardFamilyName() if that name was in the list returned by
GetFontList().  I guess LocalizedName() will usually do that, but it is less
clear with the indirection of the FindFamily() look-up and use of the
canonical name often leading to fallback into the FcFontSet equality test.

>+    // didn't find localized name, leave family name blank

If keeping a separate LocalizedName() then please reference GetFontList() and
GetStandardFamilyName() to explain.  Something like "Didn't find localized
name returned by GetFontList() corresponding to this family.  Return blank
name for GetStandardFamilyName()".

>-        return true;
>+        // check that the localized name maps to the same family
>+        gfxFontFamily* localizedFamily =
>+            gfxPlatformFontList::FindFamily(aFamilyName);
>+        if (family != localizedFamily) {
>+            aFamilyName.Truncate();
>+        }

With the new FcFontSet equality test this is now unnecessary right?
If so, please revert this change.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95ae26515feb

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/95ae26515feb

Comment 26

[John Daggett (:jtd)]

Comment on attachment 8682951 [details] [diff] [review]
patch v6 - use FcFontList to determine localized name

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 27

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

John, would it be possible to add automated tests for this "font super families" related changes? It would help catch regressions. I noticed that we also uplifted bug 1160506 this week.

Comment 28

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

Comment on attachment 8682951 [details] [diff] [review]
patch v6 - use FcFontList to determine localized name

Approved. This has been on Nightly for a week now, should be safe to uplift to Aurora44.

Comment 29

[John Daggett (:jtd)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/fb68430090a5

Comment 30

[Carsten Book [:Tomcat]]

(In reply to John Daggett (:jtd) from comment #29)
> Pushed to aurora:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/fb68430090a5

setting flags

Comment 31

[John Daggett (:jtd)]

(In reply to Ritu Kothari (:ritu) from comment #27)
> John, would it be possible to add automated tests for this "font super
> families" related changes? It would help catch regressions. I noticed that
> we also uplifted bug 1160506 this week.

Did you mean to put this comment on this bug? Or bug 1160506?

This bug is about localized family names in the pref fonts list. As such, it requires fonts with a non-English family names and the local locale set to a matching locale. So it can't really be tested except within that environment.

I'll stick a comment on bug 1160506.