Closed
Bug 235089
Opened 21 years ago
Closed 21 years ago
A Japanese display is ugly after 2004/02/19.
Categories
(Core Graveyard :: GFX: Mac, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: sugar.waffle, Assigned: darin.moz)
References
()
Details
(Keywords: regression)
Attachments
(8 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
sfraser_bugs
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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.
Attachment #141860 -
Attachment description: screen shut-1 → screen shot-1
Comment 4•21 years ago
|
||
-> darin
Assignee: pinkerton → darin
Component: General → String
Product: Camino → Browser
Version: unspecified → Trunk
Updated•21 years ago
|
Flags: blocking1.7b?
Keywords: regression
Assignee | ||
Comment 5•21 years ago
|
||
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? ;-)
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.
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
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 10•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 14•21 years ago
|
||
(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.
Summary: A Japanese display is ugly after checkin of bug 231995. → A Japanese display is ugly after 2004/02/19.
Comment 16•21 years ago
|
||
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•21 years ago
|
||
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.
Reporter | ||
Comment 18•21 years ago
|
||
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", "あくあPフォント"); user_pref("font.name.fantasy.ja", "あくあPフォント"); user_pref("font.name.monospace.ja", "Osaka−等幅");
Comment 19•21 years ago
|
||
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", "あくあPフォント"); user_pref("font.name.fantasy.ja", "あくあPフォント"); user_pref("font.name.monospace.ja", "Osaka−等幅");
Comment 20•21 years ago
|
||
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•21 years ago
|
||
(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•21 years ago
|
||
> 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 24•21 years ago
|
||
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•21 years ago
|
||
(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•21 years ago
|
||
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•21 years ago
|
||
Additional info: See:http://mozdev.org/pipermail/camino/2004-February/001588.html
Comment 28•21 years ago
|
||
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•21 years ago
|
||
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.
Component: String → GFX: Mac
Reporter | ||
Comment 30•21 years ago
|
||
(In reply to comment #28) > Created an attachment (id=142321) > rewrite nsFont::EnumerateFamilies It crashed, although this patch was applied.
Reporter | ||
Comment 31•21 years ago
|
||
Comment 32•21 years ago
|
||
(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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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 37•21 years ago
|
||
Comment on attachment 142361 [details] [diff] [review] fix Evil cast. Nice catch.
Attachment #142361 -
Flags: review+
Comment 38•21 years ago
|
||
Comment on attachment 142361 [details] [diff] [review] fix sr=dbaron. I moved the previous patch to bug 235755.
Attachment #142361 -
Flags: superreview+
Reporter | ||
Comment 39•21 years ago
|
||
It is the crash log which happened with the application of two patches(142321, 142361).
Comment 42•21 years ago
|
||
Fix checked in. Waiting for confirmation from crot0 that the patch worked for him.
Reporter | ||
Comment 43•21 years ago
|
||
(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•21 years ago
|
||
marking fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 46•21 years ago
|
||
Comment on attachment 142361 [details] [diff] [review] fix >Index: nsUnicodeMappingUtil.cpp >+ genNameString.AssignWithConversion(genName); This should have been CopyASCIItoUTF16(genName, genNameString);
Comment 47•21 years ago
|
||
jshin: Strings tend to confuse me. Can you explain to me why CopyASCIItoUTF16 is better?
Comment 48•21 years ago
|
||
So the string guide says to avoid *WithConversion() functions at all costs. So how about this patch?
Updated•21 years ago
|
Attachment #142464 -
Flags: review?(sfraser)
Comment 49•21 years ago
|
||
hm... why does this code not use nsAutoString? is genName longer than 63 characters?
Updated•21 years ago
|
Attachment #142464 -
Flags: review?(sfraser)
Updated•21 years ago
|
Attachment #142474 -
Flags: review?(sfraser)
Assignee | ||
Comment 51•21 years ago
|
||
Comment on attachment 142474 [details] [diff] [review] fix take 2.1 sr=darin
Attachment #142474 -
Flags: superreview+
Updated•21 years ago
|
Flags: blocking1.7b?
Updated•20 years ago
|
Attachment #142474 -
Flags: review?(sfraser_bugs)
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•