Bug 49410 – WebKit mishandles line boxes where the content height exceeds the specified line-height.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

WebKit mishandles line boxes where the content height exceeds the specified line-height.

[Andy Estes]

Created attachment 73672 [details]
Test case

When a line height is specified on a line box but the maximum ascent and descent of the box's inline elements exceed the line height, the box is sized according to the content height rather than the line height. CSS 2.1 states in section 10.8.1:

"When the 'line-height' value is less than the content height, the final inline box height will be less than the font size and the rendered glyphs will "bleed" outside the box. If such a box touches the edge of a line box, the rendered glyphs will also "bleed" into the adjoining line box."

This indicates that we should size the inline box according to the line height even if the content height is larger. To see the effect of this, compare the attached test case between WebKit and Gecko or IE.

[Andy Estes]

Patch on the way.

[Andy Estes]

<rdar://problem/7863974>

[Andy Estes]

Created attachment 73676 [details]
Patch

[Andy Estes]

I just noticed that there is now some obvious overlap in mathml/presentation/row.xhtml where there wasn't before. I will look into why that is.

[Dave Hyatt]

Created attachment 73690 [details]
Patch

Seems like the math is fine in the current function actually... it just wasn't prepared for negative values (since it starts maxAscent and maxDescent off at 0).

I think this fixes it?

[Andy Estes]

