102623 - fixed-width mail rendering does NOT use user-specified fixed-width fonts

Status: VERIFIED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Jungshik Shin]

When 'fixed width' font is selected for rendering
of text/plain email messages in Edit->Pref->Mail&Newsgroup
->Message Display, all characters are displayed
with the same width. This is different from what most
CJK users expect. When fixed font is selected,
they expect CJK characters to be twice as wide
as 'non-CJK characters'. For email messages
encoded in legacy CJK encodings (EUC-JP,KR, ISO-2022-JP,
Big5, GB2312-80), the distinction is easy.

For UTF-8 text, sometimes
this distinction between CJK and non-CJK characters
are ambiguous, but Unicode consortium has a technical
report on this issue at http://www.unicode.org/unicode/reports/tr11/ 
Also, Markus Kuhn has a simple portable C code
at  
http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c

Comment 1

[Roy Yokoyama]

assiging to nhotta, ccing bstell and ftang

Comment 2

[nhottanscp]

On my Windows machine, Japanese characters are shown twice the width of ASCII
characters.
Xianglan, do you see the problem with Linux build?

Comment 3

[ji]

On my systems, I also saw Japanese characters are shown twice the width of ASCII
characters. I'm on Japanese XP, RH7.1-J and Mac OS X.

Comment 4

[Jungshik Shin]

Indeed, a few Japanese email messages I have don't have
the problem I have with Korean messages. In those messages,
Japanese characters are twice as wide as US-ASCII chars
when fixed-width font is selected. 
I'm going to attach a screenshot. In that screenshot,
Korean message is displayed with a font I never specifed
to be used by Mozilla. That is, the font used is NOT
one of fonts I selected in Edit | Pref | Appearance | Fonts | Korean.
It seems like Hangul glyphs come from
-misc-fixed-medium-r-normal-ko-18-120-100-100-c-180-iso10646-1,
but glyphs for US-ASCII part don't look like they're from
that font. It's really mysterious where Mozilla picked up that
font (of course, it's on my system, but I didn't specify it be used
by Mozilla). 

  I'll try to figure this out and get back to you.

Comment 5

[Jungshik Shin]

Attachment: Korean email mesg. with fixed-width font selected

Comment 6

[ji]

I see the problem on my RH7.1-J when viewing a Korean mail. 
On my system, I don't have iso10646-1 fonts, I have ksc5601 fonts instead.

Comment 7

[ji]

It seems a regression from NS 6.1. 
And I don't see this problem with Chinese mails. 

Comment 8

[nhottanscp]

Brian, could you take a look at this?
Does this also happens with plain text view by browser?

Comment 9

[ji]

I don't see when viewing a txt file on a browser that contains a mixed string of
Korean and ASCII chars.

Comment 10

[Jungshik Shin]

I also tried an html file with some Korean-ASCII mixed strings
within <pre> and </pre>, but it doesn't have any problem.

Comment 11

[Frank Tang]

bstell- please mark it assign if you agree to work on it. 

Comment 12

[Frank Tang]

look at bug 102940 . Are they teh same font ?

Comment 13

[Jungshik Shin]

It's not so easy to tell. 's' seems pretty similar and 't' looks remotely
similar,but 'l' and 'i' are not kind of characters useful
for font comparison.
A mystery with this bug is where on earth Mozilla 
comes up with both English and Korean fonts it uses to render
a Korean mail message in the screenshot. Neither of them are
specified to be used anywhere, but still they're used...

Comment 14

[Jungshik Shin]

I set NS_FONT_DEBUG and found that Mozilla was loading
the following 4 fonts when rendering a Korean mail message.

-----------
loaded -ksh-hymjsm2-bold-r-normal--12-90-100-100-c-120-ksc5601.1987-0
loaded -wadalab-gothic-medium-r-normal--16-*-0-0-c-*-iso8859-1
loaded -kaist-gothic-medium-r-normal--16-160-75-75-c-160-johab-1
loaded -daewoo-gothic-medium-r-normal--16-120-100-100-c-160-ksc5601.1987-0
----------

The first one was NOT used to display the body of the message.
Two fonts used to render the email body are the second one
and the third one. And glyphs for US-ASCII chars from the second
one were scaled to match the width of glyphs for Korean chars
drawn from the the third font. Except for the first font,
none of them was specified to be used in my preference. 

The font selection mechanism for plain text mail rendering
with fixed font appears to be beyond end-user-control and
to be hard-coded somewhere (perhaps to use 'gothic' family....)
IMHO, this has to be fixed so that 'fixed-font' rendering
of text/plain email should be dealt with the same way as
text/plain in browser window or what's enclosed with
<pre> and </pre> in text/html. Perhaps, by specifying
implicit CSS to this case?? 

In bug 83250, Katakai san addressed some of problems, but
that part - TryFamily- didn't get called in this case.

Comment 15

[Jungshik Shin]

I grabbed the latest nsFontMetricsGTK.cpp (with Brian's
check-in dated Oct. 2nd for bug 94327) to see if that
makes any difference. It did make a difference. Because
wadalab-gothic is scalable font and there's a bitmap font
of the size Mozilla is looking for. 

However, this does not change the fact that end-user has
no controll over what fonts are used to render
email messages when fixed width font is selected in mail display
pref. setting. As I wrote before, I believe rendering
of email should be in line with rendering text/plain
in web browsing window. 

Another related issue is how to filter out fonts with 'abnormal'
(much wider than 'normal') width for US-ASCII glyphs.  These 'abnormal' US-ASCII
glyphs mostly come from East Asian
truetype fonts presented as X11 fonts by X-TT or freetype (truetype
font servers included in XFree86 4.x). Perhaps, there's not much
Mozilla can do here other than adding to Release Note that
entries like the following NOT be added to fonts.dir/fonts.scale
file in the directory where truetype fonts are.

watanabe-mincho.ttf -watanabe-mincho-medium-r-normal--0-0-0-0-c-0-iso8859-1

The best to do is ask XF86 maintainers to address this problem.

Comment 16

[Jungshik Shin]

Brian,
Just in case, I wanna add that  
your patch for bug 94327 made things better for this bug (not
worse) although it didn't touch the heart of this bug because
this bug and bug 94327 deal with two separate issues in my opinion.
I'm changing  the summary line to reflect that.

Comment 17

[Frank Tang]

ok, the text of "Fixed width font" is in 
/mailnews/base/prefs/resources/locale/en-US/pref-viewing_messages.dtd, line 46 
-- <!ENTITY fixedWidth.label "Fixed width font">
and fixedWidth.label is used in 
/mailnews/base/prefs/resources/content/pref-viewing_messages.xul, line 55 -- 
<radio class="small-margin"
group="mailFixedWidthMessages" value="true" label="&fixedWidth.label;"/>

and in mailnews/base/prefs/resources/content/pref-viewing_messages.xul
 52       <radiogroup id="mailFixedWidthMessages" 
 53                   pref="true" preftype="bool" 
prefstring="mail.fixed_width_messages"
 54                   prefattribute="value">

