Some CJK fonts have wrong underline offset, therefore, we need to adjust it. Roc suggested that we should adjust that only for well known fonts. We need some steps for fix this bug. First, we need to create the blacklist of "bad" CJK fonts. If somebody know such fonts, please tell us them. (for testing you can use attached testcase.) This bug MUST be fixed on b4! I'll post such font list, please wait.

er, Note that if you use MacOS, please test after bug 402524. Current trunk build of mac has wrong logic to computing the underline offset.

nominating to the blacklist. I think some fonts are separated by unicode range, e.g., -ExtB suffix, they should be included. Somebody know other bad fonts? Windows: FangSong Gulim (CJK characters looks good, but v and y are very similar.) GulimChe (CJK characters looks good, but v and y are very similar.) MingLiU (should also be all MingLiU_*??) MS Gothic MS Mincho MS PGothic MS PMincho MS UI Gothic PMingLiU (should also be PMingLiU-ExtB??) SimHei (CJK characters looks good, but v and y are very similar.) SimSun (should also be SimSun-ExtB??) Mac: Hei (when the size is large) Kai (when the size is large) Apple LiGothic (when the size is large) Apple LiSung (when the size is large) Osaka (when the size is large) Osaka-mono

This can fix in most cases. gfxFont::SanitizeMetrics() compute the underline offset to bottom of descent. I.e., the underline is moved out from descent space. Therefore, the underline might overlap to next line text when the line-height: 1em;. However, CJK text should have more line-height, generally. Because CJK characters are using the top of the ascent. So, if CJK text is line-height: 1em;, the text is not readable even if there are no underlines. So, we don't need to worry about this issue. gfxFontGroup has first bad font index. We take the lower underline offset of first font (current behavior) and first bad font (new behavior) in standards mode. This can help for very many CJK text rendering. Note that second or later bad fonts are not used in most cases. So, this way is enough. gfxTextRun also can compute the offset from all fonts in it at quirks mode. We should not compute only from the range of the content of textframe. Because if we do so, that makes similar bug of bug 412251. Underline might disjoint between textframes which are in same content. And note that we cannot support this feature on fontconfig platforms. Because: 1. fontconfig has multiple names for a font, we need to resolve it after |gfxPlatform:: ResolveFontName|. 2. One gfxPangoFont is not only one font. So, we need lot of work for this feature on fontconfig platforms. We cannot do before Gecko1.9. First, I need roc's review.

Basically I think this approach is OK. +gfxTextRun::GetUnderlineOffset(gfxFont* aDefaultFont) Why does this need aDefaultFont? The textrun can get the first font from its mFontGroup. I guess it's more efficient if we're already getting the font in the caller to get its metrics, so OK, but add a comment explaining why this parameter is here. Might be better for each gfxFont to store whether it's blacklisted or not. Then we don't need the GetFamilyName API. Also gfxFonts are cached aggressively so this would be more efficient. Actually we should put the blacklist bit on the FontEntry objects managed by Mac and Windows and set it when we construct the FontEntry objects. Instead of storing mFirstBadUnderlineFont in the gfxFontGroup, I would cache the underline offset there and compute it once the first time we call gfxFontGroup::GetUnderlineOffset. Can we share the IsUnderlineBlackListedFont code with other code that parses lists of font names from prefs? + nsPoint& aPt, const nsPoint& ? I don't know why you need to change that though. - // Include the text-decoration lines to the height for readable. Why are you taking this out? Because CombineTextDecorations already does it? + nscoord(NS_round(underlineOffset)), ratio); } NSToCoordRound Make gfxTextRun::GetUnderlingOffset GetUnderlineOffsetDevUnits so you don't need to undo the conversion to appunits? Maybe it would simplify the code if gfxFontGroup::GetUnderlineOffset was replaced by a method gfxFontGroup::GetMetrics, which returns a const gfxFont::Metrics& (obtained from font 0) and also takes a gfxFloat* aOverridingUnderlineOffset to fill in? It might even be best to have it return a gfxFont::Metrics (not reference) with the underline offset overridden in there directly. The cost of copying the gfxFont::Metrics is probably not worth worrying about.

Comment on attachment 304237 [details] [diff] [review] Patch v1.0 > The cost of copying the gfxFont::Metrics is probably not worth worrying about. We used to do this (by accident) and it was showing up in profiles. Also, GetFamilyName should return a const nsString&

