125777 - The default fonts for non-native system are incorrect

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Yuying Long]

Build: 02-15 trunk build

Steps:
[Case one: native system]
1. Launch browser and go to Edit | preferences | Appearance | Fonts.
2. Check the font for the language that same as OS language, e.g:
   On Japanese windows system I got fonts for Japanese:
   Serif: MS P MeiCho, Sans-Serif & Monospace: MS P Gothic
   Cursive and Fantasy: Fixedsys
   On SimpChinese windows system I got fonts for SimpChinese:
   Serif, Sans-Serif and Monospace: SongTi 
   Cursive and Fantasy: Fixedsys

[Case two: non-native system]
  By doing the same step as in case one on non-native system:
  On WinXP-SimpChinese, fonts for Japanese:
  All fonts are specified as MSGothic even I have all the Japanese fonts
available in my system
  On WinXP-JA, WinME-JA, fonts for SimpChinese:
  All fonts are specified as MSHei.

Seems we can not pick up the right fonts on non-native system.  I assume this
will happen on all platforms.

Comment 1

[Yuying Long]

-> nsbeta1

Comment 2

[Katsuhiko Momoi]

How do we know that non-systems may have all the fonts
that are defaults for the native systems? There will be no
guarantee that all the fonts are available on non-native
systems. Some users may have one font but not others. As I recall,
these font names are hard coded for native systems. They are
predictable because they ship with the fonts. For non-native
systems, hard coding of the names is very difficult. 

Comment 3

[Yuying Long]

Do we have some priority list for each language on non-native system?

like on non-SimpChinese system, we currently use MsHei instead of Song that
cause the page display is not good. 

Comment 4

[Shanjian Li]

take this one from roy, it is related with one of my bug. 

Comment 5

[Shanjian Li]

Attachment: patch

Comment 6

[Shanjian Li]

Using "font.name.serif.zh-CN" as example, we now have a fallback list called
"font.name-list.serif.zh-CN". If the font could not be find in previous
preference, the later name lish should be tried. This logic is implemented in
gfx code, but not in preference dialog code. 

Comment 7

[Shanjian Li]

Naoki/alecf, could you r/sr?

Comment 8

[Katsuhiko Momoi]

shanjian, does your patch refer to any hard-coded list of
fonts? For example, how is the font.name-list.serif.zh-CN
created? Manually or automatically? If automatically, how
is the status of more commonly used fonts reflected in that 
list? Soem more explanation would highly appreciated.

Comment 9

[nhottanscp]

What is the format of "font.name-list.serif.zh-CN"? Does that allow any number
of names to be listed? I am asking because of 'for (;;)' in the patch.

Comment 10

[Shanjian Li]

>> does your patch refer to any hard-coded list of
>>fonts? 
"font.name-list.serif.zh-CN" is specified in the same way as
"font.name.serif.zh-CN". For windows, that's in "winpref.js". And yes, fonts for
each language group are hardcoded in that file. The most direct reason to
implement that fall back list is because on localized CJK system, some fonts use
localized name. This mechanism enabled us to pick up the best available default
font for certain lanuage group.

>>What is the format of "font.name-list.serif.zh-CN"? Does that allow any number
>>of names to be listed? I am asking because of 'for (;;)' in the patch.
The format is the same as css font specification. It is coma separated name
list. That's why I have to implement it with a loop. Following is an line taken
from winpref.js:
pref("font.name-list.monospace.ja", "MS Gothic, MS Mincho, MS PGothic, MS PMincho");

Comment 11

[nhottanscp]

What does "nameList.length" have? In your example would that be four? If so, why
isn't that used to control the loop?

Comment 12

[Shanjian Li]

The "nameList" is a string contains coma separated names, it is not a list data
structure itself. Its length is string length. 

Comment 13

[nhottanscp]

Question:
In your example, which one is supposed to be selected, "MS Gothic" or "MS
PMincho"? My assumption is "MS Gothic" because "MS PMincho" is propotional.

pref("font.name-list.monospace.ja", "MS Gothic, MS Mincho, MS PGothic, MS PMincho");

Comment 14

[Katsuhiko Momoi]

Can you write down specs for the behavior in non-programming terms?
Font display has important implication for users and we don't want
to make changes without that spec understood completely.

Comment 15

[Katsuhiko Momoi]

> "font.name-list.serif.zh-CN" is specified in the same way as
> "font.name.serif.zh-CN". For windows, that's in "winpref.js". 
> And yes, fonts for each language group are hardcoded in that file.

Does the hard-coded list include CJK fonts from Windows XP & Win 2000,
the newer platforms which may have added new fonts for these languages?

Comment 16

[Frank Tang]

this is a low priority bug than others. 
it is a not a all bug bug a "all windows" bug. 
nsbeta1+ bug lower priority than others. 
also, before check in code , verify the patch with other paltforms. (mac, linux)
and do no harm there.

Comment 17

[Alec Flett]

Comment on attachment 70165 [details] [diff] [review]
patch

you should be using split(), not doing hand-parsing

it looks something like:
fontlist = nameList.split(/, /);
for (i=0; i<fontList.length; i++) {

  dataEls = selectElement.listElement.getElementsByAttribute("value",
fontList[i]);
....
}
You'll have to look up the exact syntax

Comment 18

[Shanjian Li]

alecf, I tried "split", but it does not work very well. Using following example
for explaination, 
pref("font.name-list.monospace.zh-CN", "MS Hei,  MS Song");    
Since space character " " should be kept inside a font name, it should not be
used as a separator by itself. We can use ", " either because we don't know if
there is space after coma or not, and how many space will be there. It should
work by using split(",") and then truncate space later, but I thought that might
not be optimal because it will lead to extra string copy and possibily memory
allocation. 

Comment 19

[nhottanscp]

Question:
In your example, which one is supposed to be selected, "MS Gothic" or "MS
PMincho"? My assumption is "MS Gothic" because "MS PMincho" is propotional.

pref("font.name-list.monospace.ja", "MS Gothic, MS Mincho, MS PGothic, MS PMincho");

Comment 20

[Shanjian Li]

We try them in the order of list. If "MS Gothic" is available in the system, it
will be selected, otherwise "MS Mincho, MS PGothic, MS PMincho" will be tried in
order until we find one available. If none of the listed fonts are available,
the first font in the system which declares to support that language group will
be selected. 

Comment 21

[nhottanscp]

Comment on attachment 70165 [details] [diff] [review]
patch

r=nhotta

Could you rename 'index' and 'lastIndex' to indeicate that they are character
index instead of index to strings?

Comment 22

[Shanjian Li]

I don't have any strings to index, it shouldn't cause any confusion. 

Comment 23

[Alec Flett]

Comment on attachment 70165 [details] [diff] [review]
patch

oh, sorry I was trying to research this and I had some simpler solutions. I had
been thinking of perl when I put the regular expression as a parameter to
split(). Anyway, it's just one more line to do the whitespace stripping:


var stripWhitespace = /^\s*(.*)\s*$/;
var fontNames = nameList.split(",");
for (font in fontNames) {
  selectVal = font.replace(stripWhitespace,"$1");

}

you might have to tweak that slightly but thats the basic idea - much simpler!

Comment 24

[Shanjian Li]

Attachment: new patch as suggested by alecf

Comment 25

[Shanjian Li]

It is good to know there is such trick to strip space characters. Is there any
concern about the speed as regular expression is involved? It shouldn't be a
problem here, but I just like to know. 

Comment 26

[Shanjian Li]

Comment on attachment 70937 [details] [diff] [review]
new patch as suggested by alecf

carry over naoki's review

Comment 27

[Alec Flett]

Comment on attachment 70937 [details] [diff] [review]
new patch as suggested by alecf

much easier to understand. Can you just add some blank lines so everything
isn't all crammed together?
sr=alecf with this added spacing

Comment 28

[Alec Flett]

oh, to answer your question - regular expressions are actually pretty fast -
especially if you compile them in advance like you've done here.. i.e. if you
use them again, avoid this:

for (...) {
  var regex = /.../; // this compiles this regular 
                     // expression every time you go through the loop
}

using replace() minimizes the string copying as well - in the previous patch, JS
would have probably made about 5 copies of the string before finally ending up
with the parsed string list.

Comment 29

[Katsuhiko Momoi]

shanjian, please answer the question in comment #14.

Comment 30

[Shanjian Li]

Attachment: patch added some spacing.

Comment 31

[Shanjian Li]

My patch does change any existing specification. I will try to give more
background imformation here.

Originally, we have item like "font.name.serif.zh-CN" to specify the default
generic font for certain language group. The limitation is that it only allow us
to specify one font name. The same font in localized and non-localized system
might have different names. If the specified font name there can not be found,
mozilla will pick one and that one might not be the best one on system. For
example, mozilla will pick "MS Hei" just because it's alphabetic order is before
"MS Song" on Chinese windows. Without any other hints, we can ask mozilla do
better than that. "font.name-list.serif.zh-CN" was later added to allow user to
specify a list of fonts if the one mentioned before could not be found. 

The problem this bug aimed to fix is to reveal to user this internal logic. Both
of those preference items are predefined in our code. win2k and winxp might have
some new fonts available, but so far we don't have any reason to update our list. 
If one of those new fonts does look superior to our existing default, we could
update our current preference setting. But that should be the concern of a
different bug. 

Comment 32

[Shanjian Li]

"My patch does change any existing specification" should be 
"My patch does NOT change any existing specification"

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 70975 [details] [diff] [review]
patch added some spacing.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 34

[Shanjian Li]

fix checked in to trunk.

Comment 35

[Yuying Long]

Fixed was verified on recently trunk build.