so the pref value is mail.fixed_width_messages
and mail.fixed_width_messages is used in 
/mailnews/mime/src/mimemoz2.cpp, line 1459 -- 
msd->options->prefs->GetBoolPref("mail.fixed_width_messages",
&MIME_VariableWidthPlaintext);
/mailnews/mime/src/mimetpfl.cpp, line 144 -- nsresult rv = 
prefs->GetBoolPref("mail.fixed_width_messages",

in source/mailnews/mime/src/mimemoz2.cpp#1499
1499   msd->options->variable_width_plaintext_p = MIME_VariableWidthPlaintext;

144     nsresult rv = prefs->GetBoolPref("mail.fixed_width_messages",
145                                      &(exdata->fixedwidthfont));

mailnews/mime/src/mimetpfl.cpp#144

150   // Get font
151   // only used for viewing (!plainHTML)
152   nsCAutoString fontstyle;
153   if (nsMimeOutput::nsMimeMessageBodyDisplay == obj->options->format_out ||
154       nsMimeOutput::nsMimeMessagePrintOutput == obj->options->format_out)
155   {
156     /* Use a langugage sensitive default font
157        (otherwise unicode font will be used since the data is UTF-8). */
158     char fontName[128];     // default font name
159     PRInt32 fontSize;       // default font size
160     PRInt32 fontSizePercentage;   // size percentage
161     nsresult rv = GetMailNewsFont(obj, exdata->fixedwidthfont,
162                                   fontName, 128, &fontSize, 
&fontSizePercentage);
163     if (NS_SUCCEEDED(rv))
164     {
165       fontstyle = "font-family: ";
166       fontstyle += fontName;
167       fontstyle += "; font-size: ";
168       fontstyle.AppendInt(fontSize);
169       fontstyle += "px;";
170     }
171     else
172     {
173       if (exdata->fixedwidthfont)
174         fontstyle = "font-family: -moz-fixed";
175     }
176   }
177   else  // DELETEME: makes sense?
178   {
179     if (exdata->fixedwidthfont)
180       fontstyle = "font-family: -moz-fixed";
181   }

maybe that will help you find out what happen there. I just did a lxr search. 
jshin- maybe you can use debugger to set break point there to find out what 
happen.

Comment 18

[Frank Tang]

nhotta can you look at the "fixed font appears to be beyond end-user-control and
to be hard-coded somewhere" issue since you familar with the mime code. 

Is that also true on window ?

Comment 19

[nhottanscp]

This could be related to bug 91190, font of default locale is used for unicode
document (and mail is sent as UTF-8 to the layout).
Jungshik, could you copy the text in mail view and paste into mozilla HTML
editor? Then you should be able to see the source of what it's actually sent to
the layout.

Comment 20

[Ben Bucksch (:BenB)]

Compare bug 39402, which has a patch.

Comment 21

[Jungshik Shin]

As ftang suggested,I set a breakpoint at various places in 
mimetmpl.cpp, but
somehow gdb/ddd did not stop there for me to check various
values. I'll just add some diagnostic output statement there
and see what's going on. I'll also try Ben's patch for bug 39402.

As for nhotta's suggestion, when pasted into html editor(composer),
Korean-English mixed text was rendered rather nicely with
fonts I specified for Korean(Korean part) and Western(English
part) in pref. 
 

Comment 22

[nhottanscp]

The text from mail to layout is sent as UTF-8.
Jungshik, in mozilla HTML editor, could you switch to source mode and change a
META charset to UTF-8? That's actualy what it sent to the layout. Do you see any
difference?

Ben, there is a problem using unicode font (bug 91190) although it might not be
related to this bug. Using unicode font for mail view might produce an
undesirable result because the user is not expecting mails to be shown by using
unicode font.

Comment 23

[Jungshik Shin]

I figured out the cause of the trouble.  GetMailNewsFont()
mailnews/mime/src/mimemoz2.cpp invoked by mailnews/mime/src/mimetpla.cpp
as well as mimetpfl.cpp. (the former is used for plain 'text/plain'
and the latter is for 'text/plain; format=flow' or sth. like that.
All of my Korean mesgs are 'plain text/plain' and that's why
gdb/ddd never reached the breakpoint set in mimetpfl.cpp)
strips all but 'fontfamily' off the font name  specified by user for
fixed-width font in Edit|Pref.

I specified 'sun-gothic-ksc5601.1987-0' for fixed-width font for Korean
lang group, but GetMailNewsFont() removes foundry, charset and encoding
field and leaves only 'gothic'. Later in mimetpla.cpp / mimetpfl.cpp,
CSS spec. 'font-family: gothic' is added for text/plain mail rendering.
Further down the road when font-family is used to pick an actual font,
Mozilla uses  whatever font with 'gothic' in the second field of XLFD
its font-selection mechanism picks  (whatelse could it do?)  for both
English and Korean part. Therefore, what I specified for x-western and
Korean lang. group is completely ignored except for 'gothic' family name.


Under MS-Windows, this may not be of much problem because fontfamily
name in one lang group is rarely used for font family name in other
lang groups. It's also less problematic under MS-Windows because TTFs
like 'Mincho' (for Japanese), 'Ming' or 'Song' for Chinese, 'Batang'
for Korean have glyphs for US-ASCII part as well as for Japanese,
Chinese and Korean part of Unicode. That is, using 'Mincho' to render
both US-ASCII and Japanese characters is perfectly fine. It's even
desirable because US-ASCII glyphs in Mincho TTF shipped in MS-WIndows
are specially designed to go well with Japanese glyphs in Mincho TTF.

This is not the case for Unix (at least until TTFs are much more
widely used under Unix/X11).

What can we do? An easy (but perhaps not the best) fix would be NOT
call GetMailNewsFont() in mimetpla.cpp and mimetpfl.cpp and just use
the default 'font-family: moz-fixed' which should just work fine for
mail/news as it works fine for text/plain rendering of browser window
and <pre> ... </pre> part of text/html.

Comment 24

[nhottanscp]

I looked at the code.

1994 #if defined(XP_UNIX)
1995 // extract "courier" from "adobe-courier-iso8859-1"
1996 nsAutoString tmp;
1997 familyname.Right(tmp,
familyname.Length()-familyname.FindChar(PRUnichar('-'))-1);
1998 tmp.Left(familyname, tmp.FindChar(PRUnichar('-')));
1999 #endif

The Unix specific code above, this is a side effect of bug 45762.
Using the default 'font-family: moz-fixed' as you mentioned, that will disable
the capability of using different font per mail charset. We might be able to
also specify lang attribute.

Comment 25

[Jungshik Shin]

> Using the default 'font-family: moz-fixed' as you mentioned, that will disable
> the capability of using different font per mail charset. We might be able to
> also specify lang attribute.

  Yes, I agree. I realized that 'lang' has to be specified as well
after making my suggestion of using the default 'font-family: ...'
(for XP_UNIX)

Comment 26

[Katsuhiko Momoi]

> as well as mimetpfl.cpp. (the former is used for plain 'text/plain'
> and the latter is for 'text/plain; format=flow' or sth. like that.

This seems wrong to me. "text/plain" and "text/plain;format flowed"
should have nohting to do with font selection. It should only be
relevant to line-breaking code. Am I missing something?

Comment 27

[nhottanscp]

It is not, both calls the same code to get a font. I think it was just mentioned
because the break point was not hit in mimetpfl.cpp since the testing data was
not  format flowed.

Comment 28

[Jungshik Shin]

I'm gonna attach two patches.
The first one is very simple. When XP_UNIX is defined,
it sets font-family to -moz-fixed. To my surprise, it works just fine
although LANG attribute is not specified.

The second one tries to be more thorough. When XP_UNIX is defined,
it invokes a new function (added to mimemoz2.cpp) GetMailNewsLang()
and sets LANG attribute accordingly in addition to setting
font-family to -moz-fixed. 

Perhaps, a third approach might be add an argument to GetMailNewsFont()
so that it can sets langGroup in addition to fontName, fontSize, etc.This might
be better than the the second because the second one doesn't
take care of 'font-size' for Unix/Linux.

Comment 29

[Jungshik Shin]

Attachment: a simpler patch

Comment 30

[Jungshik Shin]

Attachment: a more complex patch

Comment 31

[nhottanscp]

Jungshik, thanks for the patch.
I would like to see if 'lang' is really used for the font selection.
Your second patch specified font family as -moz-fixed and lang. And as you
mentioned, the first patch which only specified -moz-fixed also works.
Could you try to use the font name by the original function (instead of
-moz-fixed) then specify the lang info with it (i.e. your third proposal)? If
that works then we want to do that way for all platforms instead of Unix only.

There is an issue about specifying a lang. We internally use non standard names
like "x-western". If it is possible to distinguish them then we want to avoid
using them.

Comment 32

[Jungshik Shin]

Attachment: a third patch: -moz-fixed,fontsize,lang

Comment 33

[Jungshik Shin]

Attachment: 4th patch: fontname,fontsize,lang

Comment 34

[Jungshik Shin]

I've just attached two more patches. Here are break-downs of what each of
four patches does in terms of the value of the variable
openingDiv in mimetpla.cpp / mimetpfl.cpp after returning from
GetMailNewsFont() while processing an email
of 'C-T: text/plain; charset=EUC-KR'
when monospace font for Korean lang group is
set to -daewoo-gothic-ksc5601.1987-0 

Without patch (current tree)

<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-family: gothic; font-size: 16px;">
<pre wrap>

patch1 (attachment 53063 [details] [diff] [review]):
<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-family: -moz-fixed;">
<pre wrap>

patch2 (attachment 53064 [details] [diff] [review])
<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-family: -moz-fixed;" lang="ko">
<pre wrap>

patch3 (attachment 53264 [details] [diff] [review])
<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-family: -moz-fixed; font-size: 16px;" lang="ko">
<pre wrap>

patch4 (attachment 53265 [details] [diff] [review])
<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-family: gothic; font-size: 16px;" lang="ko">
<pre wrap>

I was inclined to go for patch3 (that was what I meant by
a third approach in my previous
comment, but I was not clear enough and you apparently
thought I meant patch 4). The rationale behind it is:

  - I thought -moz-fixed along with lang attribute
    would 'automatically' select lang-dependent fonts
    as specified in user.pref. That is, for 'text/plain; charset=EUC-KR',
    I expected Mozilla to 
    use font.monospace.ko and font.monospace.x-western
    to render Korean portion and US-ASCII portion respectively.     And for
'text/plain; charset=iso-2022-jp', font.monospace.ja
    and font.monospace.x-western were expected to be used.

  - family name like 'gothic' is very ambiguous in Unix/X11 as I wrote before.
    On my system, there are 4 different Korean fonts sharing the familyname
    'gothic'. When I pick one of them for font.monospace.ko,
    I like it to be used in rendering of 'text/plain; charset=EUC-KR'
    email messages along with the font specified
    for font.monospace.x-western. If I go with patch 4 or without any
    patch at all, I do not have any control over which of four 'gothic'
    fonts is actually used. Even worse is that  
    Mozilla uses a gothic font for rendering of US-ASCII part as well. 
    End-users would be puzzled at coming across this font which they 
    never specify in Edit|Pref|font as I was when reporting this bug
    for the first time.
    
    Again as I wrote before this can be
    a good thing under MS-Windows where fontfamily name is pretty
    much uniq and US-ASCII glyphs in CJK fonts are better at getting
    along with CJK glyphs than US-ASCII glyphs in non-CJK fonts.
    However, given a present state of Unix/X11
    (esp. on Linux boxes), this can lead to very ugly rendering of
    US-ASCII part.  

  - Therefore, I think langgroup-based fine control of fonts by users
    is necessary under Unix/X11.

The third approach(and the second) appeared to work well when I tried
it for 'text/plain; charset=EUC-KR'. When I changed font.monospace.ko
and font.monospace.x-western, the change was reflected in rendering.

I also looked at some 'text/plain; charset=iso2022-jp' and 
't/p; charset=gb2312' emails. At first, they looked fine
and change in font.monospace.ja and font.monospace.zh-CN were
reflected in rendering. However, a closer look showed me that 'lang' 
attrib. did not work. Somehow glyphs from font.monospace.ko
are used where it has glyphs for characters. Only when
characters are not covered by font.monospace.ko, glyphs
are drawn from font.monospace.zh-CN or font.monospace.ja. 
Actually, Mozilla didn't use a single glyph from
font.monospace.ja because all characters in my
sample Japanese emails  *happened* to be  covered by
font.monospace.ko and font.monospace.zh-CN. It seems like
the order of glyph search is 1. ko, 2. zh-CN 3. ja. More importantly,
the order is NOT affected by lang attribute.     

  Now I've got a couple of questions. The glyph search order
should be dependent on something, but I can't figure out
on what. I suspected my locale setting(LC_ALL=ko_KR.EUC-KR) 
under which Mozilla was launched. I set LC_ALL to C, but the order remained
the same. Can my language pref. setting in Preference|Navigator influence
the glyph search order? I haven't played with it, yet. 
More important question is whether this glyph search order
is dependent on lang attribute. I believe this should depend
on lang, but my test result showed me otherwise. If glyph
search order can be made dependent on 'lang', my third approach
should work well. 

One more test which can shed some light on this issue is to put up
a html file with the same CSS spec. listed at the beginning
and to see how Mozilla renders it in a browser window.   

Comment 35

[Jungshik Shin]

Attachment: Japanese email screenshot with the 3rd patch

Comment 36

[Jungshik Shin]

Attachment: GB2312 email screenshot with the 3rd patch

Comment 37

[Jungshik Shin]

attachment 53273 [details] and attachment 53274 [details] were taken with
patch 3 in and with
font.monospace.ja=mnkaname-fixed-jis0208.1983-0,
font.monospace.zh-CN=isas-song ti-gb2312.1980-0, 
font.monospace.ko=daewoo-gothic-ksc5601.1987-0, and
font.monospace.x-western=adobe-courier-iso8859-1.

Most Kanjis in iso-2022-jp message were rendered with
daewoo-gothic and only one that's not covered
by daewoo-gothic (marked with the red underline)
was rendered with isas-song ti. The same is true
of Hanzis in the GB2312 message. Only Hanzis
not covered by daewoo-gothic(marked by red circles) were rendered with
isas-song ti font while those covered by daewoo-gothic
were rendered with it.  
This is to show the glyph search order does not depend on 'lang' attribute.

BTW, all Latin glyphs were rendered with adobe-courier.

Comment 38

[Jungshik Shin]

The font 'chaos' in Linux may get resolved sooner or later
and it may be better not to hard-code anything assuming this 'chaos'.
Therefore, we may replace XP_UNIX #if with what Ben suggested
in bug 39402 in my patch #3. 

Still lang-dependent glyph search order
is very much desired. Is there a bug for this?

Comment 39

[nhottanscp]

The lang-dependent glyph search order, we thought that was working.
But it is not working as desired according to your result. Please file a bug
with your data.
About the third patch, I think Unix specific code should be localized to
GetMailNewsFont which could return a null for a font name and the caller can
handling that case differently. I am not sure what this is related to bug 39402,
I think that is asking not to specify any font info in libmime.

Anyway, we need to think about which way to go. I mean, fix gfx to implement
lang support for font selection and use the #4 patch, or use the work around it
by #3.

BTW, have you seen similar problem with text/html? The third patch addresses
plain text messages. If the problem is generic then we need a similar solution
for text/html also.

Comment 40

[Jungshik Shin]

Attachment: mail rendering simulation with html

Comment 41

[Jungshik Shin]

Attachment: screenshot of rendering of attachment 53478 by mozilla

Comment 42

[Jungshik Shin]

Attachment: patch #5 : (patch #3 + #4)

Comment 43

[Jungshik Shin]

I've just uploaded my patch #5. I think it addresses nhotta's concerns
althought not in the way he suggested.
 
It combines patch #3 and #4. How?
Users can choose between #3 and #4 behavior with
'mailnews.use_default_fontfamily (perhaps not the best name.
default_fontfamily means '-moz-fixed').
With it set to true, Mozilla behaves as if patch #3 is used.
By default, it behaves as if patch #4 is used (which is
almost identical to what it behaves without any patch at all).

