390901 - CJK - font-name is not recognised correctly in preferences.

Status: VERIFIED FIXED

(Core :: Graphics, defect, P2)

Description

[philippe (part-time)]

In all.js, the font-name for CJK font is set using the locale name (e.g. Japanese). But this name is not recognised correctly in the Preferences > Appearance pane when the user's OS language does not match (e.g. English OS language). The menu-selector shows #GungSeo for Japanese and Chinese.

In other words, the font-name needs to be 'translated' to the user's locale language for the preference pane to recognise/understand it. Gecko itself recognises the font-name correctly.

Example: for Japanese sans-serif, the font-name in all.js (and about:config) is listed as 'ヒラギノ角ゴ Pro'; the Roman name is 'Hiragino Kaku Gothic Pro'.


This affects all users of Gecko (Firefox, Camino, etc).


Follow-up on bug 364786.
See bug 364786 comment 18, bug 364786 comment 20 and bug 364786 comment 76

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

this can fix this bug.

Stuart:

Would you review the gfx part?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This bug can be reproduced on Windows too.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Oops, I forgot an idl file.

Comment 4

[philippe (part-time)]

I tried this patch. It doesn't appear to work here ?
Minefield, with a default profile, I opened the Preferences > Contents > fonts & colors. For the Japanese lang group, the prefs still list #GungSeo.

The same happens with patched Camino build.

(10.4.10 ppc, en-us login language.)

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Is there some reason we're still using Carbon font names for CJK fonts in all.js instead of Cocoa names?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #5)
> Is there some reason we're still using Carbon font names for CJK fonts in
> all.js instead of Cocoa names?

They are really cocoa names in each locales :)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #4)
> I tried this patch. It doesn't appear to work here ?
> Minefield, with a default profile, I opened the Preferences > Contents > fonts
> & colors. For the Japanese lang group, the prefs still list #GungSeo.
> 
> The same happens with patched Camino build.
> 
> (10.4.10 ppc, en-us login language.)

Really??
I cannot reproduce this bug on Win-ja if I use the patch. I'll check on Mac too.

# This patch doesn't fix the camino, they should fix this bug them selves.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 275254 [details] [diff] [review]
Patch rv1.0

er, I forgot that Mac returns postscript name... not family names.

Comment 9

[philippe (part-time)]

