Closed Bug 391076 Opened 17 years ago Closed 15 years ago

CJK font-names not correctly recognised/understood in the Appearance/fonts prefpane.

Categories

(Camino Graveyard :: Preferences, defect)

Product:
Camino Graveyard
Component:
Preferences
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Camino2.0

People

(Reporter: phiw2, Assigned: murph)

References

Details

(Whiteboard: l10n [camino-2.0])

Attachments

(3 files, 11 obsolete files)

test case comment 28
3.03 KB, text/html; charset=shift-jis
Details
screenshot
104.31 KB, image/png
Details
Patch, v3.4
14.36 KB, patch
mikepinkerton
: superreview+
Details | Diff | Splinter Review
Camino version of bug 390901. Issue: Camino, running under any 'roman' user (log-in language English,...) does not recognise the font names set in all.js for Chinese and Japanese lang group (reports missing fonts). I was expecting Gecko to provide the correct information, but apparently the 'translation' has to happen at application level (see bug 390901 comment 10). Camino does recognise/use the fonts correctly in the content (webpages). Side issue: When running on a Japanese user, Camino still does not recognise the font-name for Japanese fonts (in the pref pane only). Font family is reported as missing, although 'ヒラギノ角ゴ' (Hiragino Kaku Gothic Pro - default for Jpn sans-serif is the correct name. Manually setting the fonts (in both Appearance > Fonts and the advanced pane) works and does not create problems; although I still have to double check this under a Japanese user. Branch-1.8.x version of this issue was bug 175651.
I wrote up a patch for the Appearance pane which utilizes Masayuki Nakano's API provided for bug 390901. It fixes the original issue in comment #0 but fails to take care of that side issue (running under Japanese language). Even though I've worked with this problem before I am confused by the font naming ordeal. I'd like to make sure I fully understand this issue and how to fix it. In what format/locale should Gecko font names be expressed? I know that Cocoa does want font names to be expressed in the currently running language. But what about Gecko? Gecko's 1.8 branch dealt with non-roman font names in only their native locale. Does the trunk, since it has moved away from Quickdraw, now expect localized names?
Masayuki-san, could you help clarify the question in comment 1 ?
See gfxQuartzFontCache::AppendFontFamily http://lxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxQuartzFontCache.mm#183 Basically, we use |localizedNameForFamily| of |NSFontManager| on trunk. It returns localized family name if the name and the system are same locale. However, the cocoa's font family enumerator doesn't match for CSS spec. Cocoa groups the variable pitch font and fixed pitch font to same family. But CSS spec cannot specify the pitch in a family. Therefore, if an family has both pitch, we need to separate the family to two families. The code is here: http://lxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxQuartzFontCache.mm#282 > 282 for (PRUint32 j = 0; j < membersCount; j++) { > 283 NSArray *member = [members objectAtIndex:j]; > 284 PRUint32 newTraits = > 285 [[member objectAtIndex:INDEX_FONT_TRAITS] unsignedIntValue]; > 286 if ((isFixedPitch && !(newTraits & NSFixedPitchFontMask)) || > 287 (!isFixedPitch && (newTraits & NSFixedPitchFontMask))) { > 288 AppendFontFamily(fontManager, > 289 [member objectAtIndex:INDEX_FONT_POSTSCRIPT_NAME], PR_TRUE); > 290 break; > 291 } > 292 } And we can get the localized family name with [NSFont fontWithName:size] from the postscript name. By this trick, we can use 'Osaka−等幅' in prefs. So, we need to create the font family name list ourselves, we cannot use cocoa's enumerator. I don't know the camino's UI. Don't you use the cocoa's enumerator?
(In reply to comment #3) Thank you for clarifying. > So, we need to create the font family name list ourselves, we cannot use > cocoa's enumerator. I don't know the camino's UI. Don't you use the cocoa's > enumerator? Foremost, we allow font selection via the standard Cocoa NSFontPanel. Then, we offer more advanced configuration and that area contains popup menus which are filled by enumerating NSFontManager's |availableFontFamilies| and titled using |localizedNameForFamily:|. As philippe has mentioned, there doesn't seem to be any trouble with Gecko understanding the font names we supply back, even after the user chooses a new one from our UI, meaning Gecko sorts out the name. The only real problem is with our verification that the font does exist and selecting it in our preferences UI. So, we need to find a good way to take a Gecko derived, non-roman, font name and hand it to Cocoa in a format which it expects and can successfully create a font from. The explanation and code you linked to (as well as the API offered in bug 390901) should aid us in figuring that out.
Assignee: nobody → murph
(See also bug 410733; there are other, non-CJK, fonts out there that return different names to different APIs. Suck.)
Target Milestone: --- → Camino2.0
Using the API in bug 390901 will probably fix this... Working on a patch.
Depends on: 390901
Attached patch Patch (obsolete) — — Splinter Review
Attachment #309483 - Flags: review?(stuart.morgan)
Attached patch Export nsIFontEnumerator.h (obsolete) — — Splinter Review
I'm not sure if we're allowed to do this, but nsIFontEnumerator needs to be exported so we can #include it in our source. The patch depends on this header existing in ../dist/include.
I meant to mention... philippe: If you have time, can you verify that this also works under a Japanese locale? I just want to make sure I tested that part right here. When you do so, also load up the advanced fonts panel, close it, and make sure the correct fonts are still used on web pages (Because I fetch the popup label a slightly different way, I want to ensure it works on other locales).
Attached patch Export nsIFontEnumerator.h (obsolete) — — Splinter Review
Fixes a typo in the previous patch.
Attachment #309484 - Attachment is obsolete: true
trying to build this results in an error: /Users/phiw/camino-build/build3/config/nsinstall -L /Users/phiw/camino-build/build3/gfx/thebes/public -m 644 /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxASurface.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxAlphaRecovery.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxColor.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxContext.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxFont.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxFontUtils.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxImageSurface.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxMatrix.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxPath.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxPattern.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxPlatform.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxPoint.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxRect.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxSkipChars.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxTypes.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxTextRunCache.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxTextRunWordCache.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxFontTest.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxPlatformMac.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxQuartzSurface.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxQuartzImageSurface.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxQuartzPDFSurface.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxAtsuiFonts.h /Users/phiw/camino-build/mozilla/gfx/thebes/public/gfxQuartzNativeDrawing.h ../../../dist/include/thebes /usr/bin/perl -I/Users/phiw/camino-build/mozilla/config /Users/phiw/camino-build/mozilla/config/build-list.pl ../../../config/final-link-libs thebes Creating ../../dist/include/gfx make[5]: *** No rule to make target `nsIFontEnumerator.h'. Stop. make[5]: *** Waiting for unfinished jobs.... make[4]: *** [export] Error 2 make[3]: *** [export_tier_gecko] Error 2 make[2]: *** [tier_gecko] Error 2 make[1]: *** [default] Error 2 make: *** [build] Error 2
Comment on attachment 309501 [details] [diff] [review] Export nsIFontEnumerator.h (In reply to comment #11) > trying to build this results in an error: Sorry about that... It turns out nsIFontEnumerator.h is already exported into dist/include with the existing build configuration. For some reason, I was convinced it wasn't. I pulled and compiled a fresh build, and the generated header did prove to be where we need it. Thanks for attempting to test this, and again sorry for breaking your build. Just run a 'cvs update -dPC' in your gfx/public/ directory to revert the unnecessary patch.
Attachment #309501 - Attachment is obsolete: true
try 2, clean obj_dir Ld /Users/phiw/camino-build/build3/camino/build/Deployment/Camino.app/Contents/PreferencePanes/Appearance.prefPane/Contents/MacOS/Appearance normal i386 mkdir /Users/phiw/camino-build/build3/camino/build/Deployment/Camino.app/Contents/PreferencePanes/Appearance.prefPane/Contents/MacOS cd /Users/phiw/camino-build/build3/camino /Developer/usr/bin/g++-4.0 -o /Users/phiw/camino-build/build3/camino/build/Deployment/Camino.app/Contents/PreferencePanes/Appearance.prefPane/Contents/MacOS/Appearance -L/Users/phiw/camino-build/build3/camino/build/Deployment/Camino.app/Contents/PreferencePanes -L/Users/phiw/camino-build/build3/camino/../dist/Embed -L../dist/lib -F/Users/phiw/camino-build/build3/camino/build/Deployment/Camino.app/Contents/PreferencePanes -filelist /Users/phiw/camino-build/build3/camino/build/Camino.build/Deployment/AppearancePrefPane.build/Objects-normal/i386/Appearance.LinkFileList -lxpcom_core -lthebes -framework Foundation -framework Cocoa -arch i386 -bundle -mmacosx-version-min=10.4 -bundle_loader /Users/phiw/camino-build/build3/camino/build/Deployment/Camino.app/Contents/MacOS/Camino -Wl,-executable_path,/Users/phiw/camino-build/build3/camino/build/Deployment/Camino.app/Contents/MacOS -isysroot /Developer/SDKs/MacOSX10.4u.sdk ld: library not found for -lthebes collect2: ld returned 1 exit status ** BUILD FAILED **
philippe, what OS and what Xcode? I'll try to have a look at this tomorrow.
Status: NEW → ASSIGNED
10.5.2, Xcode 3, gcc 4.01. Regular builds are fine.
(In reply to comment #13) > -L/Users/phiw/camino-build/build3/camino/../dist/Embed -L../dist/lib Ok, well if we got to the linking stage at least the header was able to be included this time :) The linker is not finding thebes in LIBRARY_SEARCH_PATHS, and that last -L argument is probably the reason, as it was not expanded to a full path (searching instead relative to the current working directory, "OBJDIR../dist/lib", not the camino/ directory.) Might be an issue with the project patch, I'll see if I can figure out what Xcode's up to.
Attached patch Fix? for the static build (obsolete) — — Splinter Review
Here's murph's patch, with fixes for the Xcode "patented automatic search paths wackiness" and a "fix" for the static build. The problem philippe was hitting was that in a static build, lthebes is libthebes.a, not libthebes.dylib, and it lives in ../staticlib, not ../dist/lib. I just updated the library search paths for Development style; I didn't add libthebes.a to the "Link with Libraries" phase, since it seemed to me like that would cause shared builds to barf. Thus, it may not be the correct fix. It seems to fix the static build and seems to work without error, so maybe it is. Dunno. The one thing I see wrong with this patch (in en locale) is it doesn't handle the Osaka-Mono case (the default jp-mono font shows up as plain Osaka). It's something of a rat's nest of a problem, and I don't know off-hand all the bugs where the special-casing of Osaka-Mono was developed and refined.
Same as above, but without the merge gunk the crept in from somewhere.
Attachment #309812 - Attachment is obsolete: true
(In reply to comment #18) > Created an attachment (id=309818) [details] > Fix? for the static build (no merge gunk) Build succeeded ! Now to test it... (at least it worked correctly with my profile, recognised my fonts for Jpn, no longer 'user-set' in about: config).
(In reply to comment #17) > The one thing I see wrong with this patch (in en locale) is it doesn't handle > the Osaka-Mono case (the default jp-mono font shows up as plain Osaka). It's > something of a rat's nest of a problem, and I don't know off-hand all the bugs > where the special-casing of Osaka-Mono was developed and refined. Yeah, Osaka-Mono is a complete pain. Cocoa classifies it as a member of the Osaka family, so to allow it to be used as a separate family we put in a special pref to force a single face to be a "family": http://mxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxQuartzFontCache.mm#832 Unfortunately, that means that you probably need to special case this within the code for your patch. It's definately a good idea to create a test user with Japanese as the top language in the system lang prefs list, then verify that everything (incl. Osaka Mono) works in the prefs panel. Other nasty scenarios to consider is moving older profiles to trunk code, since older profiles may contain QD-style font family names (e.g. create a profile in FF2, specify "Futura Condensed" in the prefs, then open up the profile in FF3, should change automatically to "Futura").
(In reply to comment #20) > It's definately a good idea to create a test user with Japanese as the top > language in the system lang prefs list, then verify that everything (incl. > Osaka Mono) works in the prefs panel. Other nasty scenarios to consider is > moving older profiles to trunk code, since older profiles may contain QD-style > font family names (e.g. create a profile in FF2, specify "Futura Condensed" in > the prefs, then open up the profile in FF3, should change automatically to > "Futura"). > That is definitely on my todo list, complete with freaky Jpn fonts that I have laying around.
some testing 10.5.2 Intel: 1. As an English OS User: I haven't seen any problems at all (except for the 'Osaka' issue noted by Smokey). * starting with my default profile and switching fonts for Roman (Western) and Jpn), using the cocoa font panel or the Advanced pane. * starting from a brand new profile and doing the same * migrating from a Camino 1.6 profile with custom settings. John mentioned in comment 20: Futura Condensed; the selected font got moved to 'Futura'. That is the same as what Minefield does (I also tested with 'Helvetica Neue Light' --> 'Helvetica Neue). 2. As a Jpn OS user: here I had a bit of problems with the Jpn settings... Starting with a brand new profile * In the Cocoa panel, the Jpn fonts (Hiragino) are listed with their Roman names (Hiragino Kaku Gothic Pro instead of ヒラギノ角ゴ Pro). Click 'change', the font list pops up, name is in Jpn in the font-list, reselect the font, and the name is again in English; reproduced with 3 different font-families. * bizarrely, the selected font for 'monospace' is listed as 'Monaco, 14pt' (expected: Osaka 16pt). * going over to the advanced pane, and here the fun starts. Both Hiragino (serif and sans-serif) and Osaka−等幅 listed as missing (with their Jpn names). Selecting the Hiragino font(s) and clicking OK: surprise, suddenly Helvetica is selected in the cocoa pane. That is it, for now. I'll do some more testing, esp as a JPN user, time allowing. --- The whole business with the advanced pane may not be such an important issue, esp. if we get rid of the advanced pane altogether. The issues that existed before bug 175651 was fixed are no longer relevant. The user has no need to go to the advanced pane to select a font.
Comment on attachment 309483 [details] [diff] [review] Patch I don't think we need to worry about the advanced pane given bug 422576, but it sounds like we'll need to do something to handle comment 20. Also, as part of the push to isolate core use within the Camino code base, I've been trying to get everything out of the pref panes, so the nsIFontEnumerator code needed here should live somewhere in the Camino binary, wrapped in a Cocoa method that the pref pane can use. It's not ideal, but maybe a (NSString*)fontNameForGeckoFontName:(NSString*)geckoName method in NSPreferencePane base?
Attachment #309483 - Flags: review?(stuart.morgan) → review-
(In reply to comment #23) > (From update of attachment 309483 [details] [diff] [review]) > I don't think we need to worry about the advanced pane given bug 422576, but it > sounds like we'll need to do something to handle comment 20. The patch does correctly handle the profile migration issue, eg. creating a preview font for "Futura Condensed." The other issue mentioned in that comment though, which is Osaka-Mono, I'm not sure about. If I specify "Osaka-Mono" in prefs, this creates the following font: "Osaka-Mono 12.00 pt. P [] (0x1e62ce70) fobj=0x18985970, spc=6.00." ...which shows up in the font preview as plain "Osaka." If I open up the system font panel, there isn't actually a choice for an "Osaka Mono," so perhaps just previewing Osaka is acceptable, since otherwise there wouldn't be a Cocoa font and we're back to the (missing) tag. Since this bug is only focused on generating a preview of the pref, and not necessarily trying to set or convert any setting, I think showing Osaka might just be good enough. Is that reasonable? Otherwise, I have updated the patch to leave the advanced sheet alone, and segregated the Gecko calls out into PreferencePaneBase. I'll wait to get a little more clarification about the Osaka-Mono case before submitting a follow-up, though. Thanks everyone for the testing, btw.
(In reply to comment #24) > though, which is Osaka-Mono, I'm not sure about. If I specify "Osaka-Mono" in > prefs, this creates the following font: > > "Osaka-Mono 12.00 pt. P [] (0x1e62ce70) fobj=0x18985970, spc=6.00." > > ...which shows up in the font preview as plain "Osaka." If I open up the > system font panel, there isn't actually a choice for an "Osaka Mono," so > perhaps just previewing Osaka is acceptable, since otherwise there wouldn't be > a Cocoa font and we're back to the (missing) tag. In our very first stab at fixing this bug four(?) years ago, smfr disabled the "Typeface" column of the Cocoa font panel so that we stopped stuffing family+face names into Gecko prefs. If you check the panel in another Cocoa app, you'll see that Osaka has a "Regular-Mono" typeface available (as well as "Regular"), and that's what Gecko sees and uses as Osaka-Mono / Osaka−等幅.
I looked into this some more, and since I did mention that the correct font is actually obtained using either "Osaka-Mono" or "Osaka−等幅" in prefs, I wanted to figure out why the preview was still off. The preview reverting back to just plain "Osaka" is a result of the behavior in a later method in the preview generation process: http://mxr.mozilla.org/seamonkey/source/camino/PreferencePanes/Appearance/Appearance.mm#585 The |font| argument, in our scenario, would be "Osaka-Mono" but this object is later stripped down, to |baseFont|, representing just the parent family. I'll have to see if it's still necessary for us to step back to just the family, or if we can directly use specific fonts.
Attached patch Patch, v2 (obsolete) — — Splinter Review
Updated patch with Stuart's review comments. Basically, no project changes as the Gecko stuff was moved in the PreferenceBaneBase, and skipped worrying about the advanced font panel. To ensure the preview properly displays fonts such as Osaka-Mono, I did what I mentioned in the last comment and removed the line of code that strips the font down to just the family. This code was originally in place to handle cases where the user selected a font from our font panel and changing the face. This only happened on pre-10.3 systems because we have code in place to prevent the font face column on 10.3+. Since it is not possible to choose the face anymore, I feel that we should use the direct value from prefs. Philippe, if you have time, can you check to ensure that I tested this properly and that it does not break anything for users running over a non-Roman locale either?
Attachment #309483 - Attachment is obsolete: true
Attachment #309818 - Attachment is obsolete: true
Attachment #343675 - Flags: review?(stuart.morgan+bugzilla)
Man, this thing with Osaka is a pain... [1] * OS lang set to English US, my default. 1. Test 1: profile migration from Camino 1.6.4 with 'Futura Condensed' set --> OK, Futura is set (see comment 20). 2. Test 2: Starting with a fresh profile, Japanese lang group, Monospace: Osaka, 16px is set. A test page, shift_jis encoded displays correctly. In the Advanced pane the font is reported as missing. But see below. 3. messing up with Jpn fonts for serif/sans-serif: no problems, all my (screwy) Jpn fonts are recognised/used/...). * OS lang set to Chinese traditional. No particular problems to report, the prefs are set/applied - tested by a native Chinese visitor to my place. We didn't test profile migration from Camino 1.6ML though. * OS lang set to Japanese. 1. Profile migration worked OK. 2. the fields for serif and sans-serif for the Jpn lang group are correctly populated, but the name is displayed in English. Shouldn't it (could) it be Japanese ? 3. Advanced Pane for Japanese lang group: serif and sans-serif are empty, the pane can't be closed as long as the user doesn't fill the blank fields. 'Osaka-Mono' is reported as missing. 4. monospace, Jpn lang group: starting with a default profile: the monospace field shows Monaco, 14px. Ok, that is not soo bad, baring in mind that Osaka-Mono 16 actually uses Monaco 14 for Roman Characters. The reported font-size is also correct, for Roman Characters. My test file displays correctly, and uses Osaka-Mono for Jpn monospaced text. All font-sizes are correct. The preview in the pref pane is actually correct. Now comes the mess: if I change the pref for monospace to say Osaka, what is used is not Osaka-Mono, but Osaka. Different kerning, different line-height. This also applies to the Western user. As a Western user, you can test this: change that pref to anything else, then return to Osaka. I'll attach a test case that can be used for this. Keep the page open in Minefield/Firefox for reference. --- The issues about the Advanced pane noted above are actually only a problem if the user wants to change the minimum font size. --- [1] the font, not the city
Attached file test case comment 28 —
This file should trigger the use of Japanese lang group under normal configurations. (charset: shift_jis, lang attribute set to ja).
Attachment #343766 - Attachment mime type: text/html → text/html; charset=shift-jis
Is the font.single-face-list set within Camino prefs? Including 'Osaka-Mono' in that list will create a font family with a single face, which is what you probably want in the case of Osaka Mono. All other monospaced fonts that I know of are typically sepearated into distinct families. There's no way via simple CSS attributes to "reach" it, i.e. there's no combination of CSS attributes that can select a face like Osaka Mono when it's included in a larger family containing a face that has matching CSS attributes (i.e. Osaka).
(In reply to comment #28) > * OS lang set to Japanese. > 2. the fields for serif and sans-serif for the Jpn lang group are correctly > populated, but the name is displayed in English. Shouldn't it (could) it be > Japanese ? I wonder if this depends on the language of the app bundle in addition to the language of the OS? You can hack-test this by just duplicating English.lproj and renaming it Japanese.lproj; Camino will still appear in English regardless, but it'll tell the OS it has a Japanese localization available. If I've followed everything correctly, there are three problems with the new patch (ignoring the Advanced pane, which we want to remove; at the very least, we can remove the font selectors if we get this working...): 1) En OS - Jpn mono font appears set to Osaka instead of Osaka-Mono (this is maybe comment 24, just cosmetic) 2) Ja OS - Jpn mono font appears set to Monaco instead of Osaka-Mono 3) Any OS - Can't re-select Osaka-Mono after switching Jpn mono to something else (In reply to comment #30) > Is the font.single-face-list set within Camino prefs? Including 'Osaka-Mono' > in that list will create a font family with a single face, which is what you > probably want in the case of Osaka Mono. It's inherited from all.js, yes, so we have Osaka-Mono set there. I think the problem here (at least for bug 3 above) is that the Cocoa font UI doesn't expose Osaka-Mono, so we can only (re)select Osaka. Sean: this would be kind of a gross hack, but would it be possible that we special-case "Osaka" whenever someone tries to select it as the Japanese monospace font, and we actually select (and tell Gecko, and write to the prefs, etc.) "Osaka-Mono" instead?
(In reply to comment #31) > I wonder if this depends on the language of the app bundle in addition to the > language of the OS? You can hack-test this by just duplicating English.lproj > and renaming it Japanese.lproj; Camino will still appear in English regardless, > but it'll tell the OS it has a Japanese localization available. I had tried that, but no go. I also tried to put the Japanese.lproj folder from Camino 1.6.4ML in it (superhack, I know), that didn't work out either. I checked what SubEthaEdit does (for which I don't have a Japanese.lproj): display the Japanese fonts (Hiragino) with their Jpn names when the OS lang is Jpn. Smultron, which is localised, uses the English name. > 1) En OS - Jpn mono font appears set to Osaka instead of Osaka-Mono (this is > maybe comment 24, just cosmetic) Yeah, it is cosmetic, the preview is correct though. > 2) Ja OS - Jpn mono font appears set to Monaco instead of Osaka-Mono > 3) Any OS - Can't re-select Osaka-Mono after switching Jpn mono to something > else the 3 points are correct. So far, we haven't found any other issues. Point 3 is probably the most important. > Sean: this would be kind of a gross hack, but would it be possible that we > special-case "Osaka" whenever someone tries to select it as the Japanese > monospace font, and we actually select (and tell Gecko, and write to the prefs, > etc.) "Osaka-Mono" instead? I didn't want to suggest that one, but I had the same idea. That would keep the Gecko compat and web compatibility.
Comment on attachment 343675 [details] [diff] [review] Patch, v2 Okay, I'm totally lost. What I'd like is for one of the people who understands all the behavioral issues here to do behavioral reviews until they are satisfied, then request a code review from me. I can't tell if the current state of the patch is "not perfect, but undoubtedly better than what we have now" or "better in some respects, worse in others". If we have something that is an overall win with no serious regressions, I'd like to press forward with it and file specific follow-up bugs for the rest so we make progress.
Attachment #343675 - Flags: review?(stuart.morgan+bugzilla) → review?
I think the current state of this patch is around "better in some respects, worse in others". Trying to fix Osaka-Mono has created some behavioral issues, one of which (comment 28, Japanese OS, point 3) is really bad but which can go away if we fix this bug, and one of which (comment 31 point 3) is almost re-opening bug 175651 for that font. There are also some cosmetics we could do as follow-ups. Last Sean and I talked about this, he was going to try to do the hack I suggested in comment 31, which would fix comment 31 point 3. Once that's done, we could fix comment 28, Japanese OS, point 3 by removing the font selectors from the Advanced sheet (leave removing Advanced entirely by moving the min-font and default-family to the main nib to bug 422576), and file follow-ups on the cosmetic issues to other bugs. Does that sound like a plan?
The patch fixes issues with prefs.js migration (user sets, with Camino 1.6.x, Futura Condensed in Advanced pane --> correctly migrated to Futura see comment 20). Any font can correctly be selected (and is correctly displayed). The main issue with the current patch is the Osaka-mono problem: 1. display issue for the Jpn user (it appears as Monaco), but only in the prefpane. Cosmetic problem. 2. when the user (any) has switched away to some other font for Japanese, monospace, it cannot be reset to Osaka-mono. The hack suggested in comment 31 point 3 may fix both. The second issue is the advanced pane, for a Jpn user (blank fields) (follow up bug to remove the font-selectors as a first step ? - they don't have any use anymore, anyway) Jpn user is a user with Japanese as OS locale.
Because of the current interdependencies with bug 391076 and "Japanese OS, point 3", this needs to be fixed by the l10n freeze.
Flags: camino2.0b1?
Whiteboard: l10n
Comment on attachment 343675 [details] [diff] [review] Patch, v2 Going ahead and minusing this based on all the discussion subsequent to the patch, and to keep it out of the queue.
Attachment #343675 - Flags: review? → review-
Not going to block b1 on this, either.
Flags: camino2.0b2?
Flags: camino2.0b1?
Flags: camino2.0b1-
We really need to try to get this fixed early in the b4 cycle, particularly as we're now going to have zh-Hans localization in addition to jp affected by this bug. (For anyone needing a refresher, comment 34 expresses the current plan for fixing this.)
Attached patch Patch, v3 (Special-case Osaka) (obsolete) — — Splinter Review
(In reply to comment #39) > We really need to try to get this fixed early in the b4 cycle, particularly as > we're now going to have zh-Hans localization in addition to jp affected by this > bug. (For anyone needing a refresher, comment 34 expresses the current plan > for fixing this.) Attached is a new patch implementing the plan in comment 34. We resort to special-casing Osaka when saving it as the ja monospace font. Also, when displaying the font previews, the actual name indicated in the display now corresponds to the value in Gecko prefs, not the value returned by Cocoa, avoiding naming conflicts between the two. This is aimed at eliminating any confusion to what's actually being used by Gecko. philippe, I'm targeting you first for a behavioral review on this. I tried testing on my system both as an English and Japanese user, but you're better than I am at the multi-regional testing :). See if this patch fixes much of our latest problems, especially what's mentioned in comment 31. Thanks for your help.
Attachment #343675 - Attachment is obsolete: true
Attachment #384648 - Flags: review?(phiw)
(In reply to comment #40) > Created an attachment (id=384648) [details] > Patch, v3 (Special-case Osaka) This works much better ! :-) For both an English and Japanese user, changing the font-preferences for the Japanese lang group works correctly (selecting Osaka always implies Osaka-mono). And (equally important) the font is correctly used in all my test cases. The behaviour is identical to the latest Minefield build. A couple of issues I ran into: 1. for all users and lang groups. If a font (user) selected in the prefs goes missing between sessions, the pref pane does not report it as missing. A non-patched Camino reports the font as missing. (str: select 'andale mono' for Western monospace; quit Camino; disable the font using FontBook; relaunch Camino). This doesn't have any adverse effect on page display - as far as I could test so far. 2. for the Japanese user / Japanese lang group: if the user opens the advanced pref panel, the font-lists are empty and the panel cannot be closed, unless the user fills in the 3 fields. The only reason the user would have to open the advance pref pane is to set minimum font-sizes. 3. The most puzzling one: for the Japanese user only, the font size for Japanese lang group/monospace is set to 14px. When editing the pref in the Cocoa font-editing window, Monaco 14 is selected. But about:config reads the pref at its defaults (16px). Webpages display monospaced text as being 16px (for Japanese encoded pages such as shift_jis). Did you change anything that might affect profile migration (from Camino 1.6.x to Camino 2) ? I didn't test this part yet.
Comment on attachment 384648 [details] [diff] [review] Patch, v3 (Special-case Osaka) r+ becuase the basic behaviour is all correct. Nits 1 and 3 (esp) in my previous comment ought to be addressed though.
Attachment #384648 - Flags: review?(phiw) → review+
Attached patch Patch, v3.1 (obsolete) — — Splinter Review
(In reply to comment #41) Thanks philippe for testing this out for us. > A couple of issues I ran into: > > 1. for all users and lang groups. If a font (user) selected in the prefs goes > missing between sessions, the pref pane does not report it as missing. A > non-patched Camino reports the font as missing. > (str: select 'andale mono' for Western monospace; quit Camino; disable the font > using FontBook; relaunch Camino). > This doesn't have any adverse effect on page display - as far as I could test > so far. This one's interesting. I actually did have a problem in the code that was preventing us from realizing a font was missing, and I have fixed that. I accidentally caused us to always fall back to a default font for the preview only, even when the font could not be found on the system. I fixed this to correctly only resort to a default if no name is listed in prefs at all, as we did before. The strange part here though is that after disabling fonts, and even deleting them, then relaunching and debugging Camino, I found that the system must have had the fonts cached somewhere because an actual font was returned for names that were disabled prior to launch. Additionally, querying the system for available font names, the disabled fonts were still listed. On the surface, it seems like some sort of system bug. What I did to test it was just rename the font in user prefs to an invalid name, and then the font was in fact indicated as missing. > 2. for the Japanese user / Japanese lang group: if the user opens the advanced > pref panel, the font-lists are empty and the panel cannot be closed, unless the > user fills in the 3 fields. The only reason the user would have to open the > advance pref pane is to set minimum font-sizes. > > 3. The most puzzling one: for the Japanese user only, the font size for > Japanese lang group/monospace is set to 14px. When editing the pref in the > Cocoa font-editing window, Monaco 14 is selected. But about:config reads the > pref at its defaults (16px). Webpages display monospaced text as being 16px > (for Japanese encoded pages such as shift_jis). For these two items, I was unable to reproduce it. I did try though after fixing the first problem, so I'm not sure if they were caused by that. Can you re-test them with the updated patch? If they're still happening I'll have to find a better way to reproduce. > Did you change anything that might affect profile migration (from Camino 1.6.x > to Camino 2) ? I didn't test this part yet. I should not have changed anything about this scenario from Patch v2, as the only real changes involved special casing Osaka. The first problem you mentioned also existed in that patch. Thanks again for your help.
Attachment #384648 - Attachment is obsolete: true
Test 1 - English user: default profile & fresh org.mozilla.camino.plist & my day-to-day profile * profile migration from an old Camino 1.6.x profile went well (Futura Medium was converted to Futura). * switching fonts in the Japanese lang group/monospace works correctly (and back to Osaka-Mono) * issue 1 is gone - simply disabling a font in FontBook flags it as missing **). * small display issue: when selecting Osaka in the Cocoa font window, the _preview_ in the Camino's prefpane looks incorrect (it displays as Osaka regular) Test 2 - Japanese user, starting with a default profile & plist This one didn't go too well... :( * The monospace entry for the Japanese lang group flags the (default) 'Osaka-Mono' as (missing). :-( * manually changing the choice to something else and back to Osaka selects the correct font according to about:config and webpage display. * same display issue as test 1 above. * disabling fonts worked correctly (I actually the expected OS warning dialog this time). * the advanced prefpane still suffers from the same issue as previously noted in comment 41. - lists Osaka-Mono as missing - serif and sans-serif are empty I suppose because it expects the Japanese localised font-name as input. Tested with my existing Japanese user and a new Japanese user on one machine, and tested with an existing Japanese user (the 'kids' user) on another machine. All machines are 10.5.7 Intel. ** I know what you mean with fonts caching issues; dunno if it is buggy at OS level or what. Caused me - and John Daggett - mucho headaches while testing the @font-face implementation. PS - my build is here: http://kenko-do.net/_t/Camino_cjk2.dmg (Intel only)
(In reply to comment #44) > * the advanced prefpane still suffers from the same issue as previously noted > in comment 41. We're ripping that out (or at least stripping it down) once this patch lands, so we don't need to worry about regressions there.
(In reply to comment #45) > (In reply to comment #44) > > * the advanced prefpane still suffers from the same issue as previously noted > > in comment 41. > > We're ripping that out (or at least stripping it down) once this patch lands, > so we don't need to worry about regressions there. Stuart, I know that. I mentioned it, just in case it cause some other interaction(s) with the font pref pane.
(In reply to comment #44) > Test 2 - Japanese user, starting with a default profile & plist > This one didn't go too well... :( > > * The monospace entry for the Japanese lang group flags the (default) > 'Osaka-Mono' as (missing). :-( So, this sounds like the "only" thing we have to do yet is special case Osaka−Mono (or perhaps Osaka−等幅) in the "(missing)" algorithm?
(In reply to comment #47) > So, this sounds like the "only" thing we have to do yet is special case > Osaka−Mono (or perhaps Osaka−等幅) in the "(missing)" algorithm? Smokey, in the multi-lingual builds, font names listed in all.js seem to be listed as localized names... So, in the development and English builds we have: > pref("font.name.monospace.ja", "Osaka-Mono"); and in the Multilingual build: > pref("font.name.monospace.ja", "Osaka−等幅"); This is what was causing the problem with the Japanese locale above, I believe, and why I wasn't able to reproduce it here. Is it correct that the default font names listed in all.js are localized / different in those situations?
(In reply to comment #48) > Is it correct that the default > font names listed in all.js are localized / different in those situations? Only if you're comparing across branches. Gecko 1.8.1 and below use "Osaka−等幅", so a current Camino ML release will show that (but so will Cm1.6.x); we changed in Gecko 1.9.0 to use canonical font names, so "Osaka-Mono" is what any Cm2.0 builds will show.
Attached patch Patch 3.1 with debug logging (obsolete) — — Splinter Review
(In reply to comment #47) > (In reply to comment #44) > > Test 2 - Japanese user, starting with a default profile & plist > > This one didn't go too well... :( > > > > * The monospace entry for the Japanese lang group flags the (default) > > 'Osaka-Mono' as (missing). :-( > > So, this sounds like the "only" thing we have to do yet is special case > Osaka−Mono (or perhaps Osaka−等幅) in the "(missing)" algorithm? Thanks for clarifying that Smokey. I am unable to reproduce the problem where Osaka-Mono shows up as a missing font, even with my system language set to Japanese, and a fresh plist and profile. Specifying the font as either Osaka-Mono or Osaka−等幅 on my system returns a valid NSFont either time, thanks to the new -fontNameForGeckoFontName: method. I'm trying to determine what is different about running under a Japanese locale that's causing the font resolution not to work properly. So, Smokey or philippe, any ideas on what could be different about the Japanese locale? It must be something with how the name "Osaka-Mono" is specified, and how the system interprets names under that locale. To help us debug this, I created a patch to log the font names. philippe, if you could compile and run this under your Japanese user account, that'd be great. Look in your console for the lines: >2009-07-05 19:44:25.388 Camino[35061:10b] ja monospace font name in prefs: Osaka−等幅 >2009-07-05 19:44:25.389 Camino[35061:10b] ja monospace font name to Cocoa: Osaka-Mono (You can also run Camino from the terminal, and the logging will appear right in standard output, with the command "/Path/to/Camino.app/Contents/MacOS/Camino") Thanks for your help!
Attached image screenshot —
the text reads: yugao:~ phiwtest$ /applications/camino_cjk.app/Contents/MacOS/Camino 2009-07-06 10:55:26.739 Camino[13057:10b] ja monospace font name in prefs: Osaka-Mono 2009-07-06 10:55:26.740 Camino[13057:10b] ja monospace font name to Cocoa: Osaka−等幅 yugao:~ phiwtest$ At the bottom, the Japanese text you see behind the terminal window is Osaka-Mono (in a pre block of text).
(In reply to comment #51) Thanks for testing so fast philippe, that's a huge help and pinpoints the cause of this. Cocoa expects us to create the font with a localized name, Osaka−等幅 when running under a Japanese locale. The reason I wasn't seeing this is because I didn't actually restart my system, I only changed the language prefs and relaunched Camino. Now that I can reproduce it here, I'll get the whole situation taken care of.
Attached patch Patch, v3.2 (obsolete) — — Splinter Review
Alright, I think I have it this time. This updated patch takes care of the missing Osaka-Mono when running under the Japanese locale. Also, I fixed in this patch the two new bugs that this one blocks (and caused in the first place). I chose to have the sample cell display font names identically to how Cocoa would in the system font panel, that is, localized into the current locale (this is what bug 502538 was about). When saved to prefs, the name Gecko prefers is actually used instead. The only issue that I can notice now is that after changing the Japanese monospace font back to Osaka (there is no "Osaka-Mono" in the Cocoa font panel), the sample cell will indicate that the mono font is "Osaka," (as chosen in the font panel) while we actually do the right thing in code and set the Gecko pref to "Osaka-Mono." Except for cases when the font is actually changed back to Osaka, the Japanese mono font is actually listed explicitly as "Osaka-Mono," in the sample cell. It's only after changing it that the incorrect name appears (this has to do with our sample cell updating code, and to me was not worth changing the entire mechanism for this one case). So, in addition to taking care of the original problem this bug references (false missing font indications), we're also solving the issue of displaying localized names in the sample cell to match what the system itself does in the font panel (bug 502538). Also noticed and changed here is that when there's no font pref for a given region and type, we indicate this in the sample cell (bug 502535) rather than falsely indicating other values. philippe, can you make sure that the issues you've been noticing are now resolved? Also, can you double check, under your Japanese locale especially, that the font names chosen actually are used correctly on web pages by Gecko? Note that this patch effectively breaks the advanced sheet under non-roman locales, in that some drop downs will be empty and the sheet cannot be dismissed with nil values set in them.
Attachment #384873 - Attachment is obsolete: true
Attachment #386907 - Attachment is obsolete: true
Attachment #387003 - Flags: review?(phiw)
1. The basic functionality -and especially the 'Osaka-mono' problem- works great. 2. the blocking bugs: * localised font names work correctly * the 'missing' fonts issue works as expected, I think. For those lang. groups where a font is specified (e.g. Kannada), the font is listed as missing when not installed. For those with no defaults at all (e.g. Khmer), the fields displays 'no font selected'. 3. remaining issue: * as noted in comment 53: change Japanese monospace font to anything and back to Osaka, it says (and displays as) 'Osaka'. * Now for the odd thing: quit browser, restart: the field says 'Osaka-Mono'. This affects both the Japanese and the Western User. * For the 'Western' user (only). Japanese, proportional reads: 'HiraKakuPro-W3', expected: 'Hiragino Kaku Gothic Pro' (that is 'PS name' is used instead of 'Family'). The same happens for the 'Kozuka Gothic Pro' family [1]: display: KozGoPro-Regular, expected Kozuka Gothic Pro. Both of those families have a localised Jpn name. [2] [1] Adobe font, ships with Photoshop CS 3+ [2] That behaviour is not tied to particular lang. group, you could set the proportional font under Western to 'Kozuka Gothic Pro' and get the same result. Strange thing - but this happened only once under the Western User: for Japanese proportional I switched to use Kozuka Gothic Pro. Switched back to Hiragino, the field said and displayed HiraKakuPro-W6 (the bold face). Quit and restart and the field displayed HiraKakuPro-W3. 4. In all cases, the fonts as used in a webpage are correct. No odd character display or similar - so far. I'll use this build for my surfing needs today, and I'll put it on my partner's machine (native Japanese person).
Attachment #387003 - Flags: review?(phiw) → review+
Attached patch Patch, v3.3 (obsolete) — — Splinter Review
(In reply to comment #54) Thanks again for your help testing this philippe. > 3. remaining issue: > * as noted in comment 53: > change Japanese monospace font to anything and back to Osaka, > it says (and displays as) 'Osaka'. > * Now for the odd thing: quit browser, restart: the field says > 'Osaka-Mono'. > This affects both the Japanese and the Western User. I have actually correct this aspect. When updating the preview, we did not "re-translate" the name through the new Gecko/Cocoa conversion code I introduced in this bug. I now have font changes re-fetch the font through that mechanism as well, even though it's technically known. > * For the 'Western' user (only). Japanese, proportional reads: > 'HiraKakuPro-W3', expected: 'Hiragino Kaku Gothic Pro' > (that is 'PS name' is used instead of 'Family'). > The same happens for the 'Kozuka Gothic Pro' family [1]: > display: KozGoPro-Regular, expected Kozuka Gothic Pro. > Both of those families have a localised Jpn name. [2] I have taken care of these name display issues as well. As I did in the previous patch, I feel we should be using localized names to display the font choices, as |fontName| and |familyName| (which we are using currently) are not designed for use displaying to the user. In the previous patch though, I was mistakingly using the localized font name, whereas we only offer family names in the font selection panel, so that's why the sample was different that what was chosen. It will now display a localized family name. So, my goal was that the font name displayed in the sample cell should exactly match what is offered (and chosen) in the system font panel - meaning we should not name fonts differently than the panel in which they're chosen from. The only font choice who's name will again be off is Osaka-Mono, will just be displayed as "Osaka" when chosen in the font panel and in the sample. The font style in the sample will actually be correct though, as Osaka-Mono, and it will be used in web pages. It's just the display name in our sample that's incorrect. I was hesitant to add more "special case" code when it came to just displaying the name in the sample cell, just to have it say "Osaka-Mono" instead of "Osaka." In summary, here's what this patch is taking care of: - Fonts no longer falsely indicated as missing when they're not - Sample cell displays font family names in proper in proper localization (bug 502538) - Special case for setting Osaka as a monospace font - Locales with no font specified actually indicates that fact (bug 502535) - The font name in our sample cell is identical to the choices offered in the NSFontPanel (again, regardless of how Gecko names them, I felt we should indicate font names exactly like the system does). I know it seems like we're tackling multiple things under this single bug, but many of these other enhancements were related and also possible with the underlying changes I was making here. As is evident by the two new bugs this blocks now, the changes also caused or brought into light a few other issues, issues that made sense to address immediately with this patch, not leaving go for another. So, with philippe's testing, and my own debugging here, I think we can say that the behavior is now what we expect. I'm going to advance to a code review now.
Attachment #387003 - Attachment is obsolete: true
Attachment #387491 - Flags: review?(stuart.morgan+bugzilla)
Comment on attachment 387491 [details] [diff] [review] Patch, v3.3 Just minor comments; r=me with changes. I didn't actually test this, since I think that's been pretty well covered at this point :) Huge thanks to everyone who slogged through this! >+// Returns nil if the found could not be found. s/the found/the font/ >- >+ Whitespace slipped in here. >+ // Even though we know the new font we actually re-fetch it again, as some fonts "again" is redundant. >+// Returns an NSFont created by first translating a Gecko font family name (used in preferences) >+// into the value Cocoa uses to identify the font. There's occasionally a mismatch when using the >+// value in preferences to create an NSFont (usually caused by localization differences). >+// Return value is nil if a font could not be created. >+- (NSString*)fontNameForGeckoFontName:(NSString*)geckoName; Description needs fixing; it doesn't return an NSFont. >Index: src/preferences/PreferencePaneBase.mm ... >+#include "nsCOMPtr.h" >+#include "nsCRT.h" >+#include "nsIServiceManager.h" >+#include "nsIFontEnumerator.h" I know I was the one who suggested putting this here, but seeing it I had to introduce Gecko code into yet another class. Maybe the implementation should move to PreferenceManager, and this should be a passthough? That would be more consistent with the rest of the class.
Attachment #387491 - Flags: review?(stuart.morgan+bugzilla) → review+
Attached patch Patch, v3.4 — — Splinter Review
Updated with Stuart's review comments. Note for localization, there is a new Localizable.strings key created by this patch: "NoFontSelected" = "No font selected"; Displayed when there is no font specified in prefs for a given language group and type.
Attachment #387491 - Attachment is obsolete: true
Attachment #388712 - Flags: superreview?(mikepinkerton)
Comment on attachment 388712 [details] [diff] [review] Patch, v3.4 sr=pink
Attachment #388712 - Flags: superreview?(mikepinkerton) → superreview+
Landed on cvs trunk and the CAMINO_2_0_BRANCH. Thanks all around for the all the work here! Next up, bug 422576 :P
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: camino2.0b4? → camino2.0b4+
Resolution: --- → FIXED
Whiteboard: l10n → l10n [camino-2.0]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: