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

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Dmitry Stepanov, Assigned: Atul Aggarwal)

Tracking

({testcase})

unspecified
mozilla14
x86
All
testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

8 years ago
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-
Keywords: testcase
OS: Windows 7 → All
Summary: line-height reported by window.getComputedStyle is "-1.78957e+7px" if font-size is 0px → line-height reported by window.getComputedStyle is incorrect if font-size is 0px

Updated

8 years ago
Status: UNCONFIRMED → NEW
Component: General → HTML: Parser
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → parser
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?
Component: HTML: Parser → DOM: CSS Object Model
QA Contact: parser → general
QA Contact: general → style-system
(Assignee)

Comment 3

6 years ago
Created attachment 560806 [details]
Testcase

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

Comment 4

6 years ago
Created attachment 560875 [details] [diff] [review]
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.
Assignee: nobody → atulagrwl
Attachment #560875 - Flags: review?(dbaron)
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 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).
Attachment #560875 - Flags: review?(dbaron) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 570976 [details] [diff] [review]
Patch v2
Attachment #560875 - Attachment is obsolete: true
Attachment #570976 - Flags: review?(dbaron)
Comment on attachment 570976 [details] [diff] [review]
Patch v2

># HG changeset patch
># Parent 6ec5b28142d10d83046bdce764fbc1fe3598d4be
># User Atul Aggarwal <atulagrwl@gmail.com>
># 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
Attachment #570976 - Flags: review?(dbaron) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 606562 [details] [diff] [review]
Patch v2.01

Rebasing on current code.
Attachment #570976 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c1a5d45423
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/c6c1a5d45423
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Is it possible to turn this testcase into an automated test for our testsuite?
It should be, yes.
You need to log in before you can comment on or make changes to this bug.