We have several reports (e.g., bug 396809, bug 400141 and bug 380026) which are underline correctness issues. They are not platform issue, so *they are font issue*. Therefore, we should fix the bugs in XP level. The patch will come soon.

This should fix the all problems. But this patch cannot fix following case: <span lang="en-US">漢字</span> This is an invalid case, but this case is existing on the Web. For this bug, we need to check whether the font supports CJK, but it cannot do now. (bug 366664 should be fixed first.)

fmmm... the patch makes damage to UI on CJK locale when using western fonts for menus. We need another approach...

This patch changes the underline offset if the font has 'あ' or '一' or '가' on Win/Mac/Pango. 'あ' is U+3042, this is Hiragana of Japanese. '一' is U+4E00, this is CJK ideograph's first character. '가' is U+AAC0, this is Hangul's first character. I need to test on Linux and to write the code for OS/2.

This should work fine. If the font is have CJK glyphs, we ignore the underline offset of font. Instead of it, we use the bottom of the descent. gfxFont::CorrectMetrics has the hack for bug 353632. The same problem can be reproduced on Linux, but not on Mac. I think that Mac might corrects the values in their APIs. So, the issue should be XP level, and we should hack it in XP level. And gfxFont::CorrectMetrics has following functions: 1. it guarantees the underline height and the strikeline height. they are one pixel at minimum. 2. the maximum of underline offset is -1, not 0. This guarantees the underline doesn't adhere to the characters. 3. But finally, underline offset is moved to descent space. (Or if it is CJK fonts, it might be moved to the bottom of descender, and it makes wider line by spreading the descent and font height.) And I removed the changes by bug 380026. It is wrong approach. And this patch also re-fixes bug 380026.

the patch is changing the OS/2 code, OS/2 team should check it.

masayuki: thanks for thinking about us. :-) The OS/2 part of the code makes sense to me and compiles. However, I am not sure that in the end this patch really helps. At least on OS/2 we replace many glyphs from standard Unicode fonts (well, at least soon, my patch for that isn't finished yet). And because those fonts usually contain CJK glyphs this patch causes very low underlines for languages as different as Thai, Ivrit, and Eesti (I checked this on the http://wikipedia.org/ page) which before looked much better.

(In reply to comment #6) > However, I am not sure that in the end this patch really helps. At least on > OS/2 we replace many glyphs from standard Unicode fonts (well, at least soon, > my patch for that isn't finished yet). And because those fonts usually contain > CJK glyphs this patch causes very low underlines for languages as different as > Thai, Ivrit, and Eesti (I checked this on the http://wikipedia.org/ page) which > before looked much better. It's really good point. I didn't think about Unicode fonts. I'll post new patch which also checks the lang of font style.

This should work fine with Unicode font.

This only checks the actual glyph if the content is CJK. So, other language doesn't have large damage.

using FontEntry on Mac.

Thanks, rv3.0 is much better. :-) You should use gfxOS2Font::HasCJKGlyphs (not gfxPangoFont) in the OS/2 file and I think you also have to cairo_ft_scaled_font_unlock_face() in that function. Otherwise I'm happy.

thank you!

Comment on attachment 287668 [details] [diff] [review] Patch rv3.1 this has a bug, next patch is coming soon.

Comment on attachment 287668 [details] [diff] [review] Patch rv3.1 Oops, sorry. I mistook the bug number.

+ return mFontEntry->SupportsRange(sHiragana) || + mFontEntry->SupportsRange(sIdeograph) || + mFontEntry->SupportsRange(sHangul); you should be using mFontEntry->mCharacterMap->test() instead of these.

I think that the cause of bug 406729 is this bug. Stuart, I hope that you restart the review ASAP.

updating for latest trunk.

Updating for latest trunk too. Stuart, please hurry up to review this. This bug blocks other bugs!

And note that this bug's changing is already used on legacy gfx for Win32. The corrects are not Win32 API dependent issues, they depends on fonts. Therefore, we need the code in XP level.

I'm still looking at this but to these heuristics apply to all ideographic fonts? Beyond just MS Gothic? And do these metrics render Latin text rendered with glyphs from these fonts correctly? I wish there was a way of pulling this more directly out of the font information, something in the OS/2 perhaps? Relying on "does the font have glyphs in cmap range xxx?" as a test makes me a bit nervous. Also, which of the dependent bugs is best to look at see problems that this bug will help fix?

I think that the risk of new side effects is low. Because this patch's logic is already used in legacy win32 gfx. (legacy mac gfx didn't refer to font metrics for underline/strikeout line.) And this patch has the patches for bug 353632, bug 361576 and bug 380026. They are only fixed on one platform, but they are not API issues, so, we should fix it on all platforms. bug 406729 is a regression of bug 392785. That is fixed by this patch. So, I cannot check-in the patch for bug 392785 before this. bug 396809 and bug 400141 are fixed by this patch.

