82422 - line-height should be multiplied by em square height {ll} [INLINE]

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Shanjian Li]

This problem was originally reported in bug 27164, 

Comments From Ian 'Hixie' Hickson 2000-07-29 :

On http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight7.html one sees
that line-height: 1.0 is not using the font-size as the line-height.


It's also visible in the testcase of bug 77067:
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=32563


It's also visible in the testcase of bug 77067:
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=32563

Comment 1

[Shanjian Li]

From my observation, "lineheight <number>" does not seem to be interpreted. In stylestruct, 
it is specified as normal. If you change "<number>" in ian's test case, no difference will 
be made. 

Comment 2

[Shanjian Li]

add a bunch of people to cc list.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

We handle it fine on
http://www.w3.org/Style/CSS/Test/current/sec548.htm

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I was told on bug 27164 that this bug covers the problem on Ian's test.  Since
the summary of this bug is clearly false -- we do handle 'line-height:
<number>', retitling appropriately to reflect transfer from bug 27164 and
transferring other information.

Comment 5

[Hixie (not reading bugmail)]

As written by Erik in bug 27164:

Basically, the problem is that GetHeight returns the max height, and we are
multiplying this value by the CSS line-height property to compute the line
spacing. There have been a number of long discussions about this on the
[e-mail] mailing list, and it seems that many people believe that the
line spacing should be computed by multiplying line-height by the em square
height, which is different from the max height. IE and Opera also use the em
square height to compute the line spacing.

For line-height "normal", Mozilla currently uses the value 1.0 to multiply by
the return value of GetHeight. For the default font (Times New Roman) at the
default size (12pt == 16px @ 96ppi), GetHeight returns 19, and this is the same
as the line spacing used by WinNav4 and WinIE in CSS-less HTML.

However, this is not the line spacing recommended by Times New Roman itself. It
recommends 20 (when the size is 16). I'd like to propose that we try the
recommended line spacing, and see how that turns out. If we have too many
compatibility problems, we can revert to using the max height as the line
spacing, possibly only on Windows (where we seem to have the strictest
compatibility requirements).

Whether we go with the recommended line spacing or not, I'd like to propose a
number of new methods, so that the new set of height-related APIs becomes:

  GetMaxHeight, GetMaxAscent, GetMaxDescent
  GetEmHeight, GetEmAscent, GetEmDescent
  GetLeading

For line-height "normal", we should use GetEmHeight + GetLeading. Alternatively,
we could have a single API that returns that sum, called GetNormalLineHeight.
Initially, I'd like to add the new APIs. Then move all the callers to these new
methods, and finally remove the old API (GetHeight).

Bug History: see bug 4234, bug 13072, and now bug 27164.

The test case to look at is:
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight7.html

Some other related test pages:
   http://www.fas.harvard.edu/~dbaron/css/fonts/sizes/
   http://www.fas.harvard.edu/~dbaron/css/test/inlinetest
   http://www.fas.harvard.edu/~dbaron/css/test/linebox4
   http://www.fas.harvard.edu/~dbaron/css/test/emunit
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight1.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight2.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight3.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight4.html
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=32563 (bug 77067)

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I think what needs to happen to fix this is:

 1. We need to change line-height scaling factors to be multiples of the nominal
    font-size rather than the distance from max ascent to max descent.  (Do we
    do percentages correctly now, but not scaling factors?)

 2. We need to change the style system to treat 'normal' as a computed value for
    'line-height' and add code to treat normal as equivalent to a 'line-height'
    spanning max-ascent to max-descent.  (It would probably be better, although
    it might break some pages (so we might want to do it in a separate step,
    although it would probably help Japanese pages), to use the font's internal
    leading surrounding the max-ascent/max-descent box to define 'normal'.)

Other things that should happen are:

 3. We should figure out what the optimal box the height of the nominal
    'font-size' is.  I suspect it's probably not the max-ascent to max-descent
    box trimmed down half on each side.

Did I miss anything?

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Internal leading might not actually be the right term.  I'm getting the
vocabulary a bit confused.  But what I mean is, to take an XFontStruct as an
example, whether 'line-height: normal' should be equivalent to max_bounds.ascent
+ max_bounds.descent (which I think is what we do now) or ascent + descent.

Comment 8

[Hixie (not reading bugmail)]

Could you explain 3 again? I'm having difficulty parsing that into something I 
can understand... Otherwise, yes, I agree. (And I don't know the answer to your
question. Maybe rbs does?)

Comment 9

[Shanjian Li]

Ian & David,

Let me know if I am wrong. In ian's first testcase 
(ie. http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight7.html), copy it to your local
directory and modify line-height factor from 1.0 to 2.0. Try it on mozilla and it seems it does 
not work at all. Now remove the font size specification (ie. 20px;) and try it again, and line-height factor
begin to work. 

If you see the same as what I did, we might want to blame css parsing instead of font related code.
I need more time to trace the cause of this problem. But please do correct me if you believe I am in 
wrong track. 

thanks!

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

No, that's just a bug in Ian's test, not a bug in Mozilla.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

...to be more specific, the bug is because the selector |div.inner| is more
specific than |.one| or |.normal|, and because the |font| shorthand sets the
|line-height| property, so the |font| shorthand declaration overrides the
|line-height| declaration.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: corrected version of Ian's test

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Well, it turns out we actually pass Ian's test.  No wonder this issue has
confused me in the past.  (Shanjian- thanks for pointing out that the
'line-height' wasn't affecting anything.)

So I suspect the main remaining issue is polishing up our implementation of
'normal', although that may even be good.  I'm not sure...

Comment 14

[Hixie (not reading bugmail)]

Erm. Well, this is embarassing. So Erik _did_ fix it when he said he did, 
almost exactly a year ago I suck. My bad. Sorry. I've fixed the test now.

dbaron: Since we do everything CSS says we should, plus we don't break any
sites right now, I would recommend not changing anything. What do you think?

Shanjian: I'm marking the bug WORKSFORME, sorry about wasting your time!