Closed Bug 24186 Opened 21 years ago Closed 20 years ago

In NavQuirks mode, ignore line-height on block elements {ll} {compat} [TEXT]


(Core :: Layout, defect, P1, major)






(Reporter: ian, Assigned: dbaron)





(2 files)


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: (look at Community links toward bottom)

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.
Assignee: kipp → buster
Severity: minor → major
Priority: P3 → P2
Target Milestone: M14
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?  

To which I got this response:
------- Additional Comments From  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:
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.
Whiteboard: potential fix in hand...testing...
attaching fix.  need code review.
Priority: P2 → P1
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>
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
marking potential beta1 bugs
Keywords: beta1
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:

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. 
Whiteboard: potential fix in hand...testing... → [PDT+]potential fix in hand...testing...
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 

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. 
Closed: 20 years ago
Resolution: --- → REMIND
Whiteboard: [PDT+]potential fix in hand...testing... → [PDT+]
*** 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).
Resolution: REMIND → ---
Assigning to self...
Assignee: buster → dbaron
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.
Whiteboard: [PDT+]
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.
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Great job David!
With the Feb 18th build, this problem has been fixed.
*** Bug 22328 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.