Bug 10874

Summary:

lang, xml:lang, content-language ignored when choosing fonts

Product:

WebKit

Reporter:

Jakob Stoklund Olesen <stoklund>

Component:

Layout and Rendering

Assignee:

Alexey Proskuryakov <ap>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

allan.jensen, andy.dyson, ap, bashi, chrajohn, darin, efidler, falken, macpherson, meliste, mitz, nickshanks, phiw2, robburns1, tkent, webkit.review.bot, yutak

Priority:

Version:

419.x

Hardware:

Mac (PowerPC)

OS:

OS X 10.4

URL:

http://en.wikipedia.org/wiki/Han_unification

Bug Depends on:

18085, 93985, 9454, 20797, 67586, 76701, 76892, 78184, 83792, 97929

Bug Blocks:

24574, 77568

Attachments:

Description	Flags
work in progress patch	none
improved patch	none
use -webkit-locale for font selection	none
update patch after ap's review	none
revised after second review	none
revise after mitz's review	none
add tests for language-based font selection	none
Mac fix	buildbot: commit-queue-
Mac fix	none
Mac fix	none
Mac fix	mitz: review+

Jakob Stoklund Olesen

Reported 2006-09-15 13:08:21 PDT

When rendering Chinese, Japanese and Korean pages, WebKit uses the International preferences for choosing the font. The language attributes are not considered. This means that Chinese pages come out wrong on a Japanese computer. Example: <?xml version="1.0"?> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="zh-CN" lang="zh-CN"> <head> <meta http-equiv="content-language" content="zh-CN"/> <title>渤麦堂</title> </head> <body> <h1>麦界新闻 苹果 It's Showtime 特别发布会</h1> </body> </html> The H1 is rendered in different fonts, depending on the user's language choice. Traditional Chinese, simplified Chinese, and Japanese give three different results, only the simplified Chinese is correct. WebKit should look at lang/xml:lang attributes, and HTTP's Content-Language header before using user defaults to choose fonts for unihan characters.

Attachments
work in progress patch (40.23 KB, patch) 2011-06-10 03:36 PDT, Matt Falkenhagen	no flags	Details Formatted Diff Diff
improved patch (22.59 KB, patch) 2011-06-24 05:10 PDT, Matt Falkenhagen	no flags	Details Formatted Diff Diff
use -webkit-locale for font selection (26.57 KB, patch) 2011-07-07 02:14 PDT, Matt Falkenhagen	no flags	Details Formatted Diff Diff
update patch after ap's review (28.34 KB, patch) 2011-07-21 18:00 PDT, Matt Falkenhagen	no flags	Details Formatted Diff Diff
revised after second review (28.01 KB, patch) 2011-07-25 15:42 PDT, Matt Falkenhagen	no flags	Details Formatted Diff Diff
revise after mitz's review (28.77 KB, patch) 2011-08-04 01:25 PDT, Matt Falkenhagen	no flags	Details Formatted Diff Diff
add tests for language-based font selection (15.72 KB, patch) 2012-02-07 21:37 PST, Matt Falkenhagen	no flags	Details Formatted Diff Diff
Mac fix (17.85 KB, patch) 2012-04-10 13:16 PDT, Alexey Proskuryakov	buildbot: commit-queue-	Details Formatted Diff Diff
Mac fix (18.01 KB, patch) 2012-04-10 14:18 PDT, Alexey Proskuryakov	no flags	Details Formatted Diff Diff
Mac fix (20.19 KB, patch) 2012-04-11 11:58 PDT, Alexey Proskuryakov	no flags	Details Formatted Diff Diff
Mac fix (22.36 KB, patch) 2012-04-11 12:49 PDT, Alexey Proskuryakov	mitz: review+	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Dave Hyatt

Comment 1 2006-09-16 11:34:13 PDT

Do other browsers do this? What do WinIE and Firefox do?

Jakob Stoklund Olesen

Comment 2 2006-09-16 12:33:15 PDT