The rationale for this patch and user-specifiable control
of the behavior is that the number and kind of available fonts
in Unix/X11/Linux  vary widely across platform/distribution/OS
version/users.  A behavior desired by one user may not be
acceptable at all to other users. Therefore, instead
of hard-coding it, I think it's better to let users choose. 

Why patch #4(or the current status without any patch) is the default?

Because under MS-Windows (and maybe or maybe not MacOS), 
font family specified for a language group (e.g. Mincho for Japanese)
can be used to render all characters in documents belonging to 
the lang. group regardless of whether they're Japanese or Latin.
This is the case because under MS-Windows, CJK ttfs include
glyphs for Latin characters as well as CJK characters.
Under Unix/X11, this is not the case yet but it's moving toward
that direction ( e.g. '*-gulim-ksc5601.1987-0' for Korean
has the matching/corresponding '*-gulim-iso8859-1'). 

Moreoever, unless we fix the problem of 'lang' attribute NOT being
honored in font-selection (see my previous comment
and attachment 53479 [details] and attachment 53478 [details]), patch #3 behavior
can lead to use of lang-insesitive-mixed  fonts. 

However, patch #3 behavior is sometimes necessary for some users
with some set of fonts (e.g. my Linux box). For instance,
some users do not want a '-*-gothic-iso8859-1' font
to be used for US-ASCII portion of CJK text when 
they have font.monospace.x-western=adobe-times-iso8859-1
and font.monospace.[ja,zh-CN,zh-TW,ko]=*-gothic-[jisx0208.1990,gb2312.1980,
big5,ksc5601.1987]-0. They just want/expect adobe-times to be
used to render US-ASCII portion of CJK text. 

