Bug 55035 – Non-latin friendly default GUI fallback font for Chromium

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Takayoshi Kochi]

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;
 }

[Takayoshi Kochi]

Created attachment 84206 [details]
Change the default fallback font-family from arial to arial, sans-serif

[Tony Chang]

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?

[Takayoshi Kochi]

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.)

[Jungshik Shin]

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.

[Tony Chang]

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.

[Takayoshi Kochi]

Created attachment 88122 [details]
Added a ChangeLog entry and a comment on the source code.

[Takayoshi Kochi]

Created attachment 88123 [details]
Fixed a tab in ChangeLog.

[Takayoshi Kochi]

Created attachment 88126 [details]
Clarify the wording of ChangeLog statement.

[WebKit Review Bot]

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.

[Takayoshi Kochi]

Created attachment 88138 [details]
Fix comment style.

[WebKit Commit Bot]

Comment on attachment 88138 [details]
Fix comment style.

Clearing flags on attachment: 88138

Committed r82915: <http://trac.webkit.org/changeset/82915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82915 might have broken GTK Linux 32-bit Debug

[Tony Chang]

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.

[Takayoshi Kochi]

(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?

[Tony Chang]

(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

[Takayoshi Kochi]

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