196270 - Numeric line-height is not rendered identically to equivalent length or percent-based line-height

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Wevah (Nate Weaver)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030210

line-height rules with numeric values (e.g., line-height: 1.5;) are rendered
differently than line-heights with length-based (e.g., line-height: 1.5em;) or
percent-based (e.g. line-height: 150%;) line-heights; the numeric line-height
contains extra space between lines. This seems to be most noticeable under OS X,
but also occurs under Windows.

OS X: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030210
Windows: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.2.1) Gecko/20021130

Reproducible: Always

Steps to Reproduce:
1. Create a document with numeric line-height applied to an element.
2. View it.

Actual Results:  
The spacing is rendered incorrectly.

Expected Results:  
Mozilla should render numeric line heights the identically to their length-based
or percent-based equivalents.

Easy workaround for now: suffix your numeric line-heights with "em".

Comment 1

[Wevah (Nate Weaver)]

See also: http://www.w3.org/TR/CSS2/visudet.html#propdef-line-height

Comment 2

[Christopher Hoess (gone)]

Screenshot? In a current nightly on Windows 2000, I'm having trouble seeing the
difference.

Comment 3

[Chris Casciano]

This problem surfaced in a comparison vs. Safari here:
http://www.zeldman.com/daily/0203c.shtml#leadingedge

This may be related to bug 105743, but my brain isn't functioning well enoungh
to interprit the test case there.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm... I definitely see the different rendering for the third one (the percent
case).  Could that be due to using the font's mSize instead of size (compare
nsRuleNode::ComputeTextData to CalcLength for em).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Linux trunk screenshot (no xft, no bells).

And some screenshots are definitely in order, on all platforms... here is a
current Linux trunk build.

Comment 6

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

This is a duplicate of a bug Ian filed.

I also consider it a feature, but I guess I'm gradually being convinced that we
shouldn't do this.  (What do other implementations do?)

Comment 7

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

Attachment: testcase

This should be a clearer testcase.  (All font sizes are nominally 100px.)

Comment 8

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

Attachment: testcase

Er, use Ép instead of Ap.

Comment 9

[Boris Zbarsky [:bzbarsky]]

David, all versions of the testcase you posted render identically here (unlike
the original testcase for this bug....)

Comment 10

[Chris Casciano]

Attachment: alternate testcase

alternate testcase of original document w/ paragraphs floated to show more
obvious difference in line height... screenshots & other browser info to follow

Comment 11

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

I know.  But they didn't the last time I tested it, and I suspected they might
not on other platforms.  But that still doesn't explain the difference between
my testcase and the original one.

Comment 12

[Chris Casciano]

Attachment: alternate testcase - OS X Screenshot

This is a screenshot of the alternate testcase I just attached... safari 0.60
on the left with all 3 columns the same and Moz 2003030503 on the right with
the obvious longer column.

Additionally, IE 5.2.2 OS X shows all 3 columns at an identical length and
Opera 6/OS X shows the 1st and 3rd column the same with the middle (em based)
column roughly 10-15px longer.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Aha!  I see why the third chunk looks different for me -- I have a minimum font
size set that's bigger than 11px.  Our line-height computation uses font->mSize
for percent line heights.  For em heights we use font->mFont.size, and that's
also what we use for factor values (ComputeLineHeight in nsHTMLReflowState.cpp
does factor, CalcLength in nsRuleNode.cpp does em, nsRuleNode::ComputeTextData
does percent).

ccing rbs, but based on the comment in
http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsStyleStruct.h we
should be using mFont.size, methinks....

Should I spin off a separate bug on this issue?

Comment 14

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

Please don't worry about the code until we've decided what the correct behavior is.

I have a separate bug on expanding line-heights proportionally whenever minimum
font size applies.

Comment 15

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

See bug 27874, bug 27164, and bug 82422 for some of the history here.

Comment 16

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

Oh, I'm probably still getting things confused due to the history in bug 82422.

