Closed Bug 1096800 Opened 7 years ago Closed 7 years ago

Default sans-serif font for zh-CN on windows is mapped to SimSun, which is actually a serif font

Categories

(Core :: Layout: Text and Fonts, defect)

36 Branch
x86
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: duanyao.ustc, Assigned: Kwan)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.3; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141106201515

Steps to reproduce:

Open "about:config" on a windows with zh-CN locale, check the value of "font.name.sans-serif.zh-CN".



Actual results:

Default value of "font.name.sans-serif.zh-CN" is "SimSun", which is same as "font.name.serif.zh-CN".

"SimSun"(or "宋体" in Chinese) is a Chinese font which is regarded as serif font, so the default is definitely wrong.

As a result, on firefox on windows with zh-CN locale, texts with "font-family: sans-serif" style are renderred same as "font-family: serif", as shown by the attachment.



Expected results:

"font.name.sans-serif.zh-CN" should be mapped to a sans-serif font by default, such as "SimHei"(or "黑体" in Chinese), or "Microsoft Yahei"(or "微软雅黑" in Chinese). With this setting, texts with "font-family: sans-serif" look correct.
This bug exists at least since FF 29.
IE 11 do it correctly, see the attached screenshots.
Current defaults for Windows zh-CN:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2700

pref("font.name.serif.zh-CN", "SimSun");
pref("font.name.sans-serif.zh-CN", "SimSun");
pref("font.name.monospace.zh-CN", "SimSun");
pref("font.name-list.serif.zh-CN", "MS Song, SimSun, SimSun-ExtB");
pref("font.name-list.sans-serif.zh-CN", "MS Song, SimSun, SimSun-ExtB");
pref("font.name-list.monospace.zh-CN", "MS Song, SimSun, SimSun-ExtB");

Need to figure out the right fonts here, including both for Win7+ and for Win XP.

It would be useful to know:

- what are the ideal sans-serif fonts for WinXP/Win7/Win8?
- what is IE11 using?
- what did XP versions of IE use?

We can then include the best default in the font.name prefs and add the best-case fallbacks to the font.name-list prefs.
Assignee: nobody → jdaggett
Status: UNCONFIRMED → NEW
Ever confirmed: true
IE 11 on windows 8.1 for zh-CN seems using "Microsoft Yahei"(or "微软雅黑" in Chinese) font as sans-serif for Chinese scripts, and "SimSun"(or "宋体" in Chinese) as serif. For western scripts, IE 11 mostly use Arial and Times New Roman. 
See "Default font changes" http://msdn.microsoft.com/en-us/library/ie/dn467844%28v=vs.85%29.aspx

"Microsoft Yahei" was shipped since windows vista, "SimHei" since windows 2000, and "SimSun" since windows 95. 
See http://en.wikipedia.org/wiki/List_of_CJK_fonts

So I think FF can use "Microsoft Yahei"/"SimSun" on windows vista and above, "SimHei"/"SimSun" on XP.
Based on Duan Yao's helpful info in comment 5, especially 
https://msdn.microsoft.com/en-us/library/ie/dn467844(v=vs.85).aspx, and further research from MS's Fonts supplied with Windows * pages
http://www.microsoft.com/typography/fonts/product.aspx?PID=135
http://www.microsoft.com/typography/fonts/product.aspx?PID=145
http://www.microsoft.com/typography/fonts/product.aspx?PID=149
http://www.microsoft.com/typography/fonts/product.aspx?PID=161
http://www.microsoft.com/typography/fonts/product.aspx?PID=164
I've assembled this table of available fonts and their properties.
Based on these data I've come to the same conclusion as Duan Yao, and will shortly attach a proposed patch implementing such.
I've also included setting the font.name.cursive.zh-CN pref to KaiTi, since it seems an appropriate choice.
Attached patch proposed changes (obsolete) — Splinter Review
Flags: needinfo?(jdaggett)
Comment on attachment 8559469 [details] [diff] [review]
proposed changes

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

seems reasonable
Attachment #8559469 - Flags: review+
Blocks: 1131441
sorry had to back this out for xperf perma failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6458765&repo=mozilla-inbound starting with this push
Flags: needinfo?(jdaggett)
This is the error we see:
    TEST-UNEXPECTED-FAIL | mainthreadio | File '{fonts}\msyh.ttf' was accessed and we were not expecting it: {'Count': 1, 'Duration': 0.033023, 'RunCount': 1} 

xperf looks at all files access on the mainthread and reports unknown ones.  This is our whitelist of files:
http://hg.mozilla.org/build/talos/file/60d6980dec28/talos/mtio-whitelist.json

Most likely a font is something we can just add to the whitelist.

This is fairly easy to do, we can adjust it in the talos repo and update talos.json to point to the latest revision in that repo.  Before we do that, we need to confirm this is what is expected.
(In reply to Joel Maher (:jmaher) from comment #12)
> This is the error we see:
>     TEST-UNEXPECTED-FAIL | mainthreadio | File '{fonts}\msyh.ttf' was
> accessed and we were not expecting it: {'Count': 1, 'Duration': 0.033023,
> 'RunCount': 1} 
> 
> xperf looks at all files access on the mainthread and reports unknown ones. 
> This is our whitelist of files:
> http://hg.mozilla.org/build/talos/file/60d6980dec28/talos/mtio-whitelist.json
> 
> Most likely a font is something we can just add to the whitelist.
> 
> This is fairly easy to do, we can adjust it in the talos repo and update
> talos.json to point to the latest revision in that repo.  Before we do that,
> we need to confirm this is what is expected.

Ah, in that case I think KaiTi (simkai.ttf) and Microsoft YaHei (msyh.ttf) need whitelisting then, since they are newly added and thus this failure is expected, knowing about the existence of the whitelist.

SimHei (simhei.ttf) and Microsoft YaHei Bold (msyhbd.ttf) already seem to be in there however.
Flags: needinfo?(jmaher)
Here's a patch with (I believe) the needed talos changes.
Comment on attachment 8562082 [details] [diff] [review]
Add YaHei and KaiTi to the talos whitelist

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

this patch is correct.  I recommend landing this on the talos repository, then update talos.json:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json?from=talos.json&case=true#8

You could land the talos.json updates with the original patch in this bug!
Attachment #8562082 - Flags: review+
Flags: needinfo?(jmaher)
Attachment #8562082 - Flags: checkin?
Comment on attachment 8562082 [details] [diff] [review]
Add YaHei and KaiTi to the talos whitelist

please update testing/talos/talos.json to point to revision:
3edc0c98940e

no need to update the .zip for android.
Flags: needinfo?(jdaggett)
Attachment #8562082 - Flags: checkin? → checkin+
Try run all green, including the previously failing xperf test that caused the back-out.
Keywords: checkin-needed
Ok, so my mistake here was not running the 'xperf' test as part of my try push. I'll add that to future try pushes.
https://hg.mozilla.org/mozilla-central/rev/179a122ae672
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1166613
Depends on: 1181519
See Also: → 1205489
Assignee: jd.bugzilla → moz-ian
You need to log in before you can comment on or make changes to this bug.