The uri quoted covers it all, really. Basically, line-height:1.3 with varying font-sizes causes overlap, when in fact the line-height should merely be the largest font-size*1.3 on the line. Also, line-height:130% should force the line-height to 130% of the element's font-size, and so *whatever* is on the line, the line-height should *not* be affected. As I said, see the page for more details.

I think the best way to think about this is as line-boxes. You seem to be doing Example 1 (above) based on whatever starts the line, and putting all the line-height below the line (see bug 585).

I *think* (if I understand the code) I may have found the problem. In /layout/html/base/src/nsInlineReflow.cpp , you say 847 if (newLineHeight >= 0) { 848 // Apply half of the changed line-height to the top and bottom 849 // positioning of each frame. 850 topEdge += (newLineHeight - lineHeight) / 2; 851 lineHeight = newLineHeight; Line 847, I think should read if (newLineHeight >= lineHeight) { You were setting the line-height to the line-height of the block level element, I think. I'm not sure if you also need to make sure to go back to the last block level element and use its line-height as a minimum even if the parent is not block level, or if that is already covered. (I don't understand the code all that well.)

Actually, I take that back. The spec says: If the property is set on a block-level element whose content is composed of inline-level elements, it specifies the minimal height of each generated inline box. (10.8.1) The keyword here is inline box, not line box. This means this needs to be dealt with much higher up in the function. When the height of the box is increased in this way, half the leading goes on each side (as always). There are other problems with that code... see bugs 585 and 1508. I was actually reading it to try to understand 1508.

I was looking at the old versions of the file, and the changes you made to it (nsInlineReflow) on December 14 made things worse. I went back to my copy of the December 12 build, and example 1 above was fine. The changes you made then seem to me to be a layout workaround for what is a CSS inheritance problem. After reading the layout code, I am almost sure that is where the problem is. The CSS inheritance problem is that when line-height is specified as a percentage, the *calculated* value should inherit. There is also some messy stuff from bug 1508 involved here, so I'm not so sure anymore, but I still think this is (one of) the problems.

Never mind that. I just proved to myself that its not an inheritance problem. Inheritance is fine. However, I think this and bug 1508 are basically duplicates, because the problem is the following. First, there is the problem with line-height reduction above (the one where I quoted code). This may, however, be covering up other problems (i.e., calculating the height incorrectly on inline elements). Second, you are aligning the line-box incorrectly. You are placing inline boxes based on the top of the border (you have an XXX where you don't account for margin), when instead you should be using the top of the line box itself, where the top of the line box is the top of the highest inline box, and the top of each inline box is the top of the text plus half the leading (with the leading possibly modified due to the nearest block-level ancestor's line- height and being the largest of the calculated line-heights of the inline boxes surrounding the given inline box (I'm not sure about this, actually -- the spec isn't clear)). I could explain this more coherently sometime farther from midnight. Finally, Ian's assertion on the large text "(should not affect line height)" is actually wrong because the inline box containing the larger text, although it has the same height as the other, has a baseline in a different position relative to that height, and since it is baseline aligned, it increases the height of the line box.

There are actually subtle distinctions above that I didn't make about the line- height of the element versus its parent (with nested inline elements). (That was what I said I didn't understand.) The line height on an inline box is the larger of the calculated value of the line height on that inline box and its closest ancestor block level element. However, since inline elements are considered to be broken at line breaks, if something like: <tall><short> Lots of text here, that takes up many lines.</short></tall> occurs, then the line height of tall affects every line in the middle, and short is aligned vertically within tall according to its vertical-align in each line (you handle that fine now (the spacing, anyway), actually, as long as the block around it has line height of normal, which you seem to handle as an exception (is that what the 0 is?)). Sorry to have said this in so many comments. Let me know which parts of what I said that you don't understand, so I can explain again.

See http://lists.w3.org/Archives/Public/www-style/1999Jan/0027.html for an explanation of a problem with the spec's description of line-height. I think the bit about inline-box versus line-box should actually be settled the way you had it, except with proper vertical alignment. To the two problems with your line-height calculation that I list above, I also should add the following: Thirdly, vertical alignment *within* the line-box is being messed up by top padding and border. The way you are now doing things, you need to move them up by the value of their padding and border.

In regard to David's comments above, dated 01/09/99 20:38: > Finally, Ian's assertion on the large text "(should not affect line > height)" is actually wrong because the inline box containing the > larger text, although it has the same height as the other, has a > baseline in a different position relative to that height, and since > it is baseline aligned, it increases the height of the line box. Err, oops. Good point. I have now fixed the test page... http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/lineheightdemo1.html ...to take this into account. I do not have NGLayout here to test whether it passes the updated test, however.

Just checked -- the updated test is still failed by NGLayout.

Some other good examples for line-height: The first letter in: http://www.fas.harvard.edu/~dbaron/sat/na/ The tests: http://www.fas.harvard.edu/~dbaron/csstest/linebox1.html http://www.fas.harvard.edu/~dbaron/csstest/linebox2.html http://www.fas.harvard.edu/~dbaron/csstest/linebox3.html

Page modestly improved and moved. Uri updated. Hope this bug gets fixed soon.

I have written a test that covers line-height and vertical align, and padding and borders. It is available at: http://www.fas.harvard.edu/~dbaron/csstest/inlinetest.html I think you will find it useful when working on these bugs (i.e., 585, 1040, 1278, 1508, 1934, 1990, and 2014).

Setting all current Open/Normal to M4.

Most of the layout issues for this bug have been resolved; however, until the inlinetest that david baron has provided is passed I'd like to leave the bug open. Our handling of line-height and vertical alignment in the face of border/padding/margins to believe correct. Because font-size's are approximations, however, it is not easy to come up with a test that has predictable results that can match his reference rendering...

Font size matching on Windows is exact, and you're still a little off with some of the green lines. However, you have bigger problems on the nesting of inline elements and the use of text-top and text-bottom aligns. I made an image that has both NGLayout 99-02-24 (Windows) and the reference rendering. The opacity is such that you are seeing 60% NGLayout and 40% reference rendering. See: http://www.fas.harvard.edu/~dbaron/tests/nglayout/ngview.gif

Fixed. Note that the layout of the URL test is now perfect; the test provided by david baron is *correct* but depending on how fonts resolve on your system you may see some deviations (which are correct). With a debug build you can force gecko to match his test by setting the GECKO_USE_COMPUTED_HEIGHT environment variable (on unix or windoze). Every test mentioned in this bug report now passes.

Verified this bug, since remaining issues are covered in bugs 4233 and 4234.