Comment 17

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

(Also, as an authoring note: em and percentage line heights have poor
inheritance behavior, and should almost never be used in practice.  Stick to
scaling factor line heights (such as '1.0').

Comment 18

[Wevah (Nate Weaver)]

Follow-up about Windows: I had someone send me a screenshot (using the Windows
build whose UA string I posted), and yes, the difference isn't very apparent. It
*is* there, but at 11px the top paragraph's line spacing was only 1px greater
than the other two.

Comment 19

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

Note that
http://www.w3.org/TR/CSS21/visudet.html#propdef-line-height uses "computed font
size" for percentage and
http://www.w3.org/TR/CSS21/syndata.html#value-def-length says that the 'em' is
"equal to the computed value of the 'font-size' property", while
http://www.w3.org/TR/CSS21/visudet.html#propdef-line-height does not say which
value of 'font-size' applies to <length> values.  So I think we could use the
actual value if we want, for <length> values only.  However, given the change in
the definition of font size (
http://www.w3.org/TR/CSS21/fonts.html#font-size-props says "The font size
corresponds to the em square, a concept used in typography.", unlike
http://www.w3.org/TR/REC-CSS2/fonts.html#font-size-props , which says "This
property describes the size of the font when set solid."), it's become harder to
claim that the actual value is different from the computed value due to overflow
from the em square.

Is it better to be consistent between the types or to try to avoid overlap for
line-heights greater than 1.0?  I guess the CSS 2.1 change in the definition of
'font-size' is saying that we should be consistent between the types.  This
means we should remove the |fm->GetEmHeight| in |ComputeLineHeight| (in
nsHTMLReflowState.cpp).  Whatever we do, we should definitely get rid of
|nsHTMLReflowState::UseComputedHeight|.

Comment 20

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

The fix for this bug should to remove nsHTMLReflowState::UseComputedHeight and
modify all callers to act as they would if it returned true, which is not what
it does now.  As ugly as it may seem (since we really shouldn't have things that
change our standards support based on environment variables), you can test this
behavior by setting the "GECKO_USE_COMPUTED_HEIGHT" environment variable.

Comment 21

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

Actually, we may not want to act as though it returns true for all cases -- we
should still use the font box for things relating to the border and background.
 Setting it in the environment seems to lead to focus outlines being drawn
incorrectly.  (It also makes the testcase in attachment 116511 [details] draw with all 3
blocks the same height.)

Comment 22

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

Attachment: patch

Comment 23

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

Taking.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So.. I'm still trying to sort out the old bugs on this.  Is there a clear
explanation somewhere of what's going on and how this patch affects it?

Comment 25

[rbs]

+      float factor = text->mLineHeight.GetFactorValue();
+      lineHeight = NSToCoordRound(factor * font->mFont.size);

So the font's minimum-size is factored in. Is that deliberate?

Comment 26

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

The goal of this patch is to make:
 * all 'line-height' values depend on the computed 'font-size'
 * the border/background of inline boxes depend on the font's bounding box
The problem was that, before the patch, <number> 'line-height' values were based
on information from the font metrics (although seemingly not the fonts bounding
box -- I don't quite understand why not, unless it's the difference between
GetHeight and GetMaxHeight).

Bug 82422 was simply a mistake in a testcase.

(Perhaps we really should be basing things on the actual font metrics, though,
since GetHeight does seem to be doing the right thing in *most* cases.)

Comment 27

[rbs]

> * all 'line-height' values depend on the computed 'font-size'

That would be font->mSize then, since font->mFont.size is the actual size which
may have been clamped to the minimum size. But this line-height is all
confusing.

Comment 28

[rbs]

Something else which just occurred to me is the interaction of font-size-adjust.
How is that defined? In this case the (final/true) actual size is the emHeight
which integrates any correction that sizeAdjust brought in. 

Comment 29

[Hixie (not reading bugmail)]

Isn't this a dupe of bug 105743?

Comment 30

[Hixie (not reading bugmail)]

     // Negative line-heights are not allowed by the spec. Translate
     // them into "normal" when found.

Shouldn't negative line-height values be caught at the parser level?

Comment 31

[Hixie (not reading bugmail)]

rbs: As I understand it, the model is as follows:

        stylesheet cascade
                |
               \|/
          specified value <-----------------------.
                |                                 |
               \|/                                |
          computed value ------------------> inheritance
                |
               \|/
            font zoom
                |
                +-----------------------+---------.
                |                       |         |
               \|/                      |    calculation
   minimum font-size calculation        |   of 'em', 'ex',
                |                       |  '%', and ratios
               \|/                      |   used in other
         font-size-adjust               |     properties
                |                       |         |
               \|/                  height of     |
           actual value             font box      |
                |                       |         |
               \|/                     \|/       \|/
         font rasterising            inline box model

So in theory, font-size-adjust shouldn't ever be able to affect line-height.
(Note: This doesn't explain how absolute line-heights zoom with font zoom. As I
understand it, that would be a line coming out of the "computed value" bit and
going down to the "inline box model" bit, going through its own "font zoom" step
and unaffected by the "calculation..." step above.)

Comment 32

[Boris Zbarsky [:bzbarsky]]

> Shouldn't negative line-height values be caught at the parser level?

Yes, it should be.  And it is for "line-height".  It's not as part of the "font"
shorthand, but that's trivial to fix.  And then we can remove this code....

As for the diagram, I'd expect the box model stuff to happen _after_ min font
size got applied....  With your proposal it looks like min font size will lead
to lots of overlapping.

Comment 33

[rbs]

re: comment #31

Let me say that the current implementation is in serious disagreement with that,
then.

The only thing that is "cascading" is font->mSize. As soon as font->mFont.size
is used, it means that it may have the clamp introduced by the minimum size. And
as soon as GetEm|X|MaxHeight(), etc, are used, it means that they may have any
correction/adjustment introduced by sizeAdjust when realizing the physical font
from where these metrics are fetched. (i.e., this is how it is implemented.)

Comment 34

[rbs]

BTW, saw the the other day that the font's min-size (and max-size) have become
part of CSS3. So it is not just a side extra anymore. (I often wonder if CSS
isn't going to become the SGML of style. Massive, complex, and unimplementable
fully...)

Comment 35

[rbs]

It seems to me the model (if that is 'the' model) isn't feasible in that form.
The font box cannot been know without a tangible/physical font. And imagine if
someone does aRenderingContext::GetFontMetrics([in]nsFont, [out]fm), and then
fm->GetGetEm|X|MaxHeight() just to get random (err logical) values. The layout
will break havoc. All the alignments in MathML for example would become a total
mess if the metrics aren't the true physical metrics of the font under
consideration.

Comment 36

[rbs]

dbaron's patch looks reasonnable to me. Essentially the patch is doing:

     if (unit == eStyleUnit_Factor) {
       // For factor units the computed value of the line-height property 
+      // is found by multiplying the factor by the font's computed size.
+      float factor = text->mLineHeight.GetFactorValue();
+      lineHeight = NSToCoordRound(factor * font->mFont.size);
>> this part implicitly incorporates the font's minimum size as I noted.
     } else {
       NS_ASSERTION(eStyleUnit_Normal == unit, "bad unit");
+      lineHeight = GetNormalLineHeight(fm);
>> this part boils down to fm->GetNormalLineHeight(), and as I also noted,
>> fm->Something() comes from a physical font (existence), and incoporates the
>> font's minimum size, size-adjust, and possibly other constraints that CSS3
>> may bring in later.
     }

My only question is what I hinted in comment #25, re: the _awareness_ that
font->mFont.size incorportes the minimum size constraint. If so, the comment
isn't accurate:

       // For factor units the computed value of the line-height property 
+      // is found by multiplying the factor by the font's computed size.

Comment 37

[Boris Zbarsky [:bzbarsky]]

David, if I don't get to this review by Friday, I won't be able to do it for
1.4a.....

Comment 38

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

Comment on attachment 116517 [details] [diff] [review]
patch

I'm not sure this is the right approach.

Comment 39

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

*** Bug 212854 has been marked as a duplicate of this bug. ***

Comment 40

[Felix Miata]

Attachment: more compact testcase

Note that the difference between the two divs depends at the very least on
default size and zoom applied. When I look at this in the current Linux trunk,
the two divs match size at 16px default, but not at 13, 18 or 22. Regardless
your default, if you play with zoom enough, you should be able to see at least
one case each of match and mismatch.

Comment 41

[Felix Miata]

Attachment: Linux trunk screenshot of attachment 171226 using 22px default and no zoom

Comment 42

[Felix Miata]

Attachment: OS/2 FF 1.0 screenshot of attachment 171226 using 20px default and no zoom

Comment 43

[mmc]

Attachment: Screenshot at the smallest size

I tried to zoom out on the testcase, and as the page gets smaller and smaller,
the red box shrinks much more than the green one, while usually they have
roughly similar sizes.

Comment 44

[Felix Miata]

Attachment: a problem we perpetuate by continuing to defer this

Comment 45

[Felix Miata]

For clarification, authors are widely refusing to user <number> for line-height
due to this bug.

Comment 46

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow comment 44 and line 45...

Comment 47

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

Comment on attachment 189827 [details]
a problem we perpetuate by continuing to defer this

Ignore comment 44 and comment 45; they're describing a different bug from this
one, and one that is invalid.

Comment 48

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

Well, comment 44; comment 45 may be valid, but comment 44 is completely
extraneous and just wastes the time of people reading this bug.  The "testcase"
is also extremely verbose.

Comment 49

[Felix Miata]

The comment 44 attachment shows why <number> is the only value for line-height
that authors should be able to use with confidence in any event, because it is
the only one that is inherited at the specified value instead of a computed value.

This widely known bug causes authors to instead use values that inherit as
computed values, and thus to commonly trigger overlapping text for our users of
minimum font size and zoom.

Comment 50

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

I know that, but it didn't say so, and it said *lots* else.  Two sentences, with
no testcase, would have been sufficient.  Please do *not* reply to this comment
on this bug.

Comment 51

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

Attachment: patch to remove GECKO_USE_COMPUTED_HEIGHT (checked in 2005-08-02)

OK, to keep this simpler, let's start with a patch that doesn't change our
behavior at all except for removing support for checking
GECKO_USE_COMPUTED_HEIGHT.

Comment 52

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

Comment on attachment 191268 [details] [diff] [review]
patch to remove GECKO_USE_COMPUTED_HEIGHT (checked in 2005-08-02)

Remove unneeded code.

Comment 53

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

Attachment: patch to fix bug

This is now much easier to understand.

This might be bad for bitmap fonts, though.

Comment 54

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

Attachment: patch to fix bug

Comment 55

[Boris Zbarsky [:bzbarsky]]

I won't be able to look till I get back...

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 191416 [details] [diff] [review]
patch to fix bug

Looks good.  Sorry that took so long... :(

Comment 57

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

Fix checked in to trunk, 2005-08-29 12:30 -0700.

Comment 58

[Asa Dotzler [:asa]]

dbaron, what's the value of getting this into 1.5 and what's the risk? 

Comment 59

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

It's a relatively minor change:  it should only even change things at all on
some platforms, and, where it does, it will apparently improve compatibility
with other browsers.  Apparently the difference that no longer exists with this
patch was discouraging authors from using 'line-height' the right way and
encouraging them to use it the wrong way, so I'd like to get it in.

Comment 60

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

Fix checked in to MOZILLA_1_8_BRANCH, 2005-08-31 14:21 -0700.