(In reply to comment #7)
> (In reply to comment #4)
> > I tried this patch. It doesn't appear to work here ?
> > Minefield, with a default profile, I opened the Preferences > Contents > fonts
> > & colors. For the Japanese lang group, the prefs still list #GungSeo.
> > 
> > The same happens with patched Camino build.
> > 
> > (10.4.10 ppc, en-us login language.)
> 
> Really??
> I cannot reproduce this bug on Win-ja if I use the patch. I'll check on Mac
> too.
> 
> # This patch doesn't fix the camino, they should fix this bug them selves.
> 
Shouldn't Gecko Core provide the needed information to Gecko clients (Firefox, SeaMonkey, Camino, ...) ?

But Camino (an official trunk build) maybe has a bug with its prefs. I tried on a Japanese OS 10.4.10, and Camino was not able to understand 'ヒラギノ角ゴ Pro' (as listed in all.js). It did understand 'Hiragino Kaku Gothic Pro' correctly on both a Japanese and an English system, but not in the 'Advanced' pane of the prefs. I need to test this a bit more.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Because Camino doesn't use toolkit for UI. My patch doesn't have a part for Camino UI. (Of course, Camino can use new API in my patch.)

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

This should work fine on all locales and all platforms :-)

Comment 12

[philippe (part-time)]

(In reply to comment #11)
> Created an attachment (id=275336) [details]
> Patch rv2.0

Minefield Mac: Japanese works OK, but for Simplified Chinese, I still get #GungSeo for 'serif'.
(quick testing only: Minefield currently crashes constantly, but that is unrelated to this patch).

(I'll file bug later for the Camino prefs)

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Did you install "Song"?

all.js:
pref("font.name.serif.zh-CN", "Song");

My system (10.4-Ja) doesn't have this font. I think that we need to change the default pref in new bug...

Comment 14

[philippe (part-time)]

(In reply to comment #13)
> Did you install "Song"?
> 
> all.js:
> pref("font.name.serif.zh-CN", "Song");
> 
> My system (10.4-Ja) doesn't have this font. I think that we need to change the
> default pref in new bug...
> 

Yeah, right, I don't have 'Song' installed. I do have the 'additional fonts' package from the 10.4 installer (with 'STSong': /Library/Fonts/华文宋体.ttf).

But from Apple's technote, there is no 'Song' font for OS X.
http://docs.info.apple.com/article.html?artnum=301332
That selection needs a change, then.

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Does 10.4 not have the "Additional Asian Fonts" package anymore?  That's where Song comes from: http://docs.info.apple.com/article.html?artnum=25710

Comment 16

[philippe (part-time)]

(In reply to comment #15)
All the fonts listed there are included in the 10.4 'additional package', except Song and Fang Song. I don't see any other font package on my Tiger DVD. 

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Stuart:

I repost for your questions here:

The most CJK font has western name and localized name. We use which names in all.js, we need to resolve the given name to localized name if it is necessary.
(On Win and Mac, the localized name is only used the font name locale and the system locale are same, i.e., On Japanese system locale, the Japanese font names are localized, but CK font names are used western font names.)

Therefore, we need a new API for resolving the font family to the actual family name which is in the result of GetFontList.

On Linux, we don't have this bug, but the API should be function able even if this bug doesn't need it. And the code for Linux is used by BeOS and OS/2. I don't know which platforms needs this function, but I think that we don't have any loss by we having the new API on Linux. Of course, the API for Linux works fine on my environment (I have tested with changing the all.js.)

Stuart, we have this bug from oldest build of Mozilla. E.g., on Japanese Windows, we cannot see correct fonts for CK in pref dialogs. But this patch fixes the long term bug.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #16)
> (In reply to comment #15)
> All the fonts listed there are included in the 10.4 'additional package',
> except Song and Fang Song. I don't see any other font package on my Tiger DVD. 

If it's not there in 10.4, we should replace it in our prefs, since going forward there will be more new installs of Mac OS X 10.4+ and fewer upgrades from 10.3 :(

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #18)
> If it's not there in 10.4, we should replace it in our prefs, since going
> forward there will be more new installs of Mac OS X 10.4+ and fewer upgrades
> from 10.3 :(

I can fix this as part of my respin of the patch in bug 299222; what font should replace Song? 

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0.1

updating for latest trunk.

Comment 21

[Stuart Parmenter]

Comment on attachment 286989 [details] [diff] [review]
Patch rv2.0.1

this probably shouldn't use wstrings -- should use some shared string object (nsAStrings?)

i'd really prefer to not add yet another hashtable to the quartz code.  john knows this code better than I do -- is there a better way to do this?

What is the perf impact of this code?

Comment 22

[John Daggett (:jtd)]

Sorry, I don't quite understand the testcase and the expected functionality here.  My guess is that Masayuki and philippe understand the specifics but it would really help to document that a little more I think.  What are the set of steps needed to get into a situation where a name is not "recognised" in the pref panel?  What behavior were you expecting that's not correct?

Running with *either* an en or ja Mac OS X localization, with the system lang set to ja I see Japanese font names but English font names for all other fonts.  Is the problem that in this situation the serif font for Simplified Chinese shows up as #GungSeo rather than STSong?

Seems like you're looking for:

pref family name --> canonical family name --> localized family name

Right?

This area is a bit of a mess because FF2 displays *all* family names, not just the preferred family names (e.g. all the insane Hiragino family names like "Hiragino Kaku Gothic Pro W3", oy vey...).  I think the correct behavior is that any of these family names should map in such a way as to display the same family name as seen in Font Book. 

Font family names in question:

English: #GungSeo
Korean: #궁서체

English: STSong
Simplified Chinese: 华文宋体

Comment 23

[philippe (part-time)]

John,
Run OS X with system language set to English (or French, or whatever Roman language you prefer).
Open Minefield preferences > Content - Fonts & Colors > Advanced, select Japanese, for serif and sans-serif, you'll see #GungSeo.

Expected: Hiragino Kaku Gothic Pro for sans-serif, Hiragino Mincho Pro for serif.

Repeat for other language groups such ** Chinese or Korean.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #22)
> Seems like you're looking for:
> 
> pref family name --> canonical family name --> localized family name
> 
> Right?

What did you mean? pref family name might *not* be canonical family name for the system locale. I.e., you cannot see the Chinese font names in font pref panel. Because they are specified with localized name in all.js, but your locale is not Chinese. Note that the localized names are not related this bug. We only need the way to resolve the font family names from non-canonical to canonical.

> This area is a bit of a mess because FF2 displays *all* family names, not just
> the preferred family names (e.g. all the insane Hiragino family names like
> "Hiragino Kaku Gothic Pro W3", oy vey...).  I think the correct behavior is
> that any of these family names should map in such a way as to display the same
> family name as seen in Font Book. 

It's wrong in a case. FontBook groups variable width fonts and fixed width fonts to one family. However, CSS spec is not so. In CSS spec, the family names decide the text should be rendered by variable width font or fixed width font.

Comment 25

[John Daggett (:jtd)]

(In reply to comment #24)

> What did you mean? pref family name might *not* be canonical family name for
> the system locale. I.e., you cannot see the Chinese font names in font pref
> panel. Because they are specified with localized name in all.js, but your
> locale is not Chinese. 

No, I mean there's logic that shows the family name for the current locale, otherwise uses the canonical name.  Whichever is used, either one should always be resolved correctly since a CSS stylesheet could contain either.

> It's wrong in a case. FontBook groups variable width fonts and fixed width
> fonts to one family. However, CSS spec is not so. In CSS spec, the family names
> decide the text should be rendered by variable width font or fixed width font.

This relates to Osaka?  So you're saying Osaka (variable) and Osaka-Mono (fixed) need to be two different family names?  Other than that, we should display the same way FontBook does, correct?

Webkit code seems to be doing some Osaka-related special casing also:

http://trac.webkit.org/projects/webkit/browser/trunk/WebKit/WebCoreSupport.subproj/WebTextRendererFactory.m?rev=8626#L441

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #25)
> (In reply to comment #24)
> > It's wrong in a case. FontBook groups variable width fonts and fixed width
> > fonts to one family. However, CSS spec is not so. In CSS spec, the family names
> > decide the text should be rendered by variable width font or fixed width font.
> 
> This relates to Osaka?  So you're saying Osaka (variable) and Osaka-Mono
> (fixed) need to be two different family names?  Other than that, we should
> display the same way FontBook does, correct?

Exactly.

> Webkit code seems to be doing some Osaka-related special casing also:
> 
> http://trac.webkit.org/projects/webkit/browser/trunk/WebKit/WebCoreSupport.subproj/WebTextRendererFactory.m?rev=8626#L441

I hate such direct name checking approach :-(

Comment 27

[John Daggett (:jtd)]

I think we need to clean up the code in gfxQuartzFontCache before we go ahead with the changes for this bug.  So I've added a dependency on bug 404310.  Once that's done we can figure out how to make the Mac parts of this patch work.  Hopefully I can get a patch set up for that one early next week.

Comment 28

[John Daggett (:jtd)]

Attachment: list of "family" names returned iterating over ATSFontFamily names

This bug is actually part of a larger problem that affects more than just CJK font family names, with Gecko 1.8 we used a Quickdraw font API and in Gecko 1.9 we're basically using Cocoa/ATSUI font api's and there's a fundamental mismatch between the two.

In Gecko 1.8 the font list was constructed by iterating over ATSFontFamily's and pulling out their names.  This is equivalent to Quickdraw FMFontFamily names, which are grouped in rather strange and interesting ways.  For example, Futura exists along with Futura Condensed.  If you specify Futura Condensed and then use the same profile to start FF3, the Prefs > Content tab > Default field popup displays a blank!

I think the best way to resolve this is to first look up pref fonts using ResolveFontName.  If that fails, look up the name via:

ATSFontFamilyFindFromName ==> ATSFontFamilyRef
FMGetFontFromFontFamilyInstance ==> FMFont (returns a font for a given family)
FMGetATSFontRefFromFont ==> ATSFontRef
ATSFontGetPostScriptName ==> psname
[[NSFont fontWithName:psname size:0.0] familyName] ==> canonical family name

With the changes for 404310, as long as we clean up our prefs fonts, this fallback case should only occur for old prefs where the user explicitly changed a font setting, so I don't see any reason to cache this.

Comment 29

[John Daggett (:jtd)]

Attachment: screenshot, FF3 prefs content tab with Futura Condensed specified in FF2 profile

Comment 30

[John Daggett (:jtd)]

As part of the fix for this we should probably address the Osaka-Mono problem also.  On Japanese systems, Osaka-Mono is used as the default monospace font.  Unlike other monospace fonts, Osaka-Mono has been grouped into the Osaka family (see FontBook under Osaka).  With the transition from using hacky Quickdraw family names in 2.0 to using family names supplied by Cocoa (bug xxx), Osaka-Mono is no longer "reachable", there is no combination of font names and attributes that can specify Osaka-Mono.  The solution is to make a separate font family named Osaka-Mono explicitly.  Not the ideal solution, we'll need to be careful about using system API's with "Osaka-Mono" as a family name, but I think this is one case where historical usage forces us to make an exception.

Comment 31

[John Daggett (:jtd)]

(In reply to comment #30)
> As part of the fix for this we should probably address the Osaka-Mono problem
> also. 

bugzilla jp bug:

http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=6039

The fontlist dump from bug 417444 shows how the various names for Osaka relate:

https://bugzilla.mozilla.org/attachment.cgi?id=303947

(fontinit) family: Osaka, psname: Osaka, face: Regular, apple-weight: 5, css-weight: 4, traits: 0100000c
(fontinit) family: Osaka, psname: Osaka-Mono, face: Regular-Mono, apple-weight: 5, css-weight: 4, traits: 0100040c

Comment 32

[John Daggett (:jtd)]

Attachment: patch for adding localized family names on mac, v.0.1

With this patch:

- GetFontList returns localized names
- mechanism is added for adding single-face names using pref setting "font.single_face_families"

Still need to read in localized names for these single faces, that depends on 417444.

Comment 33

[John Daggett (:jtd)]

Attachment: patch for adding localized family names on mac, v.0.2

Integrates changes checked in for bug 417444.  On a Japanese machine, Japanese prefs will now display correctly (including our dearly beloved, Osaka-Mono).  But because the name is localized in Japanese, on English systems there's a mismatch between the pref setting and the available fonts (i.e. ヒラギノ角ゴ Pro vs. Hiragino Kaku Gothic Pro).

Work to be done:

1. Rework ResolveFontFamilyName on the mac:

FindFamily ==> MacOSFamilyEntry
return entry->LocalizedName()

FindFamily will always find the name, localized or not, so the prefs can use any family name, English or any other language.

The name of this function would be better as ResolveLocalizedFontFamilyName?

2. Instead of within the general getValue method in perferences.xml, the "font.namexxxx" values always seem to be accessed via the _selectDefaultLanguageGroup method of the gContentPane in contents.js.  I didn't quite track down where, XUL JS code is still a bit puzzling to me.  But that's the right place for calling ResolveLocalizedFontFamilyName, if possible.

Comment 34

[John Daggett (:jtd)]

Comment on attachment 304687 [details] [diff] [review]
patch for adding localized family names on mac, v.0.2

mac code checked in under separate bug, bug 419370

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.0

Thank you Josh-san, would you review the mac part?

Comment 36

[John Daggett (:jtd)]

Attachment: additional patch, on lookup miss resolve old QD names

Additional code to do the lookup of old QD names described in comment #28.

Comment 37

[John Daggett (:jtd)]

+  wstring resolveFontFamilyName(in wstring aName);

+     * Resolving a font name to family name. The result MUST be in the result of GetFontList().
+     * If the name doesn't in the system, aFamilyName will be empty string, but not failed.
+     */
+    virtual nsresult ResolveFontFamilyName(const nsAString& aFontName, nsAString& aFamilyName) = 0;

I don't think the meaning of "resolve" is very clear from these names.  What is being resolved?  In this case, the localized name is being fetched, right?  Maybe GetLocalizedFontName?

+                        .getComplexValue(this.name, Components.interfaces.nsISupportsString)
+                        .data;
+              // if this value is font name, we need to resolve it
+              if (this.name.match(/^font\.name\..+/))
+                return FontBuilder.enumerator.resolveFontFamilyName(val);
+              return val;

I don't like the idea of sticking this here, it will affect the performance of looking up *all* preference values, not just font settings.  It really seems like this code belongs in _selectDefaultLanguageGroup method of the gContentPane in contents.js.  I fiddled with this without any luck.  Maybe ask a XUL JS 
expert? (gavin/mconnor?)  Hmm, this code seems to be duped in contents.js / fonts.js.

The Mac version of the code is fine.  I added a separate patch with additional code to handle the lookup of old Quickdraw font family names, since those might be saved away in a profile created with Firefox 2.

To see this problem, use these steps:

1. Start Firefox 2 and create a new profile
2. In Firefox 2, change the default font to "Futura Condensed"
3. Exit
4. Start Firefox 3
5. Open the Prefs > Content tab

Result: font name is blank

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #37)
> Maybe GetLocalizedFontName?

I think that it doesn't make sense. Because if the system locale and the locale of the localized name are not same, the result is not localized name. How about GetStandardFamilyName?

> The Mac version of the code is fine.  I added a separate patch with additional
> code to handle the lookup of old Quickdraw font family names, since those might
> be saved away in a profile created with Firefox 2.

Thank you for the patch. I'll merge it to new patch.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v4.0

Comment 40

[John Daggett (:jtd)]

without a fix for this pref fonts in the Japanese section will always appear as #GungSeo.  i think this needs to at least be wanted1.9+

Comment 41

[John Daggett (:jtd)]

Comment on attachment 306301 [details] [diff] [review]
Patch v4.0

Looks good.

Any idea why we have two versions of the same method in content.js and fonts.js?   Seems like one of these should be consolidated into the other.

Also, someone else should review the Pango changes, I'm clueless about that code currently.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #41)
> Any idea why we have two versions of the same method in content.js and
> fonts.js?   Seems like one of these should be consolidated into the other.

content.js is used on prefs dialog. fonts.js is used on advanced dialog.

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 306301 [details] [diff] [review]
Patch v4.0

r? to kerlt for fontconfig part.

Comment 44

[John Daggett (:jtd)]

(In reply to comment #42)
> (In reply to comment #41)
> > Any idea why we have two versions of the same method in content.js and
> > fonts.js?   Seems like one of these should be consolidated into the other.
> 
> content.js is used on prefs dialog. fonts.js is used on advanced dialog.

Seems like these two versions should be consolidated into a single version that takes (preferences, prefs) as arguments.  Having duped code like this is not such a good thing.

Comment 45

[Karl Tomlinson (:karlt)]

Comment on attachment 306301 [details] [diff] [review]
Patch v4.0

+   * @return the standard family name on the system, if given name does not
+   *         exist, returns empty string

+    if (NS_FAILED(rv) || family.IsEmpty()) {
+        *aResult = nsnull;

I'm not so familiar with idl.  Is PRUnichar*(nsnull) an empty string?

+    result = fs->nfont;
+    if (result != 1)
+        goto end;
+

+        default:
+            // If the given name is alias for multiple fonts, we should return original name.
+            // XXX is there a better way?
+            aFamilyName.Assign(aFontName);
+            return NS_OK;

An example of this situation is:
'fc-list "ヒラギノ明朝 Pro" family' which gives
Hiragino Mincho Pro,ヒラギノ明朝 Pro,Hiragino Mincho Pro W3,ヒラギノ明朝 Pro W3
Hiragino Mincho Pro,ヒラギノ明朝 Pro,Hiragino Mincho Pro W6,ヒラギノ明朝 Pro W6

I think changing the "result != 0" test to "result < 1" and doing
FcPatternGetString on the first font would usually give a better result.

Note also, that "fc-list 'Arial Narrow' family" gives:
Arial,Arial Narrow

I don't think we want to change "Arial Narrow" to "Arial".

But then "Arial Narrow" isn't in the list of fonts.  Maybe
gfxFontconfigUtils::GetFontListInternal should always add the first two
strings in FC_FAMILY property of the pattern, so that both the "font family"
and "preferred font family" are always added.  That wouldn't solve the
GetStandardFamilyName issue if there were localized names too though.  I guess
this is a different bug.

+    CopyUTF8toUTF16(nsDependentCString(family), aFamilyName);

nsDependentCString shouldn't be necessary here as CopyUTF8toUTF16 has a char*
implementation.

Otherwise the fontconfig part looks good.

+    // XXX should return empty string if faild?

"string on failure" is better than "string if faild".
I haven't studied the logic here but my guess is that returning aName is
safer against the risk of overwriting the user's preference.

Are the user's preferences retained if they don't explicitly select a font, or
do they get changed to the StandardFamilyName if they modify something else in
the dialog?

Comment 46

[Karl Tomlinson (:karlt)]

(In reply to comment #45)
> I think changing the "result != 0" test to "result < 1" and doing
> FcPatternGetString on the first font would usually give a better result.

I mean "result != 1" to "result < 1".

Comment 47

[Karl Tomlinson (:karlt)]

Have you considered adding the current preference string to the list of available families (when not already present) instead of trying to work out which family in the list is the same font as the preference string?

This would solve the "Arial Narrow" problem (and might make GetStandardFamilyName unnecessary).

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #45)
> I don't think we want to change "Arial Narrow" to "Arial".
> 
> But then "Arial Narrow" isn't in the list of fonts.  Maybe
> gfxFontconfigUtils::GetFontListInternal should always add the first two
> strings in FC_FAMILY property of the pattern, so that both the "font family"
> and "preferred font family" are always added.  That wouldn't solve the
> GetStandardFamilyName issue if there were localized names too though.  I guess
> this is a different bug.

Arial Narrow should not an item of the result of GetFontLint. Because the style should be gone from family name, On Mac, jdaggett did so.

> Are the user's preferences retained if they don't explicitly select a font,

Yes.

> do they get changed to the StandardFamilyName if they modify something else in
> the dialog?

Yes.

(In reply to comment #47)
> Have you considered adding the current preference string to the list of
> available families (when not already present) instead of trying to work out
> which family in the list is the same font as the preference string?

No, the current list is same as Fx2. And I don't have any problem reports of the listing result.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v5.0

fix.

(In reply to comment #45)
> (From update of attachment 306301 [details] [diff] [review])
> +   * @return the standard family name on the system, if given name does not
> +   *         exist, returns empty string
> 
> +    if (NS_FAILED(rv) || family.IsEmpty()) {
> +        *aResult = nsnull;
> 
> I'm not so familiar with idl.  Is PRUnichar*(nsnull) an empty string?

I think it is ok. See nsThebesFontEnumerator::GetDefaultFont.

Comment 50

[Karl Tomlinson (:karlt)]

(In reply to comment #48)
> Arial Narrow should not an item of the result of GetFontLint. Because the
> style should be gone from family name, On Mac, jdaggett did so.

I think that depends on what type of style it is.
I've filed bug 420628 on this.

Comment 51

[Karl Tomlinson (:karlt)]

Comment on attachment 306759 [details] [diff] [review]
Patch v5.0

+    // The first item of the font list should be preferred font family.

How about the following (but see below)?

"The first family is usually the Preferred Family (see bug 420628)".

> (In reply to comment #47)
> > Have you considered adding the current preference string to the list of
> > available families (when not already present) instead of trying to work out
> > which family in the list is the same font as the preference string?
> 
> No, the current list is same as Fx2. And I don't have any problem reports of
> the listing result.

I was proposing that as a possible solution to this bug, and wondered if it
would be best if the list of available families showed the current value of
the preference exactly.

However, I can see that it is nice to translate the family name into the
current locale.  But it's not so good that this translation may change the
family name to one that actually corresponds to a different set of font faces.

A possible solution could be to consider the first family name a trial
translated name, then do a FcFontList on the trial name to check that it
actually represents the same family of fonts.  (Comparing FC_FILE and FC_INDEX
properties would be the thorough way to compare FcFontSets, but just
checking the number of fonts in the set may usually work.  The next
family in the list that has the same FC_FAMILYLANG is probably the next
candidate, if we want to search that hard.)

(In reply to comment #48)
> > Are the user's preferences retained if they don't explicitly select a font,
>
> Yes.
>
> or, do they get changed to the StandardFamilyName if they modify something
> else in the dialog?
>
> Yes.

I didn't expect "Yes" to both of these questions.  I was hoping that the
user's preference doesn't actually get changed to the approximate translation
unless the user explicitly selects a family.

I tested the patch, and the preference doesn't get changed when the size is
changed for example, which is behavior I like.  However, it is difficult to
actually change the value of the preference to the approximate translation.

If the Preference value for sans-serif is "DejaVu Sans Condensed", then
explicitly selecting "DejaVu Sans" (in the Advanced dialog) doesn't change the
preference.  The only way to change the preference is to select any different
name, then "OK", "Advanced" again, then "DejaVu Sans", and "OK" again.

This issue could be resolved by any of the following

a. showing the current value of the preference exactly

b. using the method suggested above to ensure the translation represents the
   same set of font faces

c. Changing the logic used in detecting whether the user has explicitly
   selected a family.  (I haven't looked into this, so don't know how easily
   this could be done, but I suspect either of the other two would be better.)

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> (In reply to comment #48)
>> > Are the user's preferences retained if they don't explicitly select a font,
>>
>> Yes.
>
> I didn't expect "Yes" to both of these questions.  I was hoping that the
> user's preference doesn't actually get changed to the approximate translation
> unless the user explicitly selects a family.

Wow, sorry. I misunderstand what you said. The actual pref values are not changed without user's modify, probably.

Comment 53

[John Daggett (:jtd)]

(In reply to comment #48)
> > I don't think we want to change "Arial Narrow" to "Arial".
> > 
> > But then "Arial Narrow" isn't in the list of fonts.  Maybe
> > gfxFontconfigUtils::GetFontListInternal should always add the first two
> > strings in FC_FAMILY property of the pattern, so that both the "font family"
> > and "preferred font family" are always added.  That wouldn't solve the
> > GetStandardFamilyName issue if there were localized names too though.  I guess
> > this is a different bug.
> 
> Arial Narrow should not an item of the result of GetFontLint. Because the style
> should be gone from family name, On Mac, jdaggett did so.

This is an artifact of font history.  "Arial Narrow" is technically not a member of the Arial family, it's *family* name is "Arial Narrow"!  It's known craziness, we should not try to invent a scheme that unifies this, that's really a font system problem.

Comment 54

[Karl Tomlinson (:karlt)]

Attachment: ensure that the translated family name represents the same font set as the given family

This patch contains just the gfxFontconfigUtils changes.
It is derived from the gfxFontconfigUtils part of attachment 306759 [details] [diff] [review].
Masayuki, would you like to review this?

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Karl:

Thank you for the patch.

Your patch might return a name that is not contained in the result of GetFontList. So, if the pref has non-existing value, the combobox will be empty (or first font of the drop down list). So, we need also change the GetFontList logic, and ensure that the *all* families are contained to the result, right?

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 307000 [details] [diff] [review]
ensure that the translated family name represents the same font set as the given family

Thank you, Karl. I'll merge your patch in my next patch.

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v6.0

The fontconfig part is merged.

This patch also needs stuart's review before toolkit.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 307274 [details] [diff] [review]
Patch v6.0

mconnor:

Would you review the toolkit part?

Some fonts have both localized name and English name. Which being used on a system depends on the system locale. So, we need to get the used name for each font names at setting the font pref panels. I propose that we add a new pref type which name is "fontname" that returns the actual font name on the system from given name.

Comment 59

[Mike Connor [:mconnor]]

Comment on attachment 307274 [details] [diff] [review]
Patch v6.0

>Index: toolkit/content/widgets/preferences.xml

>+            case "fontname":
>+              var family = this._branch
>+                             .getComplexValue(this.name, Components.interfaces.nsISupportsString)
>+                             .data;

nit: please fix the ugly alignment here

r=mconnor on the toolkit/browser parts

Comment 60

[Karl Tomlinson (:karlt)]

Comment on attachment 307274 [details] [diff] [review]
Patch v6.0

-static PRBool FontNameResolverProcForFaimlyName(const nsAString& aName, void *aClosure)
+static PRBool FontNameResolverProcForFamilyName(const nsAString& aName, void *aClosure)

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v6.1 (checked-in)

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #60)
> (From update of attachment 307274 [details] [diff] [review])
> -static PRBool FontNameResolverProcForFaimlyName(const nsAString& aName, void
> *aClosure)
> +static PRBool FontNameResolverProcForFamilyName(const nsAString& aName, void
> *aClosure)

Thanks, but that is removed from checked-in patch. Because the patch of bug 417014 has same method.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> FIXED

Comment 64

[philippe (part-time)]

verified with
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008031316 Minefield/3.0b5pre

Comment 65

[Ian Neal]

Presumably SeaMonkey needs to be patched too?

Comment 66

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #65)
> Presumably SeaMonkey needs to be patched too?

yes.

Comment 67

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 309078 [details] [diff] [review]
Patch v6.1 (checked-in)

>Index: toolkit/content/widgets/preferences.xml

>+            case "fontname":

>+              return FontBuilder.enumerator.getStandardFamilyName(family);

This makes preferences.xml depend on fontbuilder.js in an non-obvious way (it only happens to work for Firefox because the code that uses the type "fontname" also includes fontbuilder.js). It also looks like you're calling the enumerator's method directly, so the helper you added is unused:

>Index: toolkit/mozapps/preferences/fontbuilder.js

>+  getStandardFamilyName: function (aName) 
>+  {
>+    // XXX should return empty string on failure?
>+    try {
>+      var family = this.enumerator.getStandardFamilyName(aName);