235089 - A Japanese display is ugly after 2004/02/19.

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Mac, defect)

Description

[Hiro]

User-Agent:       
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.7a) Gecko/20040219 Camino/0.7+

Official nighlty build crashes now.
However, Camino of the dynamic link which built personally is working.
This problem had occurred.

I think that this problem is a problem different from bug183954.


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Hiro]

Attachment: screen shot-1

The screen shot after bug bug 231995 checkin.

Comment 2

[Hiro]

Attachment: screen shot-2

The screen shot before bug bug 231995 checkin

Comment 3

[Roland Mainz]

Darin:
Can you take a look, please ?

Comment 4

[Mike Pinkerton (not reading bugmail)]

-> darin

Comment 5

[Darin Fisher]

anyone want to provide me with some pointers for where to start looking?  the
only difference between the two snapshots seems to be in how the fonts are
rendered.  is that correct?  if so, then what controls that?  i.e., where do i
start looking? ;-)

Comment 6

[Hiro]

These two screen shots were displayed by the new profile.
That is, both a font setup and font size are the same initial-setting values.

Comment 7

[Hiro]

Attachment: screen shot-3

It is the screen shot of firefox which applied the patch of bug234916.

http://bugzilla.mozilla.org/attachment.cgi?id=142056&action=view
It ran without crashing by applying this patch.
But, a Japanese display is ugly as well as Camino which built by the dynamic
link.

Mac OS X 10.3.2
New Profile

Comment 8

[Hiro]

Although the Windows version Firefox was also checked, especially this problem
seems to have not happened.
Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.7b) Gecko/20040222
Firefox/0.8.0+

These seemed to be satisfactory, although Chinese, Korean, etc. were displayed
and being seen also with the OS X version Firefox and Camino.

This problem is not good for the person using Japanese.
Although seemed to be the problem which occurs in the environment restricted
considerably, it desires to fix by the next release.

Comment 9

[Darin Fisher]

is anyone besides crot0 able to confirm this problem?

Comment 10

[Jungshik Shin]

I see attachment 141860 [details] and attachment 141861 [details] are a bit different, but is the
former really ugly? Kat, can you help me with spotting the problem crot0 reported? 

It seems like there's a problem with 'round-off' somewhere (in gfx/src/mac ?).
The font size is slightly different in the menu bar at the top, which appeared
to result in a bit uglier rendering there. It doesn't look like darin's patch is
responsible for that. 

crot0, is darin's patch the only one patch that got landed between two screenshots? 

Comment 11

[Jungshik Shin]

I looked into gfx/src/mac and I can't find any suspicous check-in made in
February. If I'm hard-pressed to blame something there, it's
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsNativeThemeMac.cpp&branch=&root=/cvsroot&subdir=mozilla/gfx/src/mac&command=DIFF_FRAMESET&rev1=1.25&rev2=1.26
However, it's a very very long shot. 

darin's patch didn't change anything there. So, we have to look somewhere else
(layout, probably). 

Comment 12

[Katsuhiko Momoi]

The 2 images show that the display of the 1st is using more than one fonts. In
some places the Hiragana and Katakana glyph size is much smaller than the Kanji
size. Looks like the font alogorithm is not finding a Japanese font first and
substituting some other language's fonts instead. Let me check on my Mac to see
if I can reproduce this problem.

Comment 13

[Katsuhiko Momoi]

The problem is confirmed. I compared Fierefox 0.8 release and the current trunk
build, in the current trun build, there are only 2 pref files under:

defaults: pref

folders, i.e. firefox.js and inspector.js

In the 0.8 release, there are several others such as all.js, macprefs.js, etc.
Supposedly, an entry in firefox.js points to the font defition which used to be
in macprefs.js. 
The font definition tells firefox to use certain fonts for Japanese (and other
languages) first before trying out other fonts. Looks like under the trunk
build, this font definition is not found and so random fonts containing needed
characters are used.
  

Comment 14

[Hiro]