(In reply to comment #6)
> Created an attachment (id=73690) [details]
> Patch
> 
> Seems like the math is fine in the current function actually... it just wasn't prepared for negative values (since it starts maxAscent and maxDescent off at 0).
> 
> I think this fixes it?

It looks like this patch fixes the test case attached to this bug but not the one in Radar, which involves fallback fonts. I'll attach that test case here.

[Andy Estes]

Created attachment 73695 [details]
Test case #2

This should pass with either patch.

[Andy Estes]

Created attachment 73696 [details]
Test case #3

This should fail with hyatt's patch.

[Dave Hyatt]

I didn't patch the usedFonts code.  It should be pretty straightforward to do so.

[Dave Hyatt]

Basically there's nothing magical or special about negative line-height vs. positive line-height.  computeLogicalBoxHeights just needs to do the right thing.  You shouldn't need any extra helper functions to make the math work out.

In computeLogicalBoxHeights, the line-height is included.  This means that ascent is the distance from the top of the box including line height to the baseline of the font.  Descent is the distance from the bottom of the box including line height to the baseline of the font.

So if you have a font with a 30px ascent and a 10px descent, and you specify a 10px line height, then the leading is split above and below, so 15px shrinkage above and 15px shrinkage below.  This turns the ascent into 15px and the descent into -5px.

The math is the same regardless of if the line-height is positive or negative.  The problem is just that maxDescent and maxAscent are starting at 0 and only being allowed to grow larger, so negative values were not being properly included.

The usedFont code is presumably just not computing ascent and descent properly with this rule in mind.

I've been meaning to rename computeLogicalBoxHeights to make it more clear what is going on in that function.  The term "logical" is heavily overloaded here, but in this context it means "compute a position including line-height first", and then the function that really places the boxes adjusts the boxes to positions that exclude line-height.

[Dave Hyatt]

Created attachment 73713 [details]
Patch

Here's a prototype patch that fixes the bugs in usedFonts.  They have the exact same problem (initializing values to 0 and then maxing).   I think the usedFont code could actually be cleaned up a fair bit, but I don't know if you want to do that cleanup at the same time or not.  (I don't think those 3 separate cases are necessary that each loop through the usedFonts.)

[Dave Hyatt]

Created attachment 73714 [details]
Patch

The test case #3 that fails doesn't have anything to do with negative leading, so you're right that it is not fixed by this patch.

The issue is in the next function, where we convert to the real box position (ignoring line height).  The problem is when we do that conversion, we're using the primary font's ascent and not the max ascent of all our used fonts, and so we've lost the fact that a different ascent might have actually been used for our box.  This leads us to place the box in the wrong place.

Your patch does make the red line unbroken, since you mutate the height of the block (but you didn't really correct the placement of the line boxes themselves, which are still wrong by 1px).

Really computeLogicalBoxHeights is using the logicalTop of the boxes as scratch space, but in order to correct to the right value, we need to know the actual max ascent that was used for the particular box.  This means placeBoxesInBlockDIrection either needs to look at the usedFonts again, or we need to find a way to cache that information.

Anyway, I kind of view the negative leading problem and the usedFonts problem as two separate issues, so I wouldn't try to fix both at the same time.  This patch addresses the negative leading issue.

[Dave Hyatt]

(In reply to comment #13)
> 
> Your patch does make the red line unbroken, since you mutate the height of the block (but you didn't really correct the placement of the line boxes themselves, which are still wrong by 1px).
> 
> Really computeLogicalBoxHeights is using the logicalTop of the boxes as scratch space, but in order to correct to the right value, we need to know the actual max ascent that was used for the particular box.  This means placeBoxesInBlockDIrection either needs to look at the usedFonts again, or we need to find a way to cache that information.
> 

computeLogicalBoxHeights also has the issue that it is being fooled by distinct fonts that both share the same line height.  It does seem like your patch is on the right track in this regard, in that you probably need to track a maxHeight so that you don't incorrectly total maxAscent and maxDescent to get something that is too large.

[Dave Hyatt]

Created attachment 73723 [details]
Another patch to try.

I'm kind of confused now why we even bother examining usedFonts when the line-height is explicitly set.  What purpose does it serve?  

We clearly don't size the InlineTextBoxes around the used fonts when they're bigger, since we only look at the primary font's height and ascent to set the top and height of the InlineBox.  This means used fonts paint along the same baseline and spill out the top of the InlineTextBox.  Are they counted as overflow?  This would be one way to deal with used fonts, and then their desire to grow the line-height would only kick in when the line-height isn't explicitly set.

As things stand right now, I think there is no reason for used fonts to be examined except when line-height is unset.

Dan should probably look at this and comment.

[Dave Hyatt]

Thought about this some more after sleeping, and I don't think the rendering with the 1px offset red lines is incorrect if you're going to use both the primary font and the used fonts.  You have 2 30px boxes (the root or the anchor) with 19px ascents and you have the text box with the used font with an ascent of 20px.  Once you align both boxes on their baselines, you end up with 31px of height.

I don't know why Firefox manages an unbroken red line.  EIther they're ignoring used fonts in line height calculations when the line height is set (which is what my latest patch does), they're somehow detecting exactly what font is used and thus not using the primary font's metrics anywhere (propagating even up to to the inline flows and the root box), or maybe they don't consider used fonts at all.

Anyway, sorry for writing a book in this bug, but I would definitely address the negative leading bug separately, since it really is a separate issue from the used fonts issue.

My next-to-last patch addresses the negative leading issue without worrying about the usedFonts issue.

[Dave Hyatt]

Broke out the negative leading issue into another bug:

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

I have a patch up for review.

[Andy Estes]

Created attachment 74068 [details]
Patch to usedFonts ascent/descent computation.

This change rebaselines a number of tests but does not include updated pixel results. I'll include those when landing.

[Dave Hyatt]

Comment on attachment 74068 [details]
Patch to usedFonts ascent/descent computation.

r=me.. we should file a followup bug about the poor font choice for all of the symbols.

[Andy Estes]

(In reply to comment #19)
> (From update of attachment 74068 [details])
> r=me.. we should file a followup bug about the poor font choice for all of the symbols.

I'll file one. Thanks for the review!

[Andy Estes]

Checked in http://trac.webkit.org/changeset/72173.

[Andy Estes]

Filed https://bugs.webkit.org/show_bug.cgi?id=49648 to track the font issue with css2.1 tests.

[Andy Estes]

Checked in new results for editing tests in http://trac.webkit.org/changeset/72177.

[Andy Estes]

Checked in new results for a failing transform test and fast/text tests that always fail on my machine due to installed fonts (I pulled results from the bots for these tests).

http://trac.webkit.org/changeset/72178

[WebKit Review Bot]

http://trac.webkit.org/changeset/72173 might have broken SnowLeopard Intel Release (Tests)

[Andy Estes]

One final results update in http://trac.webkit.org/changeset/72179. Mac test bots are now green.

[Andy Estes]

Snow Leopard did indeed go green, but some tests have different results on the Leopard bot. Landed Leopard-specific results in http://trac.webkit.org/changeset/72256 and http://trac.webkit.org/changeset/72257.