THIS IS A COMPATABILITY MODE QUIRK ONLY!!! In NavQuirks mode, we should ignore the rules about minimum line-height on block level elements. Many legacy documents currently do things like: <p><font size="1">...</font></p> ...and the net result is effectively double-spaced text. This can apparently be seen on the following high profile sites: http://www.cnn.com/ http://www.latimes.com/ http://www.abcnews.com/ http://www.nytimes.com/ http://www.amd.com/ http://www.oracle.com/ http://www.zdnet.com/ (look at Community links toward bottom) http://www.wired.com/ The solution is to avoid generating the virtual empty anonymous generated inline box that is used to give each line box a minimum line height. Kipp wrote that code. What we do is act as if there was an empty inline element at the start of every line box, which has a line-height the same as the block's. This causes the line-height of the box to be the minimum height of each line box in the block, as required by CSS2. In Quirks mode only, we need to avoid generating that anonymous inline. Troy: Could you have a quick look at this? If it is easy to wrap the code which takes this virtual anonymous inline into account with a quirks mode check, then it would be cool if it could be done soon as people are clamouring for something to be done about this and it would probably earn you "friend of the tree". :-)
I think this is actually quite easy to do, although that's possibly because currently the effect of line-height on blocks is implemented *as described in the spec* (which is probably bad). At least, that's true if a) the code is the same as the last time I looked at it, which was when bug 5821 was fixed. b) my memory is correct.
The fix I thought would work did not. I can't spend any more time looking at this before my exams are over (Saturday). If anyone else wants to look, the relevant code should be in nsLineLayout::VerticalAlignFrames().
I'll take a look at this. A HUGE thanks to py8ieh, david baron, and all those who contributed to the thread on n.p.m.layout that led us to this point. Changing severity to "major" because it's such an important backwards compatibility issue. Priority P2.
I'll attach the patch that I tried. I really have no idea why it doesn't work. Probably a little time in the debugger would help figure that out...
Judging from bug 24495 (or its absence), there may be anonymous inlines somewhere that are preventing this from working...
transfering comment from wrong bug # I think I have a fix for this, but I am unable to load the ahem font to test on the given URL. I downloaded it using "Save As" with the link on the test page to my WinNT box and WinNT thinks it's corrupt and refused to recognize it as a font file. Could you put it out somewhere for standard ftp binary download? Thanks. To which I got this response: ------- Additional Comments From email@example.com 2000-01-24 21:54 ------- buster: What does your fix do? As far as David and I can tell, this bug is not a bug! It works fine. Are you removing the redundant code David mentioned in his 2000-01-24 16:47 comment? Anyway, I've put another copy of the Ahem font's TTF file at: http://www.bath.ac.uk/%7Epy8ieh/internet/projects/mozilla/AHEM____.TTF If that still causes problems, I'll mail it to you (<30K). Sorry, I don't have access to an FTP site. David: There is now a multiple line version of the test at the URI. It seems to work fine in Mozilla. ============================================================================== sorry for the confusion.
attaching fix. need code review.
Somehow I feel that the fix doesn't need to be so complicated. In quirks mode, I think you can *always* ignore the min line-height, with no problems, because the only time the min line-height can do anything is if a large (multi-line) section of the paragraph is in a smaller font. (And, I expect that for quirks mode, doing this only when the font is the only child of the paragraph will cause problems with: <p>Some text: <br><font size="1">Text</font> <br>... so that you're better of ignoring it completely).
I tried that approach, and it didn't work. Normal lines of text result in a line height that is too small, and consecutive lines run into each other. My fix is far from perfect, I'm the first to admit. But it fixes the layout of a LOT of pages, and does no harm to pages that don't contain the pattern. It's trivial to construct cases where it still doesn't work.
What happens if you try that approach, but use the min line-height when there are text-frames that are direct children of the block in the line? (I still don't quite understand how Kipp's code does what it does, so I'm not sure whether that's exactly what would be needed.) Is the problem you see in blocks with no inline elements? (That's what I'm assuming.)
marking potential beta1 bugs
I'm concerned about the proposed fix, since it uses nsIFontMetrics::GetHeight, which currently only returns the height of the font loaded at init time, which is a font containing the English glyphs. This does not take into account the machines where e.g. Japanese fonts are larger than English ones. We could change GetHeight to look at the Japanese font too, but then you've started going down the slippery slope. Someone will want to add the Chinese font, and so on. We don't want to load all of these fonts if they're not going to be used: http://bugzilla.mozilla.org/show_bug.cgi?id=20394 So I suggested a change to nsIRenderingContext, which has the GetWidth method. I'm proposing a new method called GetDimensions, which will return not only the width of the string, but also the height and ascent of the inline box generated by that string. This would mean that the VerticalAlignFrames method needs to align the frames, without calling nsIFontMetrics::GetHeight. To deal with old HTML documents with <font size="1">, we simply need to refrain from doing the minimum line-height thing (empty anonymous box). (Though it may not be "simple" in the code.)
Putting on PDT+ radar for beta1.
Erik: I understand your concern, but it seems like a somewhat separate issue. I agree we need to be more clever about what number we use when we realize we need to get a height from the font, but I think that's independent of the main point of this bug and my fix. So unless I've misinterpreted your concern, I'll check in what I have and we'll use bug 20394 as the forum for fixing the font height issue. dbaron, your last suggestion isn't bad, but I couldn't get my first attempt at it to work. The code simply has too much going on inside it for me to understand (in a short time) why it didn't work. I think we'll end up revisiting this post-beta entry.
I'm OK with your fix, but please realize that I may need to make changes there in the future. I want it to stop calling nsIFontMetrics::GetHeight, and to start using the height of the inline box, which should by then already have been computed by nsTextFrame::Reflow, which will call GetDimensions.
marking this one REMIND. It's fixed enough for beta, but there are still cases where it won't do the same thing as Nav 4.x. We may need to revisit this after beta entry.
*** Bug 20789 has been marked as a duplicate of this bug. ***
I'm reopening this bug (from REMIND) and assigning it to myself, since I have what I think should be a better fix for it (pretty much what I described above).
Assigning to self...
Accepting bug and clearing [PDT+] marker, since that was for before buster's fix. I do have an improved fix for this, but I don't think it's needed for beta1 (especially if beta1 is branched at M14). See also bug 26996 and bug 26998.
david,you did a wonderful job on the code. passes all my tests. checking in soon, hopefully tonight.
Fix checked in 2000-02-14 20:26PDT.
Great job David!
With the Feb 18th build, this problem has been fixed.
*** Bug 22328 has been marked as a duplicate of this bug. ***