(In reply to comment #10)
> crot0, is darin's patch the only one patch that got landed between two
screenshots? 
> 
No.

At the date on bonsai, it is 02/18/2004. This problem did not exist in firefox
built with the sauce at the 11:19 times.

And it is remembered that this problem arose by Camino which biulded with the
sauce which checkouted in the same time zone(02/19/2004 05:23) as biuld reported
by bug234907.
But This may be my misapprehension....

Therefore, a summary is corrected.
I am sorry to make it get confused, Darin-san and Developers.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(cc'ing bsmedberg re comment 13)

Comment 16

[Katsuhiko Momoi]

Additional info:

I see that "font.name.font_type.lg_name" pref lines are now in all.js under the
"greprefs" rather than in macprefs.js file. Presumably, the latter is no longer
used. It also seems Firefox is loading "font.name...." preferences since they
show up if you do "about:config". (BTW, why are platform specific font names in
all.js now?)

In any case, judging from some of the Chinese font glyphs used on Japanese
pages, it's clear that non-Japanese fonts are used for some characters. So even
if language font pref is known/loaded, this info is not consulted in display. 

Comment 17

[Katsuhiko Momoi]

I added the URL for the test page. 
A few guesses.

1. The page (above URL) not only uses proper Japanese encoding, euc-jp but also
a lang tag (lang="ja"). If we assume that "encoding to language family" mapping
is working, then what comes after, i.e. "language family to font-family", may be
broken. 

2. Or maybe "encoding to language family" mapping is broken. 

Comment 18

[Hiro]

Additional info:

The screen shot was taken by the default profile so that it might be easy to
check anyone.
When this problem has been noticed first, it is Camino customized by user.js.
In user.js, the font is set up as follows.
(Because, it is since a setup of Japanese fonts cannot be performed in
Preference of Camino....)
And this setup did not become effective like a screen shot after 2/19.

-- user.js is written by UTF-8.

user_pref("font.default", "sans-serif");
user_pref("font.language.group", "ja");
user_pref("font.minimum-size.ja", 10);
// fixed font size of ja
user_pref("font.size.fixed.ja", 14);
//fixe font size of ja
user_pref("font.size.variable.ja", 14);
//
user_pref("font.minimum-size.x-western", 11);
// fixed font size of US
user_pref("font.size.fixed.x-western", 15);
//fixe font size of US
user_pref("font.size.variable.x-western", 15);
//
//
user_pref("font.name.serif.ja", "ヒラギノ明朝 Pro W6");
user_pref("font.name.sans-serif.ja", "ヒラギノ丸ゴ Pro W4");
user_pref("font.name.cursive.ja", "あくあＰフォント");
user_pref("font.name.fantasy.ja", "あくあＰフォント");
user_pref("font.name.monospace.ja", "Osaka−等幅");

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The previous comment seems to be in Shift_JIS.  In UTF-8, those last 4 lines are:

user_pref("font.name.serif.ja", "ヒラギノ明朝 Pro W6");
user_pref("font.name.sans-serif.ja", "ヒラギノ丸ゴ Pro W4");
user_pref("font.name.cursive.ja", "あくあＰフォント");
user_pref("font.name.fantasy.ja", "あくあＰフォント");
user_pref("font.name.monospace.ja", "Osaka−等幅");

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In Firefox, could you look at a page that shows the problem, go to Tools -> DOM
Inspector, and see if there are any differences in the computed style for the
font-family property between builds before and after this broke?  If so, what is
the difference?

In more detail:
 * load the page
 * go to Tools -> DOM Inspector
 * click on the first button on DOM Inspector's toolbar, and then (without
clicking elsewhere) click on the text that's in a different font (be sure to use
the same text both times)
 * at the top of the right-side panel, drop down the arrow and choose "Computed
Style"
 * look at the value for 'font-family'

Comment 21

[Katsuhiko Momoi]

