36486 - Mozilla may use incorrect line spacing due to GTK confusion between font max_bounds.ascent vs font ascent (and ditto descent)

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Chris Siebenmann]

There exist X fonts where max_bounds.ascent and max_bounds.descent are
smaller than the (overall) ascent and descent in the XFontStruct. These
fonts are currently rendered with a narrower line spacing than what is
desireable (in my opinion), and certainly a narrower line spacing than
Netscape 4.x renders them.

 One such font is Chris Cannam's Bitmap Garamond BDF font, which I use
as my default browser font. You can find it at the URL
	http://www.all-day-breakfast.com/cannam/garamond.html
Mozilla renders lines in this font two pixels closer to each other than
Netscape 4.x does, and as a result not insignificantly reduces the readability
of the resulting pages.

 The X font -cannam-garamond-medium-r-normal--14-140-75-75-p-90-iso8859-1
has ascent 14, descent 5, max_bounds.ascent 13, and max_bounds.descent 4.
(As shown by 'xlsfonts -ll'.)

 The manpages (and Xlib documentation) for XQueryFont() strongly implies
that the ascent and descent values are what you should use for interline
spacing (while the max_bounds versions merely serve as the top and bottom
of the area outside of which the font will never draw pixels).

 At the moment nsFontMetricsGTK::RealizeFont primarily drives interline
spacing through mHeight, which is currently calculated off of the max_bounds
values instead of the straight values. This appears to be wrong.

 There is another problem in the woods: various pieces of code use
mMaxAscent (and presumably mMaxDescent), via GetMaxAscent et al. Quite
possibly these routines, or the callers of these routines, suffer from
the same confusion that the calculation of mHeight has.

 The real solution may be to split things out so that callers explicitly
ask for either the font's inter-line spacing (either ascent or descent as
appropriate in the face of changing fonts), which would use font ascent
and descent, or for bounding box information on the characters involved,
which would use max_bounds.ascent and max_bounds.descent as the quick
approach.

 At the advice of people on #mozilla on irc.mozilla.org, this bug report
is being cc:'d to a couple of Netscape people.

Comment 1

[Chris Siebenmann]

Attachment: Obvious patch to fix the mHeight calculation (compiles, works on my test case)

Comment 2

[troy]

Kevin, this is a gfx problem

Comment 3

[troy]

Pav, I don't know whether this is a the right component either. I usually assign 
these bugs to Kevin, but I changed the component and it went to you

Comment 4

[Erik van der Poel]

Yes, this is a GFX problem, and I normally file those under the Layout
component. Also, I'm the owner of X font height issues, so I'm reassigning it
to myself.

Comment 5

[Erik van der Poel]

The problem is that Adobe's "times" on Unix has a smaller (font ascent +
descent) than (max ascent + descent). Mozilla's default is adobe-times, so it's
quite important to us. Windows's Times New Roman gives a max height of 19 when
the 16-pixel font is requested, and Mozilla uses the max height for line-height
"normal". Times New Roman reports 20 for the recommended line spacing, but
Mozilla ignores that (and Nav4 and IE ignore it too). Adobe's times on Unix
reports a max height of 19 for the 17-pixel font, so I chose to use the max
height for the line spacing. This is indeed different from UnixNav4.

In short, X fonts, as usual, are a mess. It's infuriating. Maybe we need to come
up with some hack again. How about using the (font ascent + descent) if it's
larger than the (max ascent + descent)?

Comment 6

[Chris Siebenmann]

 Looking at http://slashdot.org/ I can see that just ascent + descent causes
problems (I wish I knew what NS 4.xx does, because its approach doesn't). It
looks better with mHeight being the maximum of ascent + descent and
max_bounds.ascent + max_bounds.descent.

Comment 7

[Chris Siebenmann]

 Further testing with mHeight being the maximum of the max_bounds ascent
and descent and the regular one has shown that this has problems too. Some
fonts (especially some TrueType fonts) seem to have max_bounds that are far
out on their regular ascent/descent figures, and cause the linespacing to
be uncomfortably wide. Times New Roman at relatively smallish sizes (as
used on Salon.com, for example) seems prone to this in my configuration.
(You'll need to patch the GTK code to allow you to use Times New Roman at
all to see this, which is another already-existing bug.)

 So a plain MAX() doesn't seem to be the solution. I'm experimenting with
allowing things to grow somewhat larger based on the difference between the
max_bounds and the normal values. Erik, do you have any figures on what the
typical and maximum differences are between the max_bounds and the regular
values that you've seen?

Comment 8

[Erik van der Poel]

I'm beginning to think that we should Just Say No. Instead of taking the max
ascent + max descent just because that makes adobe-times on X look like Times
New Roman on Windows, maybe we should just use font ascent + font descent, as
prescribed by the XLFD spec, and then if people complain, refer them to the X
people (e.g. XFree86) and have them fix the fonts themselves. It's probably
not a very good idea to put circumstantial hacks into Mozilla, since they might
bite us hard later.

Comment 9

[Chris Siebenmann]

 I agree with ignoring max_bounds.ascent/descent.

 After some more experimentation, I've found that the important thing
for making something like http://slashdot.org/ look right with using
plain ascent/descent seems to be to have mMaxAscent and (to a lesser
extent) mMaxDescent synchronized with the line height information in
mHeight, which argues that they should be set to be the same as mAscent
and mDescent.

 Another (potential) nit: mUnderlineOffset and mUnderlineSize duplicate
the calculation of interline height (and at current do it differently
from how mHeight itself is calculated, although if mHeight is changed
to be ascent+descent they'll become right). I think that the thing to
do with them is pull the height calculation out to a single point and
drive mHeight and everything else from this single value.

Comment 10

[Erik van der Poel]

I think we should fix this when we clean up the nsIFontMetrics API and its
callers. Currently, the layout engine treats line-height "normal" as 1.0, and
multiplies line-height by GetHeight, which is actually the bounding box, not
the em square. This needs to be fixed anyway, and so we might as well fix this
bug at that time too. See bug 27874.

Comment 11

[Erik van der Poel]

See bug 27164. If people disagree with my plan for that bug, then we should at
least correct the Unix version of Mozilla, to return font ascent + descent
instead of max ascent + descent (from GetHeight).

Comment 12

[Erik van der Poel]

Fixed bug 27164, so this is fixed now too.