(In reply to comment #1) > Do other browsers do this? What do WinIE and Firefox do? Firefox/Mac uses the lang attribute, as you can see by opening the wikipedia URL in FF. Don't know about WinIE. Opera 9.01/Mac uses the lang attribute. Firefox gets the example wrong - it uses a mixture of traditional and simplified Chinese fonts. Opera displays the example in a single simplified Chinese font as it should.

Alexey Proskuryakov

Comment 3 2006-09-17 00:49:08 PDT

Although Han unification is by far the most prominent, it is not the only problematic one - the same issues exist with Arabic/Urdu, Greek/Coptic, Russian/Serbian, and modern/old (church) Cyrillic (examples taken from Unicode Demystified by Richard Gillam, chapter 3).

Eric Seidel (no email)

Comment 4 2007-01-31 04:31:05 PST

it seems :lang selection doesn't work at all: http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-styling-css-05-b.html

Andrew Dyson

Comment 5 2007-03-01 11:29:59 PST

(In reply to comment #1) > Do other browsers do this? What do WinIE and Firefox do? See test case : http://www.w3.org/International/tests/sec-cjk-fonts test result : note that Safari does not take into account 'lang' when selecting fonts while FF, IE, and Opera do. http://www.w3.org/International/tests/results/results-lang-and-cjk-font.php (In reply to comment #4) > it seems :lang selection doesn't work at all: > http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-styling-css-05-b.html > I guess this needs to be a separate bug. Nonetheless, here are some links http://www.w3.org/International/tests/#css-lang http://www.w3.org/International/tests/results/css-lang http://www.w3.org/International/questions/qa-css-lang

Robert Burns

Comment 6 2007-05-06 00:01:47 PDT

This is one of those things where WebKit gets it right and most other browsers get it really really wrong. Unfortunately that creates an issue where WebKit has to decide what path to take. Changing fonts due to language attributes is a really bad idea. Its not what Unicode intended for Unihan ideographs. It takes control away from content creators. And the list goes on. Glyphs should change when content creators specify different fonts. For fallback, WebKit (and other browsers) should select an appropriate glyph based on language context. Or if an OpenType font includes support for language dependent glyph variants. But the behavior demonstrated in these examples is just plain wrong from the spec. Also, this is not to say that WebKit doesn't need to fix its support for the lang CSS selectors which I believe it did with bug#3233). However, that's handled in other bug reports. Fixing the language selector and adding CJKV language dependent fonts to the default styleshet would be a setp towards addressing this issue. The only other thing that should be done would be to ensure that fallback font selection made use of that default useragent stylesheet before other fallbacks. Unicode suggests its the users enviornment settings that should govern the font selection (for plain text anyway). I think that should also guide fallback algorithms for CSS.

Allan Sandfeld Jensen

Comment 7 2007-08-22 10:01:30 PDT

This is actually a bug in Unicode. You should report it there. But we need to fix it in webkit too. The issue is that unicode is completely inconsistent. In the beginning they represented separated symbols. For instance there is different encodings for European numbers and Arabic number even though the meaning are the same, they are separated because the symbols are different. This made Unicode usefull for encoding fonts. But when the same type of issues appears in Asian scripts, Unicode changes and decides on encoding semantics instead of symbols, so suddenly the one unicode-character has several different graphical symbol. Unfortunately it is unlikely that Unicode will fix their bugs, since it is so fundamentally flawed already. Btw. Another place language is needed is in correct capitalization (like CSS text-transform). In Turkish a small 'i' needs to be capitalized to a big İ(dot over I), and a large 'I' to be lower-cased to 'ı' (i with no dot).

Robert Burns

Comment 8 2007-08-22 11:53:38 PDT

(In reply to comment #7) > This is actually a bug in Unicode. You should report it there. But we need to > fix it in webkit too. I'm not so sure it is a bug in Unicode. Unicode has to balance many competing interests. They unify some things and dis-unify others. The rationale may not always be clear, but however they handle unification, it doesn't prevent proper glyph matching and text layout. It simply moves it to the presentation level (without an underlying character/semantic difference). > > The issue is that unicode is completely inconsistent. In the beginning they > represented separated symbols. For instance there is different encodings for > European numbers and Arabic number even though the meaning are the same, they > are separated because the symbols are different. This made Unicode usefull for > encoding fonts. But when the same type of issues appears in Asian scripts, > Unicode changes and decides on encoding semantics instead of symbols, so > suddenly the one unicode-character has several different graphical symbol. This is actually one of the *features* of Unicode. Separating the semantics from the presentation or the model from the view. > Btw. Another place language is needed is in correct capitalization (like CSS > text-transform). In Turkish a small 'i' needs to be capitalized to a big > İ(dot over I), and a large 'I' to be lower-cased to 'ı' (i with no > dot). This is actually a better example where glyphs should be different depending on the language settings. These CSS text transforms should be language aware for glyph selection. In the case of Han unification, the webpage author should select a suitable font to display the Han characters with the desired glyphs. The font the author selects will already be language dependent. However, WebKit (and the various text systems it uses) should be language aware in it's glyph fallback selection. That is when the fonts specified by the author are not available, the text system should select the appropriate Han glyphs from an appropriate font based on the language settings of that run of text.

Jungshik Shin

Comment 9 2007-11-14 12:57:15 PST

(In reply to comment #1) > Do other browsers do this? What do WinIE and Firefox do? Firefox has separate font preferences for script/langGroup and it does honor lang/xml:lang when selecting fonts. If lang/xml:lang is not specified, it falls back to script/langGroup inferred from charset (that does not always work, but in many cases, it works more or less). Of course, documents encoded in UTF-{8,16,32} (or with a lot of NCRs to represent characters outside the repertoire of the current charset), this breaks down. For UTF-{8,16,32}, it falls back to the font for the lang/scriptGroup corresponding to the current application locale. (its lang/scriptGroup is not optimal being written almost 10 years ago. It'd be done differently if it was to be done today). I have a partial patch in that direction, but it's not anywhere near to be complete. WebKit currently has only a single set of font preference across all languages/scripts. |Settings| in webkit is not expressive enough (because it does not have a general 'key-value' with or without hierarchy) to have these font preferences. font.<script>.serif font.<script>.sans-serif font.<script>.monospace As for Han-disambiguation and Cyrillic-lang dependency, we need to do more than the above, but the above is a start.

Nicholas Shanks

Comment 10 2008-03-26 02:26:46 PDT

Regarding comment #9, I believe you are touching upon a bug in CSS, whereby serif/sans-serif and monospace/proportional are actually orthogonal properties which have been conflated. For example, compare Courier and Monaco. If WebKit implements hierarchical preferences like Gecko has, we should use: font.<script>.serif font.<script>.serif.monospace font.<script>.sans-serif font.<script>.sans-serif.monospace Presumably the font used for characters that have no script would be chosen based on the surrounding script, if any.

mitz

Comment 11 2008-08-25 18:23:20 PDT

<rdar://problem/3605214> is a related Apple bug.

Jungshik Shin

Comment 12 2009-03-09 10:53:39 PDT

bug 18085 has a tentative patch (that does not use 'lang/xml:lang' but infers 'dominant script' from the document character encoding). With bug 16801 (for which I just uploaded a tentative patch) fixed and some more changes in Settings, it can be made a patch for this bug.

Jungshik Shin

Comment 13 2009-03-09 10:56:53 PDT

Sorry for bug spam. I meant to take this adding the previous comment

Nicholas Shanks

Comment 14 2009-03-13 04:33:51 PDT

(In reply to comment #8) > In the case of Han unification, the webpage author should select a suitable > font to display the Han characters with the desired glyphs. The font the > author selects will already be language dependent. I whole heartedly disagree with you here. In all my websites I do not specify a typeface for the "body copy" (main bulk text) of my site. I don't even use the 'serif' and 'sans-serif' generic classes. I let the user specify their preferred font. If they want to read the internet in comic sans, then that's their problem, and I will be happy to let them suffer through it. However, I do expect that Urdu and Farsi content will be presented in a Nasta'liq font, while Arabic would be presented using a Naskh font; and that Chinese will be presented in a Han font while Japanese will be presented in a Kanji font. Similarly, I expect a passage in Old Church Slavonic will use a single typeface for the entire passage that contains the OCS glyphs required, and not use the default Cyrillic font for 99% of the glyphs (such as Lucida Grande) and then find it needs to fall back to another font (like Gentium) to display one letter here and there, making the presentation look like rotten turds. Can you imagine what an English sentence would look like if the letters q and z had to come from Helvetica while the rest of the passage was set in Times?

Matt Falkenhagen

Comment 15 2011-06-10 03:36:39 PDT

Created attachment 96724 [details] work in progress patch

Matt Falkenhagen

Comment 16 2011-06-10 03:37:40 PDT

Comment on attachment 96724 [details] work in progress patch This patch is not yet ready for formal review, but any comments would be appreciated. It is based strongly on Jungshik's patch on bug 18085 and includes a version of the patch on bug 20797 that can be removed when that is committed. This patch: 1. Adds a script member to FontDescription. 2. Sets that script in CSSStyleSelector based on lang/xml:lang/content-language. 3. Modifies fontDataForGenericFamily to get the appropriate font for the script This patch does not yet: 1. Infer language/script from the charset (Jungshik's patch for bug 18085 has this) 2. Choose a good font when the lang attribute is used but font style is not. 3. Deal with lang changing dynamically.

Alexey Proskuryakov

Comment 17 2011-06-10 15:51:18 PDT

Comment on attachment 96724 [details] work in progress patch View in context: https://bugs.webkit.org/attachment.cgi?id=96724&action=review It's great that you are working in this. This is a huge issue, in my opinion. > Source/JavaScriptCore/ChangeLog:12 > + * wtf/unicode/UScriptCode.h: Added. Is this the same one that's being added in bug 20797? I previously made some comments in that bug. > Source/WebCore/css/CSSStyleSelector.cpp:4260 > + AtomicString lang = m_element->computeInheritedLanguage(); This bug is blocked on bug 16801, which says that language computation is likely unacceptably slow to be used for text rendering yet.

Alexey Proskuryakov

Comment 18 2011-06-10 15:52:25 PDT

> Is this the same one that's being added in bug 20797? I see now that you said that.

Matt Falkenhagen

Comment 19 2011-06-13 02:25:30 PDT

Thanks for the comments! I am aware of bug 16801 and am hoping first to get a functioning implementation to better understand what needs to be done and measure the performance impact. It may be easier to tackle bug 16801 once that is done.

Matt Falkenhagen

Comment 20 2011-06-24 05:10:39 PDT

Created attachment 98483 [details] improved patch

Matt Falkenhagen

Comment 21 2011-06-24 05:16:22 PDT

Comment on attachment 98483 [details] improved patch Sorry, I forgot to pass --no-review to webkit-patch upload. This patch is now easier to understand as it no longer contains the patch from bug 20797. It also implements the 3 things that the previous patch is listed as not implementing. Again, it is based strongly on Jungshik's earlier patches (bug 16801 and bug 18085).

Alexey Proskuryakov

Comment 22 2011-06-24 08:48:26 PDT

I talked to Dan about efficiently propagating language, and we now have an idea about how to do that. WebKit already has a -webkit-locale CSS property, which affects hyphenation and line breaking. You could just turn lang into a mapped attribute (like for example font) to translate it into -webkit-locale, and use that for font selection, too. If the difference between locale and language is important here (which I don't think it is), you may also add a new CSS property. I would suggest doing that in separate steps: 1) Add per-script defaults for languages. That would need some discussion with Safari engineers, because silently overriding preferences set by a user is not so nice. 2) Implement font selection based on explicitly specified -webkit-locale. At this point, you can already have regression tests. 3) Add mapping from lang to -webkit-locale. 4) Add logic that takes encoding into account. Does that sound like a reasonable approach to you?

Jungshik Shin

Comment 23 2011-06-24 15:04:11 PDT

> just turn lang into a mapped attribute (like for example font) to translate it into -webkit-locale, and use that for font selection We hit upon the same idea almost simultaneously :-) Yesterday, I was scanning recent changes in ChangeLog and came across Dan's patch to make LineBreakIterator dependent on webkit-locale. I'm trying it for bug 16801 (to use in bug 21312).

Matt Falkenhagen

Comment 24 2011-06-27 02:08:35 PDT

Alexey, thank you very much for your detailed comment! It sounds like a reasonable approach.

Jungshik Shin

Comment 25 2011-06-27 10:52:34 PDT

> 1) Add per-script defaults for languages. . For ICU platforms, it's almost free because ICU has an API for that : add_likelySubtag and getScript. For instance, ru will be expanded to ru_Cyrl_RU and getScript run on that expanded that will return Cyrl. If we pass sr_Latn, 'Latn' will remain as it is. 'Cyrl' can be converted to ICU UScriptCode enum with another API. There's one problem, though. The ICU API in question was added in ICU 3.8 or 4.0 and Mac OS 10.5 has ICU 3.6.... For non-ICU platforms, we need to do what Matt is doing (hard-coding and caching). Even on ICU platforms, we can do caching. > That would need some discussion with Safari engineers, because silently overriding preferences set by a user is not so nice Actually, as long as Safari does not have per-script font preferences (but just global font preferences), the behavior will not be changed because in that case, the current patch will fall back to fonts for UScriptCommon (which is the only If we want to hard-code some preferences for Safari on Mac (as the latest patch does), what we can do is : - replace each CSS generic with two fonts : one for ScriptCommon (that corresponds to the font selected by a user in Safari's UI) followed by a hard-coded font for each script + CSS generic. That way, what a user selected in the UI is still respected. As for the plan outlined, that looks great.

Matt Falkenhagen

Comment 26 2011-07-07 02:14:23 PDT

Created attachment 99959 [details] use -webkit-locale for font selection

Matt Falkenhagen

Comment 27 2011-07-07 02:22:47 PDT

Comment on attachment 99959 [details] use -webkit-locale for font selection This patch links -webkit-locale with the per-script font settings that Jungshik implemented in bug 20797 (i.e., step #2 in Alexey's comment #22). The default settings have not changed, so there should still be no visible change yet.

Matt Falkenhagen

Comment 28 2011-07-12 18:57:10 PDT

Could someone please review the latest patch? Since there is no visible effect (default fonts haven't changed), I didn't include a regression test. Would it be better to include one now and update the expectations when the font settings change?

Alexey Proskuryakov

Comment 29 2011-07-20 12:00:30 PDT

Comment on attachment 99959 [details] use -webkit-locale for font selection View in context: https://bugs.webkit.org/attachment.cgi?id=99959&action=review Mitz asked me to look over parts other than CSS and font code. > Source/WebCore/ChangeLog:30 > + * css/LocaleUtils.cpp: Added. This file should have a "Default" suffix, matching e.g Collator in JavaScriptCore/wtf/Unicode. Without it, it looks like a file that's used by all platforms. These files should be in platform/text, not in css. "Utils" is not so great to have in file name. It doesn't convey any information. I don't have a suggestion at the moment, but at the very least, "Utilities" should not be abbreviated. > Source/WebCore/WebCore.gypi:2373 > + 'css/LocaleUtils.cpp', > + 'css/LocaleUtilsICU.cpp', Why does this this file list both? Wy doesn't that break the build due to duplicate definition of localeToScriptCode()? > Source/WebCore/css/LocaleUtils.cpp:47 > + // TODO: Need to add all the languages. We use FIXME, not TODO. Furthermore, this comment is not actionable. It's not practically possible to add all languages. I would have said something like "The below list only includes languages that need correct script identification for choosing a default font". How hard would it be to just take all languages some modern version of ICU supports though? > Source/WebCore/css/LocaleUtils.cpp:48 > + const LocaleScript localeScriptList[] = { This looks like it will be copied every time, no? > Source/WebCore/css/LocaleUtils.cpp:52 > + { "de", USCRIPT_LATIN }, > + { "en", USCRIPT_LATIN }, > + { "es", USCRIPT_LATIN }, > + { "fr", USCRIPT_LATIN }, The short list of Roman languages seems particularly evil. Perhaps you should just maps them all to USCRIPT_COMMON, given that no port is going to set different defaults for COMMON and for ROMAN. > Source/WebCore/css/LocaleUtils.h:34 > +#include <wtf/text/AtomicString.h> No need to include, a forward declaration would work better. > Source/WebCore/css/LocaleUtils.h:39 > +UScriptCode localeToScriptCode(const AtomicString& locale); This should take const String&, since you don't ever use AtomicString features. > Source/WebCore/css/LocaleUtilsICU.cpp:40 > +UScriptCode canonicalizeScriptCode(UScriptCode scriptCode) This should be a static. The build should have broken because this doesn't have a forward declaration otherwise. What does "Canonical" mean here? I'm quite confused.

Matt Falkenhagen

Comment 30 2011-07-21 18:00:51 PDT

Created attachment 101680 [details] update patch after ap's review

Matt Falkenhagen

Comment 31 2011-07-21 18:09:03 PDT

Comment on attachment 99959 [details] use -webkit-locale for font selection View in context: https://bugs.webkit.org/attachment.cgi?id=99959&action=review Thank you for the review! I've added a new patch. >> Source/WebCore/WebCore.gypi:2373 >> + 'css/LocaleUtilsICU.cpp', > > Why does this this file list both? Wy doesn't that break the build due to duplicate definition of localeToScriptCode()? I looked at similar cases and it seems like the .gypi includes all files and then excludes the unwanted ones in the .gyp file (as I did here). For example, the TextIteratorIterator* files in WebCore. >> Source/WebCore/css/LocaleUtils.cpp:47 >> + // TODO: Need to add all the languages. > > We use FIXME, not TODO. Furthermore, this comment is not actionable. It's not practically possible to add all languages. > > I would have said something like "The below list only includes languages that need correct script identification for choosing a default font". > > How hard would it be to just take all languages some modern version of ICU supports though? I think it should be straightforward to do that. >> Source/WebCore/css/LocaleUtils.cpp:52 >> + { "fr", USCRIPT_LATIN }, > > The short list of Roman languages seems particularly evil. Perhaps you should just maps them all to USCRIPT_COMMON, given that no port is going to set different defaults for COMMON and for ROMAN. Right, I kind of just meant this list as a preview for before adding "all the languages". Maybe it is better to just remove the Latin languages for now. I can add all the ICU languages in a later patch. >> Source/WebCore/css/LocaleUtilsICU.cpp:40 >> +UScriptCode canonicalizeScriptCode(UScriptCode scriptCode) > > This should be a static. The build should have broken because this doesn't have a forward declaration otherwise. > > What does "Canonical" mean here? I'm quite confused. I added a comment in the code.

Alexey Proskuryakov

Comment 32 2011-07-22 17:00:47 PDT

Comment on attachment 101680 [details] update patch after ap's review View in context: https://bugs.webkit.org/attachment.cgi?id=101680&action=review > Source/WebCore/page/Settings.cpp:71 > + if (script == USCRIPT_COMMON) > + return emptyAtom; > + return getGenericFontFamilyForScript(fontMap, USCRIPT_COMMON); I don't understand why an empty font family is returned for USCRIPT_COMMON. > Source/WebCore/platform/text/LocaleToScriptMapping.h:38 > +namespace WTF { > + > +class String; I gave a bad/incomplete advice. This file should be including wtf/Forward.h. > Source/WebCore/platform/text/LocaleToScriptMapping.h:44 > +UScriptCode localeToScriptCode(const WTF::String& locale); Then you won't need this WTF:: > Source/WebCore/platform/text/LocaleToScriptMappingICU.cpp:41 > +// There are certain families of script codes that we want to treat as a single > +// script for the purpose of assigning a per-script font in Settings. For I'd call the function scriptCodeForFontSelection() then. The rest of this comment is easy to infer from the intent, which should be conveyed by function name. > Source/WebCore/platform/text/LocaleToScriptMappingICU.cpp:61 > +UScriptCode localeToScriptCode(const String& locale) And since this always returns a script code for font selection, this would be localeToScriptCodeForFontSelection(). Does that make sense? Do you envision any other uses of localeToScriptCode, with or without such canonicalization? > Source/WebCore/platform/text/LocaleToScriptMappingICU.cpp:80 > + // Ignore error that multiple scripts could be returned, since we only want > + // one script. This is a short line, no need to break it.

Early Warning System Bot

Comment 33 2011-07-24 16:33:49 PDT

Comment on attachment 101680 [details] update patch after ap's review Attachment 101680 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9233734

Matt Falkenhagen

Comment 34 2011-07-25 15:42:58 PDT

Created attachment 101929 [details] revised after second review

Matt Falkenhagen

Comment 35 2011-07-25 15:52:51 PDT

Comment on attachment 101680 [details] update patch after ap's review View in context: https://bugs.webkit.org/attachment.cgi?id=101680&action=review Thanks again for the review. I uploaded a new patch. >> Source/WebCore/page/Settings.cpp:71 >> + return getGenericFontFamilyForScript(fontMap, USCRIPT_COMMON); > > I don't understand why an empty font family is returned for USCRIPT_COMMON. It's just falling back to USCRIPT_COMMON. I feel like we always want to fallback here, but perhaps there is some situation where we must not fallback. In that case, I would have to move the fallback code outside of this function. >> Source/WebCore/platform/text/LocaleToScriptMappingICU.cpp:61 >> +UScriptCode localeToScriptCode(const String& locale) > > And since this always returns a script code for font selection, this would be localeToScriptCodeForFontSelection(). > > Does that make sense? Do you envision any other uses of localeToScriptCode, with or without such canonicalization? Yes, this makes sense. I imagine it will always be used for font selection.

Alexey Proskuryakov

Comment 36 2011-07-25 16:11:02 PDT

Comment on attachment 101929 [details] revised after second review Thank you, looks good to me. Dan?

mitz

Comment 37 2011-08-03 21:06:52 PDT

Comment on attachment 101929 [details] revised after second review View in context: https://bugs.webkit.org/attachment.cgi?id=101929&action=review r=me but please see the comment below and address if needed. > Source/WebCore/css/CSSFontSelector.cpp:500 > + if (fontDescription.genericFamily() == FontDescription::StandardFamily && !fontDescription.isSpecifiedFont()) > + return fontDataForGenericFamily(m_document, fontDescription, "-webkit-standard"); I think you need similar handling in the !m_fontFaces.isEmpty() case that follows a few lines below. That case would be hit if the document had @font-face rules but you were still looking at the initial font.

Matt Falkenhagen

Comment 38 2011-08-04 01:25:33 PDT

Created attachment 102883 [details] revise after mitz's review

Matt Falkenhagen

Comment 39 2011-08-04 01:29:58 PDT

Thanks for the review! I've added a new patch addressing your comment.

WebKit Review Bot

Comment 40 2011-08-04 08:22:18 PDT

Comment on attachment 102883 [details] revise after mitz's review Clearing flags on attachment: 102883 Committed r92375: <http://trac.webkit.org/changeset/92375>

WebKit Review Bot

Comment 41 2011-08-04 08:22:26 PDT

All reviewed patches have been landed. Closing bug.

Matt Falkenhagen

Comment 42 2011-08-04 18:10:35 PDT

This bug should probably not be marked RESOLVED until lang/xml:lang is mapped to -webkit-locale as Jungshik's patch for bug 16801 does. But it seems I can't change the Status directly.

Kent Tamura

Comment 43 2011-08-04 18:14:57 PDT

(In reply to comment #42) > This bug should probably not be marked RESOLVED until lang/xml:lang is mapped to -webkit-locale as Jungshik's patch for bug 16801 does. But it seems I can't change the Status directly. Ok, I reopen.

Matt Falkenhagen

Comment 44 2012-02-07 21:37:47 PST

Created attachment 125996 [details] add tests for language-based font selection

Matt Falkenhagen

Comment 45 2012-02-07 21:51:41 PST

Now that lang/xml:lang is mapped to -webkit-locale (bug 67586), lang/xml:lang is used for font selection. I proposed a patch with some tests for that. Font selection based on http-equiv content-language is also implemented (bug 76701). But the content-language in the HTTP header is not yet used (bug 76892).

Alexey Proskuryakov

Comment 46 2012-02-08 00:55:36 PST

Please note that this bug covers making it work in Safari (it was filed in 2006).

Matt Falkenhagen

Comment 47 2012-02-08 17:08:08 PST

(In reply to comment #46) > Please note that this bug covers making it work in Safari (it was filed in 2006). That's true. It works on Safari but the tests rely on per-script font settings support in overridePreference of DumpRenderTree which is only implemented for the Chromium port so far. I've filed bug 78184.

Matt Falkenhagen

Comment 48 2012-03-14 20:45:12 PDT

Comment on attachment 125996 [details] add tests for language-based font selection Clear r? on stale patch.

Alexey Proskuryakov

Comment 49 2012-04-10 13:16:32 PDT

Created attachment 136522 [details] Mac fix

Build Bot

Comment 50 2012-04-10 13:58:17 PDT

Comment on attachment 136522 [details] Mac fix Attachment 136522 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12383407

Alexey Proskuryakov

Comment 51 2012-04-10 14:18:30 PDT

Created attachment 136541 [details] Mac fix Oh llvm_gcc...

Alexey Proskuryakov

Comment 52 2012-04-10 18:20:57 PDT

Comment on attachment 136541 [details] Mac fix Withdrawing for now, got some offline review comments to address.

Alexey Proskuryakov

Comment 53 2012-04-11 11:58:33 PDT

Created attachment 136717 [details] Mac fix

Alexey Proskuryakov

Comment 54 2012-04-11 12:49:44 PDT

Created attachment 136728 [details] Mac fix Forgot to include tests.

Alexey Proskuryakov

Comment 55 2012-04-11 13:13:54 PDT

Fixed in <http://trac.webkit.org/changeset/113900>.

Note You need to log in before you can comment on or make changes to this bug.