[Webkit-unassigned] [Bug 40555] New: line-height handling in rendering code on font fallbacks

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40555

           Summary: line-height handling in rendering code on font
                    fallbacks
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: Macintosh
        OS/Version: All
            Status: UNCONFIRMED
          Severity: Normal
          Priority: P2
         Component: Layout and Rendering
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: gzjjgod at gmail.com


Created an attachment (id=58616)
 --> (https://bugs.webkit.org/attachment.cgi?id=58616)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.