(In reply to comment #20)

>  * look at the value for 'font-family'

I tried Mozilla trunk build (2004-02-25) since I could not find DOM Inspector on
the Firefox build. The result is:

Verdna, Arial, Helvetics, sans-serif

So the font-family info is correct but the pre-specified sans-serif font for the
language, i.e. "ja", is obviously not used. (That is, "language to pre-specified
lang-specific font of the font-family" mapping is not effective.)

Comment 22

[Katsuhiko Momoi]

> Verdna, Arial, Helvetics, sans-serif

Correct the typos: Verdana, Arial, Helvetica, sans-serif

It would be nice if the actual font name used in the layout can be accessed. But
I know from how they look and from a bit of search in the System Font Panel,
certain Chinese characters (probably almost all of them) are being displayed by
Chinese fonts on the system. 

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is the value different before and after the regression?

Comment 24

[Eiichi]

This bug is critical, because it seems to affect not only 2 bite but also 1 bite
fonts.
It seems to be caused by ignoring the font settings in prefs.js.

Comment 25

[Katsuhiko Momoi]

(In reply to comment #23)
> Is the value different before and after the regression?

No. It's the same. Mozilla trunk build 1.7a release build. 2004-02-18.
The problem is observable on all current trunk builds.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment 25 demonstrates that the problem is not related to ignoring font
preferences, since the computed font-family did not change across the regression
interval.

However, I imagine it may have something to do with nsFont::EnumerateFamilies
needing to call BeginWriting instead of casting away const.

Comment 27

[Eiichi]

Additional info:
See:http://mozdev.org/pipermail/camino/2004-February/001588.html

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: rewrite nsFont::EnumerateFamilies

I'm guessing this will fix the problem, although I have no way of telling. 
(The much simpler change that I think would also fix it is:

-  PRUnichar* start = (PRUnichar*)(const PRUnichar*)familyList.get();
+  PRUnichar* start;
+  familyList.BeginWriting(start);

but this code is unnecessarily complicated, so I decided to clean it up (and
save copying the whole string).

Comment 29

[Jungshik Shin]

Kat, can you build with dbaron's patch and see if it fixes the problem?  

How about this? 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsFont.cpp&branch=&root=/cvsroot&subdir=mozilla/gfx/src&command=DIFF_FRAMESET&rev1=3.19&rev2=3.20

It doesn't fall within the regression window (landed on Feb. 20th) and I cannot
see how it can affect the font selection on Mac. However, at least it's got
something to do with 'generic' font names. 

Comment 30

[Hiro]

(In reply to comment #28)
> Created an attachment (id=142321)
> rewrite nsFont::EnumerateFamilies
It crashed, although this patch was applied.

Comment 31

[Hiro]

Attachment: crash log

Comment 32

[Mike Pinkerton (not reading bugmail)]

(In reply to comment #29)
> How about this? 
>
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsFont.cpp&branch=&root=/cvsroot&subdir=mozilla/gfx/src&command=DIFF_FRAMESET&rev1=3.19&rev2=3.20
> 

Bryner and i were discussing that yesterday, he doesn't think it affects mac at
all, we don't use that flag anywhere. But it's something to keep an eye on.

People are reporting this with other fonts as well, not just japanese display.
Reports, however, are spotty.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If the problem is in nsFont::EnumerateFamilies, the problem would be a
regression from darin's string landing.

Could you try the -1 line +2 lines change in comment 28 instead, since the patch
crashes (although I don't know why)?

Comment 34

[jhp (no longer active)]

I am seeing a similar font selection problem, but on an English system.  The 3
line fix in comment 28 did not fix the issue for me.

What I am seeing is that MapGenericFontNameType() is always returning
kUknownGenericFontName (at
http://lxr.mozilla.org/mozilla/source/gfx/src/mac/nsUnicodeMappingUtil.cpp#326).
 This means that the mGenericFontMapping table does not get initialized with the
proper font values from prefs.

It appears that EqualsIgnoreCase() is failing in MapGenericFontNameType().

Comment 35

[jhp (no longer active)]

Attachment: fix

This fixes it in my build.  genName is an nsCAutoString, which is being passed
as |(const nsString&)genName| to the nsString constructor.  However, the buffer
of the resulting nsString still looked like a char* buffer rather than a
PRUnichar* buffer (i.e. it looked like 0x7364, rather than 0x0073 0x0064).

Comment 36

[jhp (no longer active)]

*** Bug 235703 has been marked as a duplicate of this bug. ***

Comment 37

[Simon Fraser [no longer active]]

Comment on attachment 142361 [details] [diff] [review]
fix

Evil cast. Nice catch.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 142361 [details] [diff] [review]
fix

sr=dbaron.

I moved the previous patch to bug 235755.

Comment 39

[Hiro]

Attachment: crash log

It is the crash log which happened with the application of two patches(142321,
142361).

Comment 40

[jhp (no longer active)]

crot0, try applying only attachment 142361 [details] [diff] [review].

Comment 41

[Mike Pinkerton (not reading bugmail)]

jasper, this is the font bug you were telling us about on irc yesterday.

Comment 42

[jhp (no longer active)]

Fix checked in.  Waiting for confirmation from crot0 that the patch worked for him.

Comment 43

[Hiro]

(In reply to comment #40)
> crot0, try applying only attachment 142361 [details] [diff] [review].

I apply only this patch and think that this problem fixed!!
(The check was performed by Camino.)

Japanese is displayed by font setup customized by the user.js file.
(Customize is comment#18 and comment#19.)

Comment 44

[jhp (no longer active)]

marking fixed.

Comment 45

[Boris Zbarsky [:bzbarsky]]

*** Bug 235652 has been marked as a duplicate of this bug. ***

Comment 46

[Jungshik Shin]

Comment on attachment 142361 [details] [diff] [review]
fix

>Index: nsUnicodeMappingUtil.cpp

>+  genNameString.AssignWithConversion(genName);

  This should have been 

    CopyASCIItoUTF16(genName, genNameString);

Comment 47

[jhp (no longer active)]

jshin: Strings tend to confuse me.  Can you explain to me why CopyASCIItoUTF16
is better?

Comment 48

[jhp (no longer active)]

Attachment: fix part 2

So the string guide says to avoid *WithConversion() functions at all costs.  So
how about this patch?

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... why does this code not use nsAutoString? is genName longer than 63 characters?

Comment 50

[jhp (no longer active)]

Attachment: fix take 2.1

Good catch.  Thanks...

Comment 51

[Darin Fisher]

Comment on attachment 142474 [details] [diff] [review]
fix take 2.1

sr=darin

Comment 52

[Jason Barnabe (np)]

*** Bug 235658 has been marked as a duplicate of this bug. ***