They can also want to have a finer control over font selection.
When they have several '-*-gothic' fonts for Korean, they don't
want Mozilla to pick one of them but they want the exactly
the one they specify ('-daewoo-gothic'). 

Those users can set 'mailnews.use_default_fontfamily' (again,
this is not the best name and even can be misleading) to true.


Finally, we definitely have to look into
and fix the font selection mechanism so that
'lang' attribute is taken into account. It's clear from
my screenshots (attachment 53479 [details] : simulating
patch #3 behavior or #5 behavior with mailnews.use_default_fontfamily
set to true) that 'lang' attribute
is not honored by the font selection routine. 
About half of Hanzis (covered by KS C 5601) in zh-CN section 
is rendered with
'-daewoo-gothic-ksc5601.1987-0'(green) while the other half
of Hanzis (NOT covered by KS C 5601)
are rendered with '-isas-song ti-gb2312.1980-0' (red).
In ja section, most Kanjis are rendered with -daewoo-gothic,
but one Kanji (not covered by KS C 5601/KS X 1001 but
covered by both GB 2312-1980 and JIS X 0208) is 
rendered with 'isas-song ti'. And, one Kanji
(only covered by JIS X 0208) is rendered with
'kappa-mincho-jisx0208.1990-0' (blue).

I have little idea why this order of font search order
(1. ko, 2. zh-CN, 3. ja) is used
by Mozilla. I changed my language preference
in Pref|Navigator|Language (moving down ko to the
bottom and moving ja or zh-CN to the top), but
this didn't make any difference.

I thought it might be foundray names sorted in
alphabetical order. 'daewoo' comes
first and 'isas' and 'kappa' follow it.
However, this turned out not to be the case, either.
When I changed font.monospace.ko to 'sun-gothic',
it (alphabetically the last) just took precedence over 'isas' (zh-CN)
and 'kappa'(ja). 

I guess I have to look into nsFontMetricsGTK.cpp as
nhotta suggested. 

Comment 44

[Frank Tang]

jshin:
I think patch 4 is the right thing to do. I don't think patch 5 is a good idea 
becaues it make it too complex. I recognize the need (or the motivation ) to 
push you to creaet patch 3 or 5. But I think that need should not be addressed 
here, but need to be put into the Linux GFX system. Please try to push patch 4 
into the tree for now. And work with bstell/ shanjian to find out a way to 
address the "same face name from different foundary mean different face" issue.

Comment 45

[Frank Tang]

see 104881 for the spin off issue.
jshin- can we focus on patch 4 in this bug? I believe it will address the basic 
issue. And let's file other bugs to address the "give uesr more control" issue. 

Comment 46

[nhottanscp]

I filed bug 105171 for the issue about internal lang names like "x-western".
Jungshik, do you agree with going with #4 patch?

Comment 47

[Jungshik Shin]

I'm afraid it's a tough call to make especially given a chaotic situation
for fonts under X11. It can be argued that dealing with this issue in
gfx is  better than dealing with it mailnews/mime.   However, there are
a couple of questions to ask before making a decision:

 - Can the loss of information in GetMailNewsFont() (foundry
   and encoding name stripped away)  be made up for
   later in gfx?  Adding 'lang' attribute would certainly
   help and it may be possible. But, is it desirable?
   Down the road, gfx may have to go over again
   what's already done in GetMailNewsFont() (namely,
   retrieving font.*.${lang}, and figuring out
   charset and matching it with 'lang') to make
   up for the loss of info.  This repetition may be doable,
   but how is gfx supposed to distinguish between
   mail/news rendering for which this repetition IS necessary
   and  ordinary text/html  rendering for browser window
   for which this is NOT necessary?

 - What exactly do we want Mozilla to do in terms of mail/news
   rendering?

My answer to the second question is that plain text mail/news should
be rendered as if the whole message body is enclosed by <pre> tag in
text/html. With lang-dependent font search order in place and working
(which is NOT at the moment) in GFX, patch #3/#5 should make mail/news
rendering work the way I think it's done.