(In reply to comment #21) > I'm still looking at this but to these heuristics apply to all ideographic > fonts? Beyond just MS Gothic? And do these metrics render Latin text rendered > with glyphs from these fonts correctly? What is the answer to this? > > I wish there was a way of pulling this more directly out of the font > information, something in the OS/2 perhaps? Relying on "does the font have > glyphs in cmap range xxx?" as a test makes me a bit nervous. > I agree with this. We should certainly change the mac code to be faster like the windows code for this lookup if we're to keep it, but I really don't like the idea of "does this have any CJK glyphs." There really needs to be a better way.

Added another testcase to 400141. Also, I think the patch needs to be updated, there were some changes made to gfxPangoFonts.h.

Screenshot shows FF2 vs. FF3 w/ patch 3.3 on Mac. Under FF2 the underline was displayed very close against the baseline while in FF3 with patch 3.3 it displays a disjoint underline.

Safari renders the baseline tight against the text.

Looks like IE simply renders the underline lower than FF/Safari for all text.

(In reply to comment #23) > (In reply to comment #21) > > I'm still looking at this but to these heuristics apply to all ideographic > > fonts? Beyond just MS Gothic? And do these metrics render Latin text rendered > > with glyphs from these fonts correctly? > > What is the answer to this? Is this underline issue? Or Superscript/Subscript rendering? I think that the underline issue is not only for MS Gothic. But Superscript and Subscript issue is only for it. If the font has CJK glyphs, the underline is rendered to bottom of descent even if the text is Latin text. However, I believe that CJK users like such rendering. > > I wish there was a way of pulling this more directly out of the font > > information, something in the OS/2 perhaps? Relying on "does the font have > > glyphs in cmap range xxx?" as a test makes me a bit nervous. > > > > I agree with this. We should certainly change the mac code to be faster like > the windows code for this lookup if we're to keep it, but I really don't like > the idea of "does this have any CJK glyphs." There really needs to be a better > way. Do you have the idea? Note that Safari/IE/Opera don't prefer the underline offset/size of fonts.

(In reply to comment #28) > Note that Safari/IE/Opera don't prefer the underline offset/size of fonts. And legacy Mac gfx code is too.

Current my patch breaks Meiryo's underline position, but Fx2 doesn't break it. I failed to port the underline position logic from legacy win gfx :-( The reason is that the legacy gfx uses otmDescent for computing. Most CJK fonts returns same value for otmDescent and tmDescent, However, Meiryo doesn't so...

updating for comment 30. In gfxWindowsFont, it is assumed that internal leading is always included only in ascent. But Meiryo is not so, it has very large internal leading and only a part of it is in ascent. We should prefer the otmAscent and otmDescent for emAscent and emDescent. (On Linux and Mac, we don't assume that the internal leading is only in ascent.)

+ +PRBool +gfxAtsuiFont::HasCJKGlyphs() +{ + return mFontEntry->TestCharacterMap(PRUnichar(0x3042)) || + mFontEntry->TestCharacterMap(PRUnichar(0x4E00)) || + mFontEntry->TestCharacterMap(PRUnichar(0xAAC0)); +} + I still don't like doing this for any platform. We need to come up with a better way to tell if we need to adjust the metrics.

(In reply to comment #32) > + > +PRBool > +gfxAtsuiFont::HasCJKGlyphs() > +{ > + return mFontEntry->TestCharacterMap(PRUnichar(0x3042)) || > + mFontEntry->TestCharacterMap(PRUnichar(0x4E00)) || > + mFontEntry->TestCharacterMap(PRUnichar(0xAAC0)); > +} > + > > I still don't like doing this for any platform. We need to come up with a > better way to tell if we need to adjust the metrics. The reason of I added it is for Linux UIs. Linux UIs are rendered Western Font + user locale's font. And lang of font of UIs is system locale. So, if we don't check it, Linux UI's underline is broken. I'll try to use system font checking instead of glyph checking.

Using system font checking instead of CJK glyph checking.

minimized patch.

would you continue to review?

Stuart: Would you continue to review for new patch?

i don't understand this new patch either. the langgroup is often wrong coming from the page or defaulting to western. if this enough? also, correcting these metrics make in this way make you end up with different baselines for mixed japanese/english text which seems out of whack. Imho, we either need to lower the entire baseline for all fonts or not do this. If we really want some similar approach we need to detect in some way, from the font, that we should do this and not try to make guesses from the language group or other typically poor information we have about the state we're in. Imagine mixed text runs here -- things aren't going to look very good.

(In reply to comment #38) > the langgroup is often wrong coming from the page or defaulting to western. if this enough? The lang of the elements of UIs and the lang of lang non-specified elements in UTF-8 pages are same as system locale. > also, correcting these metrics make in this way make you end up with > different baselines for mixed japanese/english text which seems out of > whack. It's true in quriks mode if each languages text are in different textframe (e.g., in different element). However, standards mode only uses first font metrics of text-decoration specified. > Imho, we either need to lower the entire baseline for all fonts or not do > this. If we really want some similar approach we need to detect in some way, > from the font, that we should do this and not try to make guesses from the > language group or other typically poor information we have about the state > we're in. Imagine mixed text runs here -- things aren't going to look very good. Your only worry is decoration lines disjoint problems between different fonts? If so, it should be fixed on layout level. not gfx. And note that it's rare case I think. Because layout code uses first font's metrics for decoration lines. So, actually, we can look the problem with between different font element, e.g., one is lang="en-US" and other is lang="ja".

(In reply to comment #38) > i don't understand this new patch either. the langgroup is often wrong coming > from the page or defaulting to western. if this enough? So, I meant in previous comment that for the users whose system locale is CJK, they can look the corrected decoration lines in most pages, but UIs are not broken by the new checking. I think that this is a best workaround for now.

Stuart: How do you think for my comment?

If we use the first font's lowered metrics followed by non-cjk don't we end up with the underline now running through english text?

(In reply to comment #42) > If we use the first font's lowered metrics followed by non-cjk don't we end up > with the underline now running through english text? No. The latest patch doesn't check whether the font is CJK or not. And the first font's lower point is used for underline offset only if the lang attr of the element is CJK. If the lowest point of first font is higher than second or later CJK font's lowest point, the line might be overlapped to the text, however, I think that the case is very rare. Because CJK fonts don't have larger descent then Western fonts generally (CJK characters doesn't need descent). And the case of non-CJK fonts are listed after CJK fonts is very rare case. Because most CJK fonts have Western characters, and other characters are not used in most documents.

And even if you think we should check the all font metrics for the text, it's not in gfx level issue, it should be layout level.

This is testcase for underline position of some font-family cases. 1. font-family has Japanese font at first, and Western font at second. This case is very rare case. Because second font might be ignored. 2. font-family has Western font at first, and Japanese font at second. This case is major case in Japan. 3. font-family only has Japanese font. This case is used in most sites in Japan. And for each cases, lang attr is set "en-US" or "ja" or "ar". And for experimental, the case of Japanese new system font on Vista is last. It has very large internal leading, it is larger than general western fonts.

The underline is too near the Kanji character("MS PGothic is Japanese default font on Windows, and the case (font-size: 19px) is worst case). And the glyph in descent is overlapped by the underline. Therefore, the text is not pretty. "Meiryo" cases (in most bottom table row) are good, it has suitable values for underline offset. Note that in the first cases (font-family: ja, en, others;), the western font (Times New Roman) is not used. Because most CJK fonts have western glyphs. (If they don't have the western characters, we cannot use them.)

This is a screenshot of patched build. Only when the lang is ja, the underline positions are corrected. Kanji characters are not overlapped by the underline. And descent of western characters are very clear. Note that Japanese people might not like the underline being positioned to baseline position. This screenshot is similar to Japanese typographic. And "Meiryo" has very large internal leading, but this patch suppresses to the underline being positioned too far from the text.

I don't know much about this issue, but if the problem is just that certain popular fonts have a bad underline-offset value, then I'm comfortable with just blacklisting certain font names at an XP level, giving them special treatment such as increasing the underline offset to the descent of the font.

roc: Thank you for you advice. However, the blacklist approach cannot save following case: font-family: "non-blacklisted western font", "blacklisted CJK font"; We decide the underline position only from first font metrics, Therefore, non adjusted underline might overlap to CJK characters. Some major sites (generally, official sites of major companies) uses such font-family specification. Therefore, I bet your approach cannot solve this problem... Ideally, we should check all used fonts for the text (or all fonts for the fontgroup) for deciding the underline position, but it is not realistic now.

And I hope that the underline of Western fonts is also positioned at bottom of descent space. It's natural for CJK users (or CJK typography), I think.

(In reply to comment #50) > And I hope that the underline of Western fonts is also positioned at bottom of > descent space. It's natural for CJK users (or CJK typography), I think. Of course, only in CJK context.

> it is not realistic now. Why not? It doesn't sound very hard to me.

(In reply to comment #52) > > it is not realistic now. > > Why not? It doesn't sound very hard to me. We are in b4 cycle, now. Is there enough testing term? How many weeks we have for Gecko1.9?

O.K. I'll post a new patch which doesn't have the underline adjusting for CJK fonts. It should be landed in early time for other bugs. And the CJK fonts issue will be filed to new bug, thanks.

This patch is removed the part for CJK fonts underline offset adjusting from previous patch. So, by this landing, bug 380026 will be reopened, but bug 392785 and bug 396809 are will be marked as FIXED.

Stuart: gfxFont::AdjustFontMetrics is better name?

karlt suggested "SanitizeMetrics". It sounds good. Thank you, karlt!

only renames.

Oops, I forgot to change the comment, sorry for the spam.

checked-in! Thank you stuart!