(In reply to comment #6) > - // Include the text-decoration lines to the height for readable. > > Why are you taking this out? Because CombineTextDecorations already does it? right. and also gfxFont::SanitizeMetrics did too. > Make gfxTextRun::GetUnderlingOffset GetUnderlineOffsetDevUnits so you don't > need to undo the conversion to appunits? other gfxTextRun methods use appunits, the new method uses same rule. # gfxFloat is used both appunits and devunits, it's too unreadable...

(In reply to comment #6) > Can we share the IsUnderlineBlackListedFont code with other code that parses > lists of font names from prefs? The values are changed by ResolveFontName, so, it's not simply between other similar points. But I have same opinion. I think we should do this after Gecko1.9. (jdaggett also will create the new similar code in bug 390901.)

(In reply to comment #8) > other gfxTextRun methods use appunits, the new method uses same rule. > # gfxFloat is used both appunits and devunits, it's too unreadable... If you put DevUnits in the function name I think it's OK. I see you've done that, thanks :-). It would be great if you could summarize what you changed each time you post a new version of a patch.

+ if (!mStyle.systemFont && gfxPlatform::IsUnderlineBlackListedFont(GetFamilyName())) { Why not take the blacklist status as a parameter? The caller of Sanitize on each platform can give it to you directly from the fontentry (or 'false' on Linux). + gfxFont* firstFont = GetFontAt(0); + if (!firstFont) + return; Can GetFontAt(0) ever actually return null? + mMetrics = firstFont->GetMetrics(); + if (!aFirstBadUnderlineFont) + return; Maybe it would be a bit more efficient to make mMetrics just a pointer to a Metrics. Then if there is no bad font we make it point to the first font's metrics. If there is a bad font we heap-allocate a new Metrics and override its underlineOffset. Only a tiny bit more code and saves quite a bit of space in gfxFontGroup in the common case. + void InitMetrics(gfxFont *aFirstBadFont = nsnull); Don't use a default parameter. Why do you need this USING_FONTCONFIG #define? We're never going to call IsUnderlineBlackListedFont on those platforms. IsUnderlineBlackListedFont should be IsUnderlineBlacklistedFont since "blacklist" is one word. Can't gBadUnderlineFonts just be a member of the gfxPlatform object and IsUnderlineBlacklistedFont be non-static? I wrote the following comment: Maybe it's not worth having text-run-specific underline offsets? It only affects quirks mode anyway since in standards mode, underlines are painted by the inline frames. How about just having textframe use the fontgroup's underline offset always? That would simplify code and we won't need an extra field in gfxTextRun. It would also give more consistent results between standards and quirks mode, which would be good. ... then I realized that you've already done this. Good! But why are GetUnderlineOffsetDevUnits() and mUnderlineOffsetDevUnits still in gfxTextRun since you're not using them?

(In reply to comment #11) > It would be great if you could summarize what you changed each time you post a > new version of a patch. I'm sorry. (In reply to comment #12) > + gfxFont* firstFont = GetFontAt(0); > + if (!firstFont) > + return; > > Can GetFontAt(0) ever actually return null? Yes. At OOM. > ... then I realized that you've already done this. Good! But why are > GetUnderlineOffsetDevUnits() and mUnderlineOffsetDevUnits still in gfxTextRun > since you're not using them? Oops... It's a simple mistake, sorry.

gfxFont::SanitizeMetrics is added a param, therefore, I can drop the GetFamilyName(). And now, gfxFontGroup::GetMetrics returns a pointer of const gfxFont::Metrics. Because if OOM occurred, it cannot return its reference. And nsTextFrameThebes uses gfxTextRun::GetUnderlineOffsetDevUnits value, now. gfxFontGroup::InitMetrics is renamed to gfxFontGroup::InitMetricsForBadFont. And it is called only when the font group has one or more bad fonts. Otherwise, we don't need to call it.

> And nsTextFrameThebes uses gfxTextRun::GetUnderlineOffsetDevUnits value, now. No, I don't want nsTextFrame to use gfxTextRun::GetUnderlineOffsetDevUnits. Did you misunderstand what I wrote? Let me rephrase... Underline drawing in nsTextFrame only affects quirks mode, since in standards mode, underlines are painted by the inline frames. If we let textframe just use the gfxFontGroup underline offset, that would simplify code and we won't need an extra field in gfxTextRun. It would also give more consistent results between standards and quirks mode, which would be good. So we should let textframe just use the gfxFontGroup underline offset and get rid of the textrun changes.

Ugh... Sorry. I didn't think that you change the original idea. Because I didn't think that the idea is not bad. It was just my imagination and it misled my understanding...

no problem. I hope you agree with the change.

Make mSanitizedMetrics an nsAutoPtr. + return mIsBadUnderlineFontFamily ? PR_TRUE : PR_FALSE; mIsBadUnderlineFontFamily != 0 + return mIsBadUnderlineFont ? PR_TRUE : PR_FALSE; similar Nearly done :-)

(In reply to comment #17) > no problem. I hope you agree with the change. Of course, I agree. The same rendering result in both modes is better.

using nsAutoPtr and chenging the PRPackedBool to PRBool converting parts.

Comment on attachment 304841 [details] [diff] [review] Patch v1.4 Thank you, roc. And should be reviewed by Stuart, I think.

Stuart? Would you review this?

Comment on attachment 304841 [details] [diff] [review] Patch v1.4 + // This returns the metrics for this font group. If this font group has + // one or more fonts which have bad underline font metrics (it is in blacklist, + // see gfxPlatform::IsUnderlineBlacklistedFont), this returns sanitized font metrics. + // Other wise this return first font's metrics. + // Note that if OOM occurs, this returns null. + const gfxFont::Metrics* GetMetrics() { + if (mSanitizedMetrics) + return mSanitizedMetrics; + gfxFont* font = GetFontAt(0); + if (!font) + return nsnull; // OOM + return &font->GetMetrics(); + } I'm not really a big fan of this method. This comment should say this returns the first font's metrics rather than the metrics for the font group. It was a design decision to not add a method like this to the font group to require users to get the metrics for the font that they want. style things: please return const reference rather than a pointer to be consistent with the other GetMetrics. Also in thebes we doo 'foo *bar' and not 'foo* bar' Also, if GetFontAt(0) fails we should just crash. Each gfxFont should just have the corrected metrics -- we shouldn't keep around a pointer on the font group to the sanitized metrics of the font group... +PRBool +gfxPlatform::IsUnderlineBlacklistedFont(const nsAString& aFamilyName) I don't think we want to use IndexOf here. This is going to be a lot of iterating over this array.... + for (PRUint32 i = 0; i < mFonts.Length(); ++i) { + gfxAtsuiFont* font = static_cast<gfxAtsuiFont*>(mFonts[i].get()); + if (font->GetFontEntry()->FamilyEntry()->IsBadUnderlineFontFamily()) { + InitMetricsForBadFont(font); + break; + } + } so you want the sanitized metrics on the fontgroup to always be those of the first bad font? Oh, wait, this code is just confusing. Won't this result in us getting the metrics for all the fonts in the fontgroup every time?

(In reply to comment #23) > This comment should say this returns the first font's metrics rather than the > metrics for the font group. It was a design decision to not add a method like > this to the font group to require users to get the metrics for the font that > they want. Unfortunately that isn't completely possible for layout code like nsInlineFrame, which has to lay itself out without knowing which fonts are actually used by its descendants. > Each gfxFont should just have the corrected metrics -- we shouldn't keep > around a pointer on the font group to the sanitized metrics of the font > group... Can't do that, since the sanitized metrics depend on other fonts in the fontgroup. > so you want the sanitized metrics on the fontgroup to always be those of the > first bad font? No, just the underline offset. > Oh, wait, this code is just confusing. Won't this result in > us getting the metrics for all the fonts in the fontgroup every time? No, we get the metrics for the first font, and the first "bad" font if there is one.

This changes: * gfxFontGroup::GetMetrics() to gfxFontGroup::GetUnderlineOffset(). * gfxFontGroup uses mUnderlineOffset which is initialized from each platform's sub class of gfxFontGroup. * now, dropping Osaka-mono from blacklist. Because it is *very* bad font :-( If it is in blacklist, the underline is too far. But if it is not in blacklist, the underline overlaps to text. However, the later is rare case. We should give up now for Osaka-mono. * gfxPlatform::IsUnderlineBlacklistedFont is removed, on initializing the font list on Win and Mac, it is checked and set the flag to FontEntry/MacOSFamilyEntry. * gfxQuartzFontCache::InitSingleFaceList() doesn't change the FontEntry's FontFamilyEntry. If we found another font family for Osaka-mono, the FontFamily should refer the new FamilyEntry. * gfxFont::SanitizeMetrics() is now checking that the internalLeading and the externalLeading are there. If the font has enough leadings, the underline might be positioned to *below* of the descent space. Otherwise, it might be positioned to *bottom* of the descent space.

Moving bugs that aren't beta 4 blockers to target final release.

Stuart: I need to work bug 421353. That changes the layout of some pages. So, it should be fixed 1.9b5 if we land it on Gecko1.9. However, the changing points are same as this bug, so, I cannot work on that after landing this patch. Please hurry up to review this.

checked-in, thanks.