The line-height of some Japanese fonts which have no vertical spacing should default to 1.2, but actually it's almost 1.0 with some fonts. Linux: see Bug 425004 Comment #25 Windows testcase: attachment 368855 [details] screenshot: attachment 368856 [details] Mac testcase: attachment 368855 [details] screenshot: http://bugzilla.mozilla.gr.jp/attachment.cgi?id=4020

According to http://www.w3.org/TR/CSS21/visudet.html#line-height line-height:normal "Tells user agents to set the used value to a "reasonable" value based on the font of the element. The value has the same meaning as <number>. We recommend a used value for 'normal' between 1.0 to 1.2." So in most situations we use a line-height provided by the font designer (or sometimes maxAscent + maxDescent or maxHeight if that is larger). However, in many situations (perhaps only associated with CJK fonts) the font designer's line-height is not sufficient. There are a number of screenshots showing the problems in bug 76097 comment 111 and following comments. To correct these situations we have this code: http://hg.mozilla.org/mozilla-central/file/956071116564/layout/generic/nsHTMLReflowState.cpp#l2024 GetNormalLineHeightCalcControl() returns eCompensateLeading. This was added in bug 76097 for bug 63650. The reasoning given in bug 76097 comment 76 is "add extra external leading (compensation) if font vendor does not provide either internal or external leading." Similarly the comment says // If both internal leading and external leading specified by font itself // are zeros, we should compensate this by creating extra (external) leading // in eCompensateLeading mode. This compensation is not being applied when internal or external leading is only close to zero. The problem seems to be that these internal and external leading metrics are not metrics direct from the font (expect perhaps external leading on Mac) but are calculated as functions of other metrics. Other metrics are at least sometimes rounded, and so the leadings end up being non-zero. If we are going to try to make a guess as to whether the font-designer has included appropriate leading then I think we need to look at the metrics in the font itself (in GFX code). The best metric in the font itself would seem to be sTypoLineGap, which is pretty much internal + external leading if we ignore windows extending the externalLeading to include usWinAscent + usWinDescent. Perhaps we could also consider LineGap (from hhea) but I suspect this will often be zero because the designer has included leading in the Ascender/Descender. I'm imagining something like: if (sTypoLineGap == 0 && LineGap == 0) lineheight = MAX(NORMAL_LINE_HEIGHT_FACTOR * emHeight, maxHeight); and then set leadings appropriately for the lineHeight. One possible problem here is that Ahem matches this test (and probably matches with the existing test, sometimes), but using 1.2 for a line-height seems within the requirements of the CSS specification. Thoughts?

Sounds good for me. And if some tests fail by the changes, it should be the bug(s) of the tests. line-height: normal; depends on CSS-UAs. So, that should not be compared with fixed layout.

Ccing Saito-san. Saito-san, can this interest you?

I can get following font metrics on my linux system with using pango library directly. I think that the descent of IPAPGothic is too small so also I can't see an underline in a box. "Bitstream Vera Sans 50" ascent=65.000000 descent=17.000000 char_width=30.467773 approximate_digit_width=44.000000 underline_position=-7.000000 underline_thickness=5.000000 strikethrough_position=21.000000 strikethrough_thickness=5.000000 "IPAPGothic 50" ascent=62.000000 descent=9.000000 char_width=30.467773 approximate_digit_width=43.000000 underline_position=-15.000000 underline_thickness=4.000000 strikethrough_position=29.000000 strikethrough_thickness=5.000000 How about following approach? --- gfx/thebes/src/gfxPangoFonts.cpp.org 2008-12-30 23:49:57.000000000 +0900 +++ gfx/thebes/src/gfxPangoFonts.cpp 2009-04-23 13:01:47.000000000 +0900 @@ -927,16 +927,20 @@ gfxPangoFont::GetMetrics() pango_font_metrics_get_underline_thickness(pfm) / FLOAT_PANGO_SCALE; mMetrics.strikeoutOffset = pango_font_metrics_get_strikethrough_position(pfm) / FLOAT_PANGO_SCALE; mMetrics.strikeoutSize = pango_font_metrics_get_strikethrough_thickness(pfm) / FLOAT_PANGO_SCALE; + // the descent should include the underline + if ((mMetrics.underlineSize - mMetrics.underlineOffset) > mMetrics.maxDescent) + mMetrics.maxDescent = mMetrics.underlineSize - mMetrics.underlineOffset; + // We're going to overwrite this below if we have a FT_Face // (which we normally should have...). mMetrics.maxAdvance = mMetrics.aveCharWidth; } else { mMetrics.maxAscent = 0.0; mMetrics.maxDescent = 0.0; mMetrics.aveCharWidth = 0.0; mMetrics.underlineOffset = -1.0;

In a way the underline is part of the font, so ensuring that the descent includes the underline makes some sense. But sometimes we are setting the underline offset to the descent: http://hg.mozilla.org/mozilla-central/file/b822a565b4e6/gfx/thebes/src/gfxFont.cpp#l728 Masayuki would know more about what fonts have bad underline offset: http://hg.mozilla.org/mozilla-central/rev/7d03ce447802 I guess the descent is probably more likely to be correct than the underline. Masayuki: do you think, adding enough leading to accommodate the underline could be an option?

"The descent must include the underline" is good solution for the daily use, probably. But I already designed so. See gfxFont::SanitizeMetrics which is pointed by Karl in comment 5. If the Saito-san's suggestion fixes some problems, we have some bugs in gfxFont::SanitizeMetrics or layout level :-( I think that we need to expand the leading of CJK fonts if it's too narrow. Because most CJK characters use the character cell fully for the glyphs. So, the narrow line-height can cause the unreadable. (Therefore, the line-height computation does it.) This bug's theme should be that we should not check as 0. Instead of that, we should check whether the leading is narrow or not.

I think that the descent doesn't have enough room because the rendering goes through an underline and the text scrolls on the input field in the box. As to IPAPGothic, I think that the descent should be extended than the underline is placed upward.

How about following computing for normal line height if the leading is less than 10% of emHeight? diff -p8 -Naur ./layout/generic/nsHTMLReflowState.cpp.org ./layout/generic/nsHTMLReflowState.cpp --- ./layout/generic/nsHTMLReflowState.cpp.org Sat Jun 14 09:34:09 2008 +++ ./layout/generic/nsHTMLReflowState.cpp Sun May 10 02:31:41 2009 @@ -2016,20 +2016,20 @@ GetNormalLineHeight(nsIFontMetrics* aFon aFontMetrics->GetExternalLeading(externalLeading); aFontMetrics->GetInternalLeading(internalLeading); aFontMetrics->GetEmHeight(emHeight); switch (GetNormalLineHeightCalcControl()) { case eIncludeExternalLeading: normalLineHeight = emHeight+ internalLeading + externalLeading; break; case eCompensateLeading: - if (!internalLeading && !externalLeading) + if ((internalLeading + externalLeading) * 1.0f / emHeight < (NORMAL_LINE_HEIGHT_FACTOR - 1.0f) / 2) normalLineHeight = NSToCoordRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR); else - normalLineHeight = emHeight+ internalLeading + externalLeading; + normalLineHeight = emHeight + internalLeading + externalLeading; break; default: //case eNoExternalLeading: normalLineHeight = emHeight + internalLeading; } return normalLineHeight; }

Attachment 376361 [details] seems to be shown properly. However I don't know which bug fix this issue.

I tested attachment 368855 [details] by recent nightly build on Linux and Windows 10. I cannot reproduce on both environment. Can anyone still reproduce?