Last Comment Bug 536758 - line-height reported by window.getComputedStyle is incorrect if font-size is 0px
: line-height reported by window.getComputedStyle is incorrect if font-size is 0px
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla14
Assigned To: Atul Aggarwal
:
Mentors:
http://www.stepanov.lv/pub/mozilla/bu...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-25 15:18 PST by Dmitry Stepanov
Modified: 2012-11-10 12:37 PST (History)
5 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (688 bytes, text/html)
2011-09-18 12:32 PDT, Atul Aggarwal
no flags Details
Patch v1 (1.37 KB, patch)
2011-09-19 04:50 PDT, Atul Aggarwal
dbaron: review-
Details | Diff | Splinter Review
Patch v2 (1.40 KB, patch)
2011-11-01 05:50 PDT, Atul Aggarwal
dbaron: review+
Details | Diff | Splinter Review
Patch v2.01 (1.45 KB, patch)
2012-03-16 07:16 PDT, Atul Aggarwal
no flags Details | Diff | Splinter Review

Description Dmitry Stepanov 2009-12-25 15:18:04 PST
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) 2009-12-31 01:05:58 PST
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 [:bz] 2010-04-09 22:35:10 PDT
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 2011-09-18 12:32:39 PDT
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.
Comment 4 Atul Aggarwal 2011-09-19 04:50:02 PDT
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.
Comment 5 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-09-19 16:20:38 PDT
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+2 (review requests must explain patch) 2011-09-19 16:41:16 PDT
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 2011-11-01 05:50:50 PDT
Created attachment 570976 [details] [diff] [review]
Patch v2
Comment 8 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2011-12-06 16:31:37 PST
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
Comment 9 Atul Aggarwal 2012-03-16 07:16:52 PDT
Created attachment 606562 [details] [diff] [review]
Patch v2.01

Rebasing on current code.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-03-16 15:40:03 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c1a5d45423
Comment 11 Phil Ringnalda (:philor, back in August) 2012-03-17 17:29:00 PDT
https://hg.mozilla.org/mozilla-central/rev/c6c1a5d45423
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-11-10 11:13:58 PST
Is it possible to turn this testcase into an automated test for our testsuite?
Comment 13 Boris Zbarsky [:bz] 2012-11-10 12:37:36 PST
It should be, yes.

Note You need to log in before you can comment on or make changes to this bug.