Bug 40555 – line-height handling in rendering code on font fallbacks

• Status: UNCONFIRMED
• Component: Layout and Rendering

[Jiang Jiang]

Created attachment 58616 [details]
Test case for this issue

I believe the line-height handling in WebCore/rendering/InlineFlowBox.cpp has a bug.

The InlineFlowBox::computeLogicalBoxHeights() functions calculates the maxAscent and maxDescent like this (typical code path):

1) First it will retrieve lineHeight from the stylesheet and baselinePosition from the font, then use baselinePosition as the maxAscent and (lineHeight - maxAscent) as maxDescent. It is reasonable, but has a potential problem which I am going to show later.
2) Then it will enumerate through all the inline boxes, for each box, it will try to find all the fonts used in that box, for each of these fonts, it will find out the max baseline position from the ascent and descent value specified by the font, with the following code:

                lineHeight = parentLineHeight.value();
                baseline = 0;
                for (size_t i = 0; i < usedFonts->size(); ++i) {
                    int halfLeading = (lineHeight - usedFonts->at(i)->ascent() - usedFonts->at(i)->descent()) / 2;
                    baseline = max(baseline, halfLeading + usedFonts->at(i)->ascent());
                }

3) Then it will use this baseline to calculate minDescent again: 

            curr->setY(verticalPositionForBox(curr, m_firstLine));
            int ascent = baseline - curr->y();
            int descent = lineHeight - ascent;
            if (maxAscent < ascent)
                maxAscent = ascent;
            if (maxDescent < descent)
                maxDescent = descent;

4) The resulting lineHeight will be maxAscent + maxDescent

Now let's see where is the problem. Here is an example, suppose we have a page with style:

    p {
        font-family: STSong, serif;
        font-size: 14px;
        line-height: 22px;
    }

The STSong font has ascent = 12, descent = 2, height = 14; the default serif font (Times New Roman) has ascent = 13, descent = 4, height = 17.

1. At first, we have lineHeight = 22, baseline = ascent + (lineHeight - height) / 2 = (13 + (22 - 17) / 2) = 15, then will can get maxAscent = 15, maxDescent = 22 - 15 = 7, as in step (1);
2. When we enumerating all the fallback fonts (STSong especially), we found halfLeading = (22 - 12 - 2) / 2 = 4, halfLeading + ascent = 16, it's larger than the original baseline (15), so we replace the baseline with 16, as in step (2)
3. Finally, we calculate the ascent to be 16 (assuming curr->y() = 0 at this point), descent = 22 - 16 = 6, then we found ascent > maxDescent, so we set maxAscent = 16, but maxDescent (7) is larger than descent (6), so we ignore the new descent. (step 3)
4. As a result, the lineHeight become 16 + 7 = 23, which unnecessarily exceeds the line-height specified in stylesheet. While in fact, all the fonts used in here only need maxAscent = 13, maxDescent = 4.

So, in my opinion, the correct value here is maxAscent = 16, maxDescent = 6. In short, when we increase the baseline in step 2, we need to decrease the maxDescent accordingly, otherwise the resulting lineHeight will exceeds the value originally specified.

A test case is attached.

Created attachment 59330 [details]
Screen shot of attachment 58616 [details] in Safari 5.0 on Mac OS X 10.5.6

The line spacing in the test case is 22px. Which version of Safari, WebKit and Mac OS X are you seeing 23px on? Can you please attach a screenshot? Thanks!

[Jiang Jiang]

Created attachment 59335 [details]
Corrected test case

In Mac OS X, the fallback font will be STSong for Simplified Chinese.

[Jiang Jiang]

The previous test case got the "font-family" wrong since I put "STSong" before "serif", so it will use STSong whenever possible. In the corrected test case, only "serif" is specified (or you can put "STSong" after it), then both Times New Roman and STSong will be used. Times New Roman will be used for Latin characters, and STSong will be used for Chinese characters. That's when the line-heights get mixed up.

(You can compare both test cases to see the differences.)

(In reply to comment #2)
> Created an attachment (id=59335) [details]
> Corrected test case
> 
> In Mac OS X, the fallback font will be STSong for Simplified Chinese.

In this new example, at least on my system, some characters (e.g. U+4EA4 交) use HiraMinProN-W3 as the fallback font, while others use STSong. HiraMinProN-W3 has different metrics. Do you have an example involving only two fonts?

[Jiang Jiang]

(In reply to comment #4)
> (In reply to comment #2)
> > Created an attachment (id=59335) [details] [details]
> > Corrected test case
> > 
> > In Mac OS X, the fallback font will be STSong for Simplified Chinese.
> 
> In this new example, at least on my system, some characters (e.g. U+4EA4 交) use HiraMinProN-W3 as the fallback font, while others use STSong. HiraMinProN-W3 has different metrics. Do you have an example involving only two fonts?

Oh, that's because in System Preferences -> Language & Text -> Language, you have “日本语” before "简体中文". You can drag "简体中文" before “日本语”.

To limit this test case to only two fonts, you can put "STSong" right after "serif" as I described in my previous comment. I will attach a new test case.

[Jiang Jiang]

Created attachment 59339 [details]
Test case that only applies to Times New Roman and STSong

Created attachment 59342 [details]
Test case not involving font fallback

First of all, this behavior is not unique to font fallback. This test case shows how with all fonts being specified explicitly and no fallback occurring, the line height is 23px as well. Secondly, I believe that this behavior is correct, and Firefox renders this test case the same, with a 23px line height. The cases involving font fallback just mimic the same behavior, i.e. they always behave as if the runs using fallback fonts were inlines explicitly specifying the fallback families.

[Jiang Jiang]

Created attachment 59343 [details]
Compare the results in Safari 5 (left) and Firefox 3.6.3 (right)

(In reply to comment #7)
> Created an attachment (id=59342) [details]
> Test case not involving font fallback
> 
> First of all, this behavior is not unique to font fallback. This test case shows how with all fonts being specified explicitly and no fallback occurring, the line height is 23px as well. Secondly, I believe that this behavior is correct, and Firefox renders this test case the same, with a 23px line height. The cases involving font fallback just mimic the same behavior, i.e. they always behave as if the runs using fallback fonts were inlines explicitly specifying the fallback families.

You are right about this behavior is not unique to font fallback, but are you sure Firefox renders this test case the same? In my setup, Firefox 3.6.3 renders it in constant 22px line height. (If you compare a longer test case as I provided, the container box is obviously higher than that in Safari/WebKit, see attached screenshot)

About what is the correct behavior, let's consider this: suppose we have a constant baseline which both Times New Roman and STSong will be put on, above this baseline, the max ascent is 13px, below it, max descent is 4px, so the line height specified (22px) is more than enough to fit in both fonts, why should we extend it to 23px?