Last Comment Bug 24186 - In NavQuirks mode, ignore line-height on block elements {ll} {compat} [TEXT]
: In NavQuirks mode, ignore line-height on block elements {ll} {compat} [TEXT]
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P1 major (vote)
: M14
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
: Chris Petersen
Mentors:
http://www.bath.ac.uk/%7Epy8ieh/inter...
: 20789 22328 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2000-01-17 17:01 PST by Hixie (not reading bugmail)
Modified: 2000-03-07 18:47 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
why doesn't this fix work? or does it? (1.44 KB, patch)
2000-01-24 12:41 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
"good enough for beta" proposed fix (8.36 KB, patch)
2000-01-30 20:46 PST, buster
no flags Details | Diff | Splinter Review

Description Hixie (not reading bugmail) 2000-01-17 17:01:20 PST
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". :-)
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-01-17 17:33:59 PST
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.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-01-17 17:59:59 PST
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().
Comment 3 buster 2000-01-24 11:15:14 PST
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.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-01-24 12:34:48 PST
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...
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-01-24 12:41:33 PST
Created attachment 4521 [details] [diff] [review]
why doesn't this fix work?  or does it?
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-01-24 16:16:22 PST
Judging from bug 24495 (or its absence), there may be anonymous inlines
somewhere that are preventing this from working...
Comment 7 buster 2000-01-24 23:02:06 PST
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 py8ieh=bugzilla@bath.ac.uk  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.
Comment 8 buster 2000-01-30 20:44:33 PST
attaching fix.  need code review.
Comment 9 buster 2000-01-30 20:46:45 PST
Created attachment 4730 [details] [diff] [review]
"good enough for beta" proposed fix
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-01-31 06:11:34 PST
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).
Comment 11 buster 2000-01-31 08:06:53 PST
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.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-01-31 08:17:53 PST
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.)
Comment 13 buster 2000-01-31 09:29:40 PST
marking potential beta1 bugs
Comment 14 Erik van der Poel 2000-01-31 10:49:42 PST
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.)
Comment 15 leger 2000-01-31 17:26:37 PST
Putting on PDT+ radar for beta1. 
Comment 16 buster 2000-02-01 11:12:37 PST
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.
Comment 17 Erik van der Poel 2000-02-01 11:18:29 PST
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.
Comment 18 buster 2000-02-01 23:40:16 PST
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. 
Comment 19 ekrock's old account (dead) 2000-02-02 12:09:41 PST
*** Bug 20789 has been marked as a duplicate of this bug. ***
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-02-08 20:37:15 PST
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).
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-02-08 20:37:35 PST
Assigning to self...
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-02-11 20:39:17 PST
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.
Comment 23 buster 2000-02-14 17:17:51 PST
david,you did a wonderful job on the code.  passes all my tests.  checking in 
soon, hopefully tonight.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-02-14 20:58:55 PST
Fix checked in 2000-02-14 20:26PDT.
Comment 25 troy 2000-02-15 10:40:05 PST
Great job David!
Comment 26 Chris Petersen 2000-02-18 12:36:38 PST
With the Feb 18th build, this problem has been fixed.
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2000-03-07 18:47:22 PST
*** Bug 22328 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.