Bug 55035 - Non-latin friendly default GUI fallback font for Chromium
Summary: Non-latin friendly default GUI fallback font for Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 57825
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-23 04:13 PST by Takayoshi Kochi
Modified: 2013-04-11 13:19 PDT (History)
10 users (show)

See Also:


Attachments
Change the default fallback font-family from arial to arial, sans-serif (534 bytes, patch)
2011-03-01 03:03 PST, Takayoshi Kochi
no flags Details | Formatted Diff | Diff
Added a ChangeLog entry and a comment on the source code. (1.66 KB, patch)
2011-04-04 14:02 PDT, Takayoshi Kochi
no flags Details | Formatted Diff | Diff
Fixed a tab in ChangeLog. (1.66 KB, patch)
2011-04-04 14:04 PDT, Takayoshi Kochi
no flags Details | Formatted Diff | Diff
Clarify the wording of ChangeLog statement. (1.71 KB, patch)
2011-04-04 14:06 PDT, Takayoshi Kochi
no flags Details | Formatted Diff | Diff
Fix comment style. (1.71 KB, patch)
2011-04-04 15:03 PDT, Takayoshi Kochi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takayoshi Kochi 2011-02-23 04:13:04 PST
Currently WebCore/rendering/RenderThemeChromiumSkia.cpp defins 'Arial' as the default GUI font,
but this is not suitable for languages that use non-latin characters, like CJK.

I'd like to propose that we would use "Arial, sans-serif" so that some proper fallback sans-serif font
could be selected for non-latin characters.


Index: WebCore/rendering/RenderThemeChromiumSkia.cpp
===================================================================
--- WebCore/rendering/RenderThemeChromiumSkia.cpp       (revision 79111)
+++ WebCore/rendering/RenderThemeChromiumSkia.cpp       (working copy)
@@ -79,7 +79,7 @@
 // sizes (e.g. 15px). So, for now we just use Arial.
 const String& RenderThemeChromiumSkia::defaultGUIFont()
 {
-    DEFINE_STATIC_LOCAL(String, fontFace, ("Arial"));
+    DEFINE_STATIC_LOCAL(String, fontFace, ("Arial, sans-serif"));
     return fontFace;
 }
Comment 1 Takayoshi Kochi 2011-03-01 03:03:48 PST
Created attachment 84206 [details]
Change the default fallback font-family from arial to arial, sans-serif
Comment 2 Tony Chang 2011-03-01 10:31:49 PST
If this brings us closer to IE's behavior, I think that's a good thing.

Does this change the results of any layout tests?  Can we add some layouts tests that illustrate the change?

The comment mentions that IE uses a font based on the encoding.  How hard is that to do (might not be worth the effort)?

The FIXME mentions that we don't match IE for ANSI encodings.  Is that still the case?

I thought Arial had CJK characters, or is that only Arial Unicode?
Comment 3 Takayoshi Kochi 2011-03-01 22:41:02 PST
Thanks for the comment.
I don't have IE/Windows handy now, so I'll check later today.

Arial doesn't have CJK characters, but "Arial Unicode MS" does (but a lot bigger).
http://en.wikipedia.org/wiki/Arial_Unicode_MS

(I'll also check if IE falls back "Arial" to "Arial Unicode MS" when it
needs to render any CJK characters.)
Comment 4 Jungshik Shin 2011-03-31 16:11:28 PDT
I believe that IE (I'm not sure about IE 9) uses different fonts even for controls like 'buttons' depending on the language ('lang' specified per element/page), the page encoding (as an indirect indicator of lang) and the UI language of IE. The same is true of Firefox, IIRC. 

In the past, Chrome used to have 
https://bugs.webkit.org/show_bug.cgi?id=18085 and Ojan's patch (that relied on it) to emulate Firefox and IE. 

Anyway, I believe adding 'sans-serif' here will certainly help at least as long as the UI language of Chrome matches that of the intended language of a page in current view.
Comment 5 Tony Chang 2011-03-31 17:06:32 PDT
BTW, I think this code change is fine.  You need to include a ChangeLog and set the r? flag if you want it reviewed.

Also, if it's not testable, please say why this change isn't testable.  I suspect you can create a manual test in WebCore/manual-tests if you're unable to make a layout test.
Comment 6 Takayoshi Kochi 2011-04-04 14:02:22 PDT
Created attachment 88122 [details]
Added a ChangeLog entry and a comment on the source code.
Comment 7 Takayoshi Kochi 2011-04-04 14:04:02 PDT
Created attachment 88123 [details]
Fixed a tab in ChangeLog.
Comment 8 Takayoshi Kochi 2011-04-04 14:06:42 PDT
Created attachment 88126 [details]
Clarify the wording of ChangeLog statement.
Comment 9 WebKit Review Bot 2011-04-04 14:10:46 PDT
Attachment 88126 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:79:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Takayoshi Kochi 2011-04-04 15:03:18 PDT
Created attachment 88138 [details]
Fix comment style.
Comment 11 WebKit Commit Bot 2011-04-05 02:18:18 PDT
Comment on attachment 88138 [details]
Fix comment style.

Clearing flags on attachment: 88138

Committed r82915: <http://trac.webkit.org/changeset/82915>
Comment 12 WebKit Commit Bot 2011-04-05 02:18:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2011-04-05 03:50:40 PDT
http://trac.webkit.org/changeset/82915 might have broken GTK Linux 32-bit Debug
Comment 14 Tony Chang 2011-04-05 09:44:36 PDT
Reopening.  This caused a bunch of tests to change results:
  https://bugs.webkit.org/show_bug.cgi?id=57825

They will need rebaselining if the change is expected and correct.
Comment 15 Takayoshi Kochi 2011-04-06 16:23:26 PDT
(In reply to comment #14)

Sorry for the trouble, and for this naive question:
where can I find the information which 270 tests failed for this?
Comment 16 Tony Chang 2011-04-06 16:38:20 PDT
(In reply to comment #15)
> (In reply to comment #14)
> 
> Sorry for the trouble, and for this naive question:
> where can I find the information which 270 tests failed for this?

It's on the waterfall.  Specifically:
http://chromegw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win/builds/3321
Comment 17 Takayoshi Kochi 2011-04-06 16:39:58 PDT
Thanks for the info!
I'll look into the failures and get back to you once everything is resolved.

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > 
> > Sorry for the trouble, and for this naive question:
> > where can I find the information which 270 tests failed for this?
> 
> It's on the waterfall.  Specifically:
> http://chromegw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win/builds/3321