Of course, if the answer to the second question is different from mine,
your answer as to what to do with this bug is also different from mine.
That's why I came up with patch #5. By default, with that patch, Mozilla
works the way it does now (except that 'lang' attribute is added),
but if there's anyone who thinks the same way as I do, (s)he can have
her/his way.

The answer to my second question can  also be platform-dependent.
Under MS-Windows (and probably MacOS), by selecting font-family for
ja/zh-TW/zh-CN/ko, users usually expect Mozilla to use that font-family
for all characters regardless of whether they're Latin, Cyrillic or
Japanese/Chinese/Korean. On the other hand, Unix users may not mean that
and they expect Latin characters to be rendered with font specified
for Western and CJK characters to be rendred with font specified for
CJK lang. group. To make things more complicated, this preference of
Unix/X11 users can change over time as more consistent font-availablity
becomes the norm rather than the exception on that platform. This is
another reason for patch #5.

  BTW, gfx *does* regard fonts with identical family name but with
different foundry name as different (unless Brian's patch to bug 94327
which introduce Anyfoundry-Family-* is checked in). However, mailnews
hands over to gfx *only* family name (without any patch) or family name
and lang (with  patch #4) after stripping away foundry and character
set/encoding. In that case, gfx has to do extra work (which is already
done  in GetMailNewsFont()) to get back lost info as I wrote above
(in my first question).

Comment 48

[Jungshik Shin]

I filed bug 105199 for glyph-search-order not dependent on lang attribute.

Comment 49

[Jungshik Shin]

I made patch #6, a variant of patch #4. In addition to 
font-family and font-size, it specifies x-foundry as
below:

<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-family: gothic; font-size: 16px; x-foundry=daewoo" lang="ko">
<pre wrap>

when font.monospace.ko=daewoo-gothic-ksc5601.1987-0.

Since currently gfx does not know anything about x-foundry, 
Mozilla behaves exactly the same way as with patch #4. However,
if we modify gfx later to refer to x-foundry when picking fonts
to use, patch #6 seems to be a good choice. 

I'm still not sure abuot what Unix/X11 users want given a chaotic
font situation in Unix/X11. Therefore, I'm also considering
making a hybrid of patch #6 and patch #3 (to be called
patch #7) just like patch #5
is a hybrid of patch #4 and patch #3.

If we decide to go with either patch #6 or yet-to-be-made patch #7,
we have to modify gfx to make use of 'x-foundry'. In that case,
bug 104881 ftang filed has to be reworded accordingly. 
Moreoever, needless to say, we have to work on bug 105199 
whichever patch we may decide to use. 

BTW, I set font.monospace.ja=kappa-mincho-jisx0208.1990-0
and found that a Japanese email mesg (in ISO-2022-JP) is
rendered with daewoo-mincho-ksc5601.1987-0 (without patch,
with patch #4 and #6 and with patch #5 but with mailnews.use_default_font_family
off ). Only characters not present in daewoo-mincho  font are
rendered with kappa-mincho. This is an expected result and yet another
consequence of non-lang depdent glyph search order.

Comment 50

[Jungshik Shin]

Attachment: patch #6: add x-foundry : a variant of patch #4

Comment 51

[nhottanscp]

We may remove the font-family attribue if lang works.
I agree that plain text mail should be rendered as using <pre>. I think that is
already described in "moz-text-plain", so once the lang attribute works, it is
supposed to work like that. So I don't think we need an extra x-foundry here.

Comment 52

[Jungshik Shin]

> We may remove the font-family attribue if lang works.

  We can go without calling GetMailNewsFont() (or we have
to streamline it a bit), but we still
have to take into account Preference|Mail&News|Display | Plain Text Disp. font.
That is, depending on  obj->options->variable_width_plaintext_p
in mimetpla.cpp (exdata->fixedwidthfont in mimetpfl.cpp)
we can go with the default font-family (by not specifying font-family)
or with 'moz-fixed'. 
  
> I agree that plain text mail should be rendered as using <pre>. I think that is
> already described in "moz-text-plain", so once the lang attribute works, it is
> supposed to work like that. So I don't think we need an extra x-foundry here.

 I thought about adding something very similar at the end of my last
comment, but at the last moment I decided not to. Why? I wasn't sure
how Mozilla should interpret and use the values of
font.{monospace,sans,serif}.$langgroup.  Considering it again, I think
that's a good idea. 

  Summing up, what we can do here on *all platforms* is :

 If fixed width font is specified in Pref|Mail&News|Display|Plain Text
rendering, use 


<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-family: -moz-fixed; font-size: 16px;" lang="ko">
<pre wrap>

Otherwise, use

<div class="moz-text-plain" wrap=true graphical-quote=true
style="font-size: 16px;" lang="ko">
<pre wrap>

Then, make the style system do the job the right way. 

This is similar to patch #3 but without XP_UNIX dependency.

If we can agree on this, we have to wait until bug 105199 (lang attribute
issue) is fixed. Otherwise, Japanese emails would be rendered
with Korean fonts if Mozilla is running under Korean locale and vice versa.
(see my commment to bug 105199 about locale
taking precedence over lang atrribute in font selection)

Comment 53

[ji]

Nominating for nsbeta1, since it's a regression from 6.1 and it's very bad for
Korean mail display on linux. 

Comment 54

[Jungshik Shin]

Thanks for the nomination :-)
If you run Mozilla under Korean locale, 
Japanese or SC and TC mail messages get rendered
badly as well depending on what fonts you have
on your system. The same is true of Mozilla
launched under SC or TC locale rendering
Japanese and Korean emails. Anyway, 
I believe this bug can be fixed
rather easily when bug 94327, bug 104075 and 
bug 105199 get fixed. (fixing
bug 104075 alone could help a lot in many cases) 

Comment 55

[nhottanscp]

nsbeta1+

Comment 56

[nhottanscp]

The change in mail code doesn't have to wait for bug 105199 fix. I will start
the review process for the 4th patch (id=53265).

Comment 57

[Jungshik Shin]

Attachment: a new patch (a variant of patch#3)

As I wrote before, I'm more inclined to go with
patch #3 than patch #4 because I think
once bug 105199 is fixed, there's no need
to specify font-family other than the generic
moz-fixed. This new patch is different from
patch #3 in that on all platforms(not only
on Unix) it 
uses the generic font-family. Thus,
a part of GetMailNewsFont() to read out
fontfamily from pref. is gone in this patch.

Basically, what it does is put
the following :

 IF fixed-font is specified to be used for
     mail/news rendering, add the following
   
     <div class="moz-text-plain" wrap=true graphical-quote=true
     style="font-family: -moz-fixed; font-size: 16px;" lang="ko">
     <pre wrap>

  ELSE
      <div class="moz-text-plain" wrap=true graphical-quote=true
       style="font-size: 16px;" lang="ko">
      <pre wrap>

Comment 58

[Jungshik Shin]

My last patch worked as intended in a sense along 
with Shanjian's patch for bug 105199.

Why in a sense? 

There's a very strange problem unrelated with my
patch that caused my patch not to work.  
In mimemoz2.cpp, charset is always set to "us-ascii"

