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

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
Layout: Text
P2
normal
RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: Wevah, Assigned: dbaron)

Tracking

({fixed1.8})

Trunk
mozilla1.8beta4
fixed1.8
Points:
---
Bug Flags:
blocking1.8b5 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch][no l10n impact], URL)

Attachments

(11 attachments, 3 obsolete attachments)

39.95 KB, image/png
Details
1.32 KB, text/html
Details
2.54 KB, text/html
Details
93.84 KB, image/gif
Details
11.18 KB, patch
Details | Diff | Splinter Review
1.83 KB, text/html
Details
19.20 KB, image/png
Details
33.09 KB, image/png
Details
20.46 KB, image/png
Details
8.10 KB, patch
roc
: review+
bsmedberg
: approval1.8b4+
Details | Diff | Splinter Review
3.35 KB, patch
Mike Schroepfer
: approval1.8b4+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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".
(Reporter)

Comment 1

15 years ago
See also: http://www.w3.org/TR/CSS2/visudet.html#propdef-line-height
Screenshot? In a current nightly on Windows 2000, I'm having trouble seeing the
difference.

Comment 3

15 years ago
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.
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).
Created attachment 116508 [details]
Linux trunk screenshot (no xft, no bells).

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

Comment 6

15 years ago
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?)
(Assignee)

Comment 7

15 years ago
Created attachment 116509 [details]
testcase

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

Comment 8

15 years ago
Created attachment 116510 [details]
testcase

Er, use Ép instead of Ap.
Attachment #116509 - Attachment is obsolete: true
David, all versions of the testcase you posted render identically here (unlike
the original testcase for this bug....)

Comment 10

15 years ago
Created attachment 116511 [details]
alternate testcase

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

Comment 11

15 years ago
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

15 years ago
Created attachment 116512 [details]
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.
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?
(Assignee)

Comment 14

15 years ago
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.
(Assignee)

Comment 15

15 years ago
See bug 27874, bug 27164, and bug 82422 for some of the history here.
(Assignee)

Comment 16

15 years ago
Oh, I'm probably still getting things confused due to the history in bug 82422.
(Assignee)

Comment 17

15 years ago
(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').
(Reporter)

Comment 18

15 years ago
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.
(Assignee)

Comment 19

15 years ago
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|.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 20

15 years ago
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.
(Assignee)

Comment 21

15 years ago
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.)
(Assignee)

Comment 22

15 years ago
Created attachment 116517 [details] [diff] [review]
patch
(Assignee)

Comment 23

15 years ago
Taking.
Assignee: font → dbaron
Severity: minor → normal
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.4alpha
(Assignee)

Updated

15 years ago
Attachment #116517 - Flags: superreview?(bzbarsky)
Attachment #116517 - Flags: review?(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

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

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

Comment 26

15 years ago
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

15 years ago
> * 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

15 years ago
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. 
Isn't this a dupe of bug 105743?

     // 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?
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.)
> 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

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
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.
David, if I don't get to this review by Friday, I won't be able to do it for
1.4a.....
(Assignee)

Comment 38

15 years ago
Comment on attachment 116517 [details] [diff] [review]
patch

I'm not sure this is the right approach.
Attachment #116517 - Flags: superreview?(bzbarsky)
Attachment #116517 - Flags: review?(bzbarsky)
(Assignee)

Comment 39

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

Comment 40

13 years ago
Created attachment 171226 [details]
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

13 years ago
Created attachment 171227 [details]
Linux trunk screenshot of attachment 171226 [details] using 22px default and no zoom

Comment 42

13 years ago
Created attachment 171228 [details]
OS/2 FF 1.0 screenshot of attachment 171226 [details] using 20px default and no zoom

Updated

12 years ago
Flags: blocking1.8b4?
Whiteboard: [patch] → [patch][no l10n impact]

Comment 43

12 years ago
Created attachment 189177 [details]
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.

Updated

12 years ago
Flags: blocking1.8b4? → blocking1.8b4-

Comment 44

12 years ago
Created attachment 189827 [details]
a problem we perpetuate by continuing to defer this

Updated

12 years ago
Attachment #189827 - Attachment mime type: text/plain → text/html

Comment 45

12 years ago
For clarification, authors are widely refusing to user <number> for line-height
due to this bug.
I'm not sure I follow comment 44 and line 45...
(Assignee)

Comment 47

12 years ago
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.
Attachment #189827 - Attachment is obsolete: true
(Assignee)

Comment 48

12 years ago
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

12 years ago
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.
(Assignee)

Comment 50

12 years ago
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.
(Assignee)

Comment 51

12 years ago
Created attachment 191268 [details] [diff] [review]
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.
Attachment #191268 - Flags: superreview?(roc)
Attachment #191268 - Flags: review?(roc)
Attachment #191268 - Flags: superreview?(roc)
Attachment #191268 - Flags: superreview+
Attachment #191268 - Flags: review?(roc)
Attachment #191268 - Flags: review+
(Assignee)

Comment 52

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

Remove unneeded code.
Attachment #191268 - Flags: approval1.8b4?
Attachment #191268 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Updated

12 years ago
Attachment #191268 - Attachment description: patch to remove GECKO_USE_COMPUTED_HEIGHT → patch to remove GECKO_USE_COMPUTED_HEIGHT (checked in 2005-08-02)
(Assignee)

Comment 53

12 years ago
Created attachment 191409 [details] [diff] [review]
patch to fix bug

This is now much easier to understand.

This might be bad for bitmap fonts, though.
(Assignee)

Comment 54

12 years ago
Created attachment 191416 [details] [diff] [review]
patch to fix bug
Attachment #191409 - Attachment is obsolete: true
Attachment #191416 - Flags: superreview?(bzbarsky)
Attachment #191416 - Flags: review?(bzbarsky)
I won't be able to look till I get back...
Comment on attachment 191416 [details] [diff] [review]
patch to fix bug

Looks good.  Sorry that took so long... :(
Attachment #191416 - Flags: superreview?(bzbarsky)
Attachment #191416 - Flags: superreview+
Attachment #191416 - Flags: review?(bzbarsky)
Attachment #191416 - Flags: review+
(Assignee)

Comment 57

12 years ago
Fix checked in to trunk, 2005-08-29 12:30 -0700.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.4alpha → mozilla1.8beta4
(Assignee)

Updated

12 years ago
Attachment #191416 - Flags: approval1.8b4?

Comment 58

12 years ago
dbaron, what's the value of getting this into 1.5 and what's the risk? 
(Assignee)

Comment 59

12 years ago
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.

Updated

12 years ago
Attachment #191416 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
(Assignee)

Comment 60

12 years ago
Fix checked in to MOZILLA_1_8_BRANCH, 2005-08-31 14:21 -0700.
You need to log in before you can comment on or make changes to this bug.