536758 - line-height reported by window.getComputedStyle is incorrect if font-size is 0px

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Dmitry Stepanov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6

If you set font-size on an element to 0px, you get "-1.78957e+7px" value of line-height property from window.getComputedStyle(). It does not matter what value of line-height is currently set on element. If you have font-size of 0px, you always get the same as value of line-height - "-1.78957e+7px".

Reproducible: Always

Steps to Reproduce:
1. Open URL i provided
2. Dialog box will pop up with line-height value.
3. Notice incorrect report of line-height. See page's source code.
Actual Results:  
-1.78957e+7px

Expected Results:  
20px

Comment 1

[Tim (fmdeveloper)]

Seeing same results as reporter on XP with 3.5.6 and 12/30/09 Minefield. Changing font size from 0 to 1 shows correct result.

Result on Ubuntu 9.1 was -3.57914e+7px on 3.5.6.

-Changed from Win 7 to all-
-Changed title from "-1.78957e+7px" to "incorrect" as calc is different on Ubuntu-
-Added testcase keyword-

Comment 2

[Boris Zbarsky [:bzbarsky]]

This has nothing to do with the HTML parser.

The line-height computation involves dividing by the font size, which is where you get the weird very large value.  See nsComputedDOMStyle::GetLineHeightCoord.

It's not quite clear to me what the "right" value to return here is (for example what the used line-height is in this case).

I suppose we could skip the division if font->mSize == font->mFont.size.  Would that help?

Comment 3

[Atul Aggarwal]

Attachment: Testcase

Adding testcase as provided link does not work anymore. 
Click on the "Show line-height" button to preview the problem.

Comment 4

[Atul Aggarwal]

Attachment: Patch v1

It's better to check for zero than checking for equality as then we need to check for zoom level also. Moreover, i believe we should always avoid division by zero.

Comment 5

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

We still want to adjust for text zoom when the font size is 0, though.  (I believe when one of the font sizes is zero, they'll both be zero.)

Comment 6

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

Comment on attachment 560875 [details] [diff] [review]
Patch v1

>Bug 536758 - line-height reported by window.getComputedStyle is incorrect if font-size is 0px

The commit message shouldn't be the name of the bug; it should be a description of what your patch is changing.  For example:
Change window.getComputedStyle().lineHeight to skip font size ratio adjustment for minimum font size when font sizes are 0.

>-  aCoord = NSToCoordRound((float(aCoord) *
>-                           (float(font->mSize) / float(font->mFont.size))) /
>-                          mPresShell->GetPresContext()->TextZoom());
>+  if (font->mFont.size != 0) {
>+    aCoord = NSToCoordRound((float(aCoord) *
>+                             (float(font->mSize) / float(font->mFont.size))) /
>+                             mPresShell->GetPresContext()->TextZoom());
>+  }

As I said in the previous comment, you probably want to leave the division by TextZoom when the font size is 0 (i.e., move it up or down outside the if), and thus probably want a temporary float rather than repeated manipulation of aCoord (followed by rounding and assignment to aCoord at the end).

Comment 7

[Atul Aggarwal]

Attachment: Patch v2

Comment 8

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

Comment on attachment 570976 [details] [diff] [review]
Patch v2

># HG changeset patch
># Parent 6ec5b28142d10d83046bdce764fbc1fe3598d4be
># User Atul Aggarwal <[e-mail]>
># Date 1320131210 -19800
>Bug 536758 - window.getComputedStyle().lineHeight to skip font size ratio adjustment for minimum font size when font sizes are 0.

You dropped the word "Change " before "window.getComputedStyle().lineHeight".

>   aCoord = nsHTMLReflowState::CalcLineHeight(mStyleContextHolder,
>                                              blockHeight);

You shouldn't start messing with aCoord up here, then move the result into fCoord and back into aCoord.  Also, don't call it |fCoord|; the f prefix doesn't match any of our conventions.  So just start here with:

float result = float(nsHTMLReflowState::CalcLineHeight(...));

>   // CalcLineHeight uses font->mFont.size, but we want to use
>   // font->mSize as the font size.  Adjust for that.  Also adjust for
>   // the text zoom, if any.
>   const nsStyleFont* font = GetStyleFont();
>-  aCoord = NSToCoordRound((float(aCoord) *
>-                           (float(font->mSize) / float(font->mFont.size))) /
>-                          mPresShell->GetPresContext()->TextZoom());
>+  
>+  float fCoord = float(aCoord) / mPresShell->GetPresContext()->TextZoom();
>+  if (font->mFont.size != font->mSize) {

You should probably leave this as a != 0 check.  It may also be good to have an assertion (outside the check) that (font->mFont.size == 0) == (font->mSize == 0).

r=dbaron with those things fixed

Comment 9

[Atul Aggarwal]

Attachment: Patch v2.01

Rebasing on current code.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c1a5d45423

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c6c1a5d45423

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is it possible to turn this testcase into an automated test for our testsuite?

Comment 13

[Boris Zbarsky [:bzbarsky]]

It should be, yes.