1937 nsresult GetMailNewsFont(MimeObject *obj, PRBool styleFixed, char
*fontName, PRUint32 nameBuffSize, 
1938 PRInt32 *fontPixelSize, PRInt32 *fontSizePercentage)
1939 {
1940 nsresult rv = NS_OK;
1941 1942 nsIPref *aPrefs = GetPrefServiceManager(obj->options);
1943 if (aPrefs) {
1944 MimeInlineText *text = (MimeInlineText *) obj;
1945 nsCAutoString aCharset;
1946 PRUnichar *unicode = nsnull;
1947 nsCAutoString convertedStr;
1948 nsCAutoString variableFontType;   // serif, sans-serif
1949 
1950 // get a charset
1951 if (!text->charset || !(*text->charset))
1952 aCharset.Assign("us-ascii");
1953 else
1954 aCharset.Assign(text->charset);

It used to work fine, but somehow text->charset
is NULL for SC, J and K messages and later langGroup
is set to x-western because charset is 'us-ascii'.

Therefore, to see my code work, I have to hardcode
charset to 'gb2312', 'iso-2022-jp', and 'euc-kr'
in place of 'us-ascii'. 

I'm wondering if there's any known regression
of this kind from recently landed patches.
If that's tracked down and fixed and we agree
on which one to use, my last patch or #4,
we can go ahead. 

Comment 59

[nhottanscp]

>It used to work fine, but somehow text->charset
>is NULL for SC, J and K messages and later langGroup
>is set to x-western because charset is 'us-ascii'.
This might be related to shanjian's change, add him to cc.

Comment 60

[Shanjian Li]

Jungshik, 
the problem you reported is addressed in bug 112904. 

Comment 61

[Jungshik Shin]

I sort of tracked down the cause of charset not being
set by the time it's refered to in mailnews/mime/src/mimemoz2.cpp.
The cause is that the way MimeInlineText (or MimeInlineTextPlain) is 
initialized has changed by
Shanjian's patches for bug 12481 and bug 107084 since mid-October
when I tested my patch. 

MimeInlineText_initialize used to set charset
before October 23, but it doesn't do it any more.  It's now
done by MimeInlineText_initializeCharset which has to be
*explicitely* invoked. It's not yet invoked when 
GetMailNewsFont() in mimemoz2.cpp
tries to use the value of charset so that it's null. 

A quick-fix might be just to call MimeInlineText_initiaiize()
in mimemoz2.cpp after making it available to mimemoz2.cpp,
 but I'm not sure what raminification that
will have. 

Comment 62

[Jungshik Shin]

Shanjian,
Thank you for quickly fixing this. 
What a coincidence ! Or, is it inevitable? :-)
I've just come up with almost identical patch and it seems to work
fine.

Comment 63

[Jungshik Shin]

Naoki,

Can you review the last patch if you agree that's better than
patch #4? With 105199 fixed, just using the generic font-style
appears sufficient. 

Comment 64

[nhottanscp]

So the latest patch has the change of using "-moz-fixed" in addition to the lang
attribute support. Anything equivalent for variable width case? Then we could
remove the face argument altogether.

Comment 65

[Jungshik Shin]

> So the latest patch has the change of using "-moz-fixed" in addition to the lang
> attribute support. Anything equivalent for variable width case? 

  For variable width case, it's not specifying 'font-family' in "<div .... ...>".
I searched for the equivalent of '-moz-fixed' pseudo-font-face for variable
width case, but I couldn't find any.  It seems like just not specifying 
'font-family' is equivalent to specifying '-moz-fixed' for fixed width case.
 

> Then we could remove the face argument altogether.

 Yes, I had that in mind, too. However, I wasn't sure if it's all right
to remove the rest of the code in GetMailNewsFont() which deals with
'non-lang-sensitive' case. Do we still use mailnews.font.name.html
and mailnews.font.name.plain (in pref*.js)?  Lyx search yielded no reference to them
other than mimemoz2.cpp. How about 'mailnews.language_sensitive_font'? 
It's on by default, but I suspect everybody wants it. (well, maybe not
everyone.....)

  I'm talking about the following part in GetMailNewsFont():

1966 // if indicated by the pref, do language sensitive font selection
1967 PRBool languageSensitiveFont = PR_FALSE;
1968 rv = aPrefs->GetBoolPref("mailnews.language_sensitive_font",
&languageSensitiveFont);
1969 if (NS_SUCCEEDED(rv) && languageSensitiveFont) {
      snip........
2042 }
2043 // otherwise, use the mailnews font setting from pref
2044 else {
2045 2046 // get a font name from pref, could be non ascii (need charset conversion)
2047 // this is not necessary if we insert this tag after the message is
converted to UTF-8
2048 rv = aPrefs->CopyUnicharPref(!styleFixed ? "mailnews.font.name.html" :
"mailnews.font.name.plain", &unicode);
2049 if (NS_FAILED(rv))
2050 return rv;
2051 
2052 rv = nsMsgI18NConvertFromUnicode(nsCAutoString("UTF-8"),
nsAutoString(unicode), convertedStr);
2053 PR_FREEIF(unicode);
2054 if (NS_FAILED(rv))
2055 return rv;
2056 
2057 if (convertedStr.Length() >= nameBuffSize)
2058 return NS_ERROR_FAILURE;
2059 
2060 PL_strcpy(fontName, convertedStr.get());
2061 2062 // get a font size from pref
2063 rv = aPrefs->GetIntPref(!styleFixed ? "mailnews.font.size.html" :
"mailnews.font.size.plain", fontPixelSize);
2064 if (NS_FAILED(rv))
2065 return rv;
2066 
2067 }


 

Comment 66

[nhottanscp]

I think variable width is used for text/html.
I agree there are some code currently unused.
For this bug, I prefer to added a minumum change required then do the clean up 
and other experiments separately. Or do you think we need to add the 
'-moz-fixed' change to fix this bug?

Comment 67

[Jungshik Shin]

> I think variable width is used for text/html.

  Do you think that  given the following pref. setting,
the case A and the case B will be rendered differently:

  font.default:  serif
  mailnews.language_sensitive_font: true
  font.name.monospace.ko: fdry1-gothic-ksc5601.1987-0
  font.name.serif.ko: fdry1-myeongjo-ksc5601.1987-0
  font.name.sansserif.ko: xxxx
  mail.fixed_width_messages: false

  font.name.serif.x-western: adobe-times-iso8859-1

1. text/html

 A.
  <div class="moz-text-html" style="font-family: myeongjo;" lang="ko">
  .........
  </div>

 B.
  <div class="moz-text-html" lang="ko">
  .......
  </div>


  I thought they would be rendered identically as far as characters
covered by KS C 5601 font is concerned *provided* that there's only one
'myeongjo' (if there are two 'myeongjo-ksc5601.1987-0' with different
foundries and they're different, it could be problematic.)
Even without font-family specified, I thought the font system, with
the help of lang and font.default, picks up the font specified by
font.name.serif.ko, which is 'myeongjo'.



For US-ASCII part, A and B could lead to a different rendering.
If font.name.serif.x-western is not 'myeongjo' (as in the example
above) but there IS a 'myeongjo' font that covers US-ASCII part
(iso8859-1), A. will get all the text  rendered with 'myeongjo'
(-myeongjo-ksc5601.1987-0 and -myeongjo-iso8859-1) while in B. US-ASCII
part will be rendered with -times-iso8859-1. If font.name.serif.x-western
is not 'myeongjo' and there is NOT a 'myeongjo' font that covers US-ASCII,
A. and B. will give the identical result.

Here are four more comparisons:

2. text/plain, flowed, variable
 A.
  <div class="moz-text-flowed" style="font-family: myeongjo; font-size=16px;"
   lang="ko">

 B.
  <div class="moz-text-flowed" style="font-size=16px;" lang="ko">

3. text/plain, flowed, fixed
 A.
  <div class="moz-text-flowed" style="font-family: gothic; font-size=16px;"
   lang="ko">

 B.
  <div class="moz-text-flowed" style="font-family: -moz-fixed; font-size=16px;"
   lang="ko">

4. text/plain, variable
 A.
  <div class="moz-text-plain" style="font-family: myeongjo; font-size=16px;"
   lang="ko">

 B.
  <div class="moz-text-plain" style="font-size=16px;" lang="ko">

5. text/plain, fixed
 A.
  <div class="moz-text-plain" style="font-family: gothic; font-size=16px;"
   lang="ko">

 B.
  <div class="moz-text-plain" style="font-family: -moz-fixed; font-size=16px;"
   lang="ko">

> For this bug, I prefer to added a minumum change required then do the clean
> up  and other experiments separately.

 As for cleaning up the unused code (e.g. 'non-lang-sensitive'
branch in GetMailNewsFont()), either way is all right for me.  If you
prefer to leave some unused code as they're for now, it's fine.

> Or do you think we need to add the  '-moz-fixed' change to fix this bug?

  As for this part, I guess we need to go with that (that is,
if I have to choose between patch #4 and the latest patch, I'd
choose the latter.). That is, I think removing 'font selection' code
(for lang-sensitive-case) in GetMailNewsFont(), using  '-moz-fixed'
for fixed-style and not specifying font-family for variable style are
necessary. Because otherwise mozilla may pick a  font *different* from
what users specify in font.name.(monospace|serif|sansserif).ko down the
road if there are multiple fonts with the same family/face name but with
different foundries and possibly radically different looks.  Specifying
'lang' may be an insurance against this possibility, but I'm not sure.
If 'lang' can override 'font-family' spec., font-family-selection code
in GetMailNewsFont() is redundant, isn't it?

Comment 68

[nhottanscp]

>If 'lang' can override 'font-family' spec.
I am not sure about this.

Any, I will let you hanlde this since you are providing the patches. I prefer
the smaller fix but I am also fine with the latest patch.
Please ask the module owner (ducarroz) for the review. Also please make sure not
to check in before the depending bug is fixed if you take the last patch.

Comment 69

[Jungshik Shin]

ducarroz,

Could you review my latest patch? 

Naoki,
As you wrote, I'll make sure that bug 105199 and bug 112904 are resolved before
checking in my patch. 

Comment 70

[Jungshik Shin]

 First of all, in my bug report and comments  I misused the term
CSS (property) when refering to 'lang' attribute for html tags like
div,span,p and so forth. This misuse of mine may have something to do
with what you don't like (adding pseudo-css property?) in Shanjian's
patch.

DB>But is there any need for this pseudo-property to be parsed?
SJ>Yes, definitely. Whenever we are selecting any font, we always 
SJ>need to know what 
SJ>language we are dealing with. That allow us to choose the right font
SJ> for that language.

DB>So you're proposing a new CSS property because we don't use the CSS2 font
DB>selection algorithm correctly so things are broken?  If we use the CSS2 font
DB>selection algorithm correctly it should be able to solve most of
DB> these problems by specifying fonts in the correct order.  

  Did you mean that CSS2 font selection algorithm is currently broken
in Mozilla or that it's not broken but  Shanjian's patch assumed that
it's broken and bypassed it?  If the latter is the case, building
a *lang-dependent* virtual font-family list  may be a solution.
(I suspect that Mozilla builds these 'internal' 'virtual' lists for
langs or character codings it support at the start-up  for a couple of
Mozilla's internal classes like '-moz-fixed' and what Shanjian's patch
does is to activate one of them according to the value of 'lang')


 :lang(zh-TW) { font-family: trad-chinese-font, x-western-font,.....}
 :lang(zh-CN) { font-family: simp-chinese-font, x-western-font,.....}
 :lang(ja) { font-family: ja-font, x-western-font,.....}
 :lang(ko) { font-family: ko-font, x-western-font,.....}

However, there's a problem with this in X11 unless
this font names in font-family list include not only font-face but also
foundry, character set and encoding (the first two and the last two
elements of XLFD. See the footnote 1 below)

DB>Or do I not understand the problem correctly?

  I'm not sure  whether Shanjian's way of fixing this bug is most
desirable.  You're certainly a much better judge in CSS related issues
than I'm.  However, I think you probably agree  that HTML 'lang' attribute
specification should be utilized to help font selection  in *absence*
of sufficient information provided via CSS.

What would you say Mozilla should do when rendering a document like
http://www.unicode.org/iuc10/x-utf8.html ?  Whether that's right or
wrong, there are documents with 'lang' attribute specified but without
font specified via CSS. I believe when 'font-family' is not specified
in CSS, Mozilla kinda build a virtual font-family list to use. 

  The question here is what fonts go there in what order and what
considerations have to be given when building it. One of factors to
consider is 'lang' attribute, isn't it?  The locale under which Mozilla
is launched and the encoding (character coding) of the currrent document
are considered but 'lang' attribute is out of the equation. 
The result of not taking into account 'lang' attribute leads
to a very ugly rendering as shown in attachment 53479 [details]. 

DB>Could you explain very clearly what the problem that you're trying
DB> to fix is,

 Given a html snippett like this (with no font specification
in CSS), what does Mozilla have to do?  An example of this 
(misnamed!!) is given at the URL in the URL field and
another is at <http://www.unicode.org/iuc/iuc10/x-utf8.html>.

<div lang="zh-CN"> Simplified Chinese </div>
<div lang="ja"> Japanese</div>
<div lang="zh-TW">Traditional Chinese</div>
<div lang="ko">Korean</div>

When 'font-family' is not specified, I believe 'lang' attribute has to
be refered to to build a 'virtual' font-family list picking fonts
from user pref. files. For <div lang="zh-CN">, the first font
in this virtual font-family list should be a font specified for
Chinese(Simplified)  and for <div lang="ja">, it should
be a font for Japanese. 

Instead of doing this, Mozilla without Shanjian's patch depends on the
character coding(encoding) of the document and the locale under which
Mozilla is launched. If the character coding(encoding) of the document is
not 'unicode', that determines what fonts go into the 'virtual' font list
in what order. In case it's unicode, the locale is used to pick fonts.
If Mozilla is launched under zh-TW locale, for all four <div>'s above,
the first font in the virtual font-family list is always a font for
traditional Chinese regardless of 'lang'. It's a font for Korean if
Mozilla is launched under ko locale. attachment 53479 [details] clearly shows
this problem.
 

DB> why you need to add a new CSS property that you'd expect all authors
DB> to add to their stylesheets to fix it?

   Not all authors are expected to add 'pseudo-css' lang property to their
stylesheets.  To facilate lang-dependent font selection, they *only* have
to add 'lang' attribute to html tags such as <div>, <p>, and <span>.

  
footnote 1:
In Unix/X11, it's not uncommon that a single font-face/font-family name
is used for multiple fonts with sometimes drastically different looks. In
addition, there may be multiple gothic fonts with different character set
and encoding. In an ideal world, those fonts should all have identical
look and feel and be interchangeable, but the reality is not that simple.
(see my comments for bug 102623 and bug 94327)

Comment 71

[Jungshik Shin]

Ooops. My last comment is supposed to be posted to bug 105199. Sorry for
cluttering up
your mailbox. 
 

Comment 72

[Jean-Francois Ducarroz]

Comment on attachment 59656 [details] [diff] [review]
a new patch (a variant of patch#3)

Looks good. R=ducarroz

Comment 73

[(not reading, please use [e-mail] instead)]

Comment on attachment 59656 [details] [diff] [review]
a new patch (a variant of patch#3)

instead of passing through pointers to buffers and the buffer sizes, any reason
why we shouldn't pass through references to nsCStrings?  in addition to
preventing  potential buffer overruns, you can simplify the code (you don't ned
to check if the convertedStr length and you can check IsEmpty() instead of
checking the first char of a string for \0

also, this should be reviewed by nhotta.

Comment 74

[Jungshik Shin]

Thank you for 'sr'ing my patch and your comments.

(From update of attachment 59656 [details] [diff] [review])
> instead of passing through pointers to buffers and the buffer sizes,
> any reason why we shouldn't pass through references to nsCStrings?

  Not that I know of. I wanted to minimize changes unless absolutely
necessary. That was also, I believe, what Naoki wanted. Therefore,
I didn't make any 'structural'(?) change.

> in addition to preventing  potential buffer overruns, you can simplify
> the code (you don't ned to check if the convertedStr length and you can
> check IsEmpty() instead of checking the first char of a string for \0

  Anyway, I'm attaching a new patch based on your suggestion.
  I couldn't test it because the latest source
  doesn't work with IMAP server I fetch my messages from.
  I'll try again later.

> also, this should be reviewed by nhotta.

  Actually, he had been paying close attention to all of my patches
including the latest one you reviewed. He didn't put up his name
as a reviewer, but effectively he reviewed my patches before
ducarroz reviewed it.

Comment 75

[Jungshik Shin]

Attachment: yet another patch per sspitzer's comment

basically identical to attachment 59656 [details] [diff] [review]
as far as Mozilla's apparent behavior
is concerned. However, internally
some simplifications and streamlining
have been done per sspitzer's comment.
I also removed several lines in GetMailNewsFont()
which reads in variable font type from pref. file.
variableFontType is not any more in GetMailNewsFont().

Comment 76

[Jungshik Shin]

Despite a strange problem with IMAP mail fetching
(which I checked is not due to my patch),
I managed to test my newest patch. It worked as intended
(i.e. the same manner as my second newest patch
of late November ducarroz reviewed).
  
ducarroz and sspitzer,

Could you take a look at attachment 62910 [details] [diff] [review] ?
This introduced rather drastic changes and
Naoki may have some reservation about it
for reasons unknown to me. 
If that's likely, we have to wait for him to 
be back. 

Comment 77

[nhottanscp]

-      rv = nsMsgI18NConvertFromUnicode(nsCAutoString("UTF-8"),
nsAutoString(unicode), convertedStr);
+      rv = nsMsgI18NConvertFromUnicode(nsCAutoString("UTF-8"),
nsAutoString(unicode), fontName);

The charset conversion above can be done by NS_ConvertUCS2toUTF8 instead of
calling that generic function which is slower. Or you could use
AssignWithConversion if the value is always "-moz-fixed" or an empty string. In
fact, if that's the case you can remove the fontname parameter.

Comment 78

[Jungshik Shin]

Attachment: a new patch addressing Naoki's concerns

I removed fontName argument from GetMailNewsFont() (perhaps,
the name of the function needs to be changed to reflect that).
I also eliminated unused code in GetMailNewsFont(). 
My preliminary test result indicateed that it works as it should.

Comment 79

[nhottanscp]

1) I think this part need to use charsetAtom. Use nsICharsetConverterManager2 to
get charsetAtom from charsetName then GetCharsetLangGroup takes charsetAtom.

+    aCharSets = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID);
+    if (!aCharSets)
+      return NS_ERROR_FAILURE;
+      
+    // get a language, e.g. x-western, ja
+    nsAutoString u;
+    u.AssignWithConversion(aCharset.get());
+    rv = aCharSets->GetCharsetLangGroup(&u, getter_AddRefs(aLangGroup));


2) Please change nsCAutoString to nsCString.
+extern "C"  nsresult GetMailNewsFont(MimeObject *obj, PRBool styleFixed,
PRInt32 *fontPixelSize, PRInt32 *fontSizePercentage, nsCAutoString& fontLang);

3) Please review indentation.

Comment 80

[Jungshik Shin]

Attachment: a new patch per Naoki's comment

Naoki,
I believe I took care of all three points of yours. Can
you take a look? 
Thanks,

Comment 81

[nhottanscp]

Comment on attachment 68227 [details] [diff] [review]
a new patch per Naoki's comment

r=nhotta

Comment 82

[David :Bienvenu]

JF, do you want to review this too, since it's in mime?

Comment 83

[Jean-Francois Ducarroz]

fine with me too. R=ducarroz

Comment 84

[David :Bienvenu]

Comment on attachment 68227 [details] [diff] [review]
a new patch per Naoki's comment

It looks like you might have tabs here. The indentation seems wrong. Other than
that, sr=bienvenu
+			// generic font-family name ( -moz-fixed for fixed font
and NULL for
+			// variable font ) is sufficient now that bug 105199
has been fixed.

Comment 85

[(not reading, please use [e-mail] instead)]

Comment on attachment 68227 [details] [diff] [review]
a new patch per Naoki's comment

looks good, some minor issues:

>+    nsCOMPtr<nsIAtom> aLangGroup;
>+    const PRUnichar* langGroup = nsnull;
>+    nsCAutoString aPrefStr;

don't name local variables "aFoo", that notation is for method arguments.  

please fix aCharset and aPrefStr;

>+    rv = aCharSetConverterManager2->GetCharsetAtom2(PromiseFlatCString(aCharset).get(),getter_AddRefs(charsetAtom));

PromiseFlatCString(aCharset).get() should just be charset.get() [assuming you
rename aCharset to charset]

>+    // calculate percentage
>+    *fontSizePercentage = (PRInt32)((float)*fontPixelSize / (float)originalSize * 100);

just double checking, can originalSize ever be 0?  
That would only happen if someone set the default in the default prefs .js to
0, right?

>+      PR_snprintf(buf, 256, "<div class=\"moz-text-html\"  lang=\"%s\">", 
>+                  PromiseFlatCString(fontLang).get());

PromiseFlatCString(fontLang).get() should be fontLang.get() 

fix those minor issues, and then sr=sspitzer

Comment 86

[nhottanscp]

checked in (with the super reviewers' comments)

Thank you for the contribution.

Comment 87

[Jungshik Shin]

Naoki,
Thank you for checking in.

ji,
Can you verify? My test results with last night's build
showed nothing unusual (everything was what it's supposed
to be). 

Comment 88

[ji]

Verified as fixed with 02/18 builds.