Closed Bug 98052 Opened 18 years ago Closed 18 years ago

getComputedStyle rounds font-size to integer pixels

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: hsivonen, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(Keywords: testcase)

Attachments

(2 files, 1 obsolete file)

Build ID: 2001090105

Reproducible: Always.

Steps to reproduce a:
a1) Load the attached test case.

Steps to reproduce b:
b1) Download the test harness from bug 31961.
b2) Load
http://www.cnn.com/2001/CAREER/jobenvy/08/28/dilbert.scott.adams/index.html
either in the test harness.
b3) Click the zoom button.
b4) Zoom to 120% after the alert says 1.2.

Actual results:
When the logical resolution is 96 px/in, getComputedStyle return 15px if the
original font size is 11pt. 18px / 15px = 1.2. However, the layout system stores
the font size with more precision. In the layout system 11pt at 96px/in is less
than 15px. When the 11pt font is zoomed by 120%, the displayed size is not 18px
but smaller.

Expected results:
Expected getComputedStyle use the same precision as the internal storage of the
 layout system. That is, expected getComputedStyle to return 11pt or 14.66666666px.
Attached file Test case
It turns out that the layout system stores the font size as a floored pixel
value. 18px/14.6666666666px = 1.227272727 but scaling by even 125% doesn't make
the font 18px. However 18px/14px = 1.285... and scaling by 129% indeed makes the
size 18px.
It seems to me that the problem is
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsROCSSPrimitiveValue.cpp#68

NSTwipsToIntPixels() is called instead of calling NSTwipsToFloatPixels().
Attached patch Patch that might fix this (obsolete) — Splinter Review
I attached an *untested* patch that might fix this. I am unable to test the
patch because I don't have a working build setup. (I don't have CodeWarrior and
building with Apple's tools doesn't work, yet.)

BTW, looking a few lines down in nsROCSSPrimitiveValue.cpp it looks like this:
    case CSS_MM :
      {
        float val = NS_TWIPS_TO_MILLIMETERS(mTwips);
        char buf[64];
        PR_snprintf(buf, 63, "%.2fcm", val);
        tmpStr.AppendWithConversion("mm");
        break;
      }
I think
tmpStr.AppendWithConversion(buf);
is missing. Or am I missing something? Wouldn't tmpStr.AppendFloat(val); be even
better?
Blocks: 42417
Is this OS X specific?  If not, please change OS version to All to indicate a
generic Mac platform bug
taking this.  Henri, thanks for the testcase and patch!
Assignee: jst → bzbarsky
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
Keywords: patch, testcase
removing patch keyword, since that patch will cause all px values returned by
getComputedStyle to be float (which would be wrong, imo).

Ian, what's the expected behavior of getComputedStyle in the following
situation:

font-size: 11pt
11pt == 14.666666 px
Layout uses a 14px font in these circumstances

The more I think about it the more convinced I am that computed style should
just return "14px" in this case....
Keywords: patchqawanted
>cause all px values returned by getComputedStyle to be float 
>(which would be wrong, imo).

Why?

>font-size: 11pt
>11pt == 14.666666 px
>Layout uses a 14px font in these circumstances

According to my testing, it doesn't. The value used for font zoom calculation of
still a float, which is exactly why I filed this bug in the first place.
It makes no sense to return sub-pixel values for computed lengths in most cases
because those are not the values layout is using to actually draw the elements. 
Doing this could lead to weird rounding errors, in my opinion, of course.  :) 
Ian? 

The current computed style implementation does not take font zoom into account
at all.  I've got a patch that fixes that, but I'd like to get this issue
resolved at the same time...

Did I misunderstand comment #2?  You seem to say that layout stores the 11pt
font as 14px...  I'm not sure how you measured that the scaled font is smaller
than 18px, but I know when layout requests a font to actually draw from the
platform it requests an integer-pixel font.  The algorithm for this is actually
fairly convoluted, and the font requested can easily be as many as 2px off from
the computed "float" font size, especially on Linux and especially with CJK
fonts.

Hence my question to Ian: what should we really do here?
Attached patch Patch to fix.Splinter Review
OK.  Talked to Ian.  The basic results of the conversation were:

1)  There really is no good reason not to return floating-point values for
lengths
    in computed style

2)  The computed font-size value should _not_ include text zoom.  Otherwise you

    get weird effects like:

elem.style.fontSize = document.defaultView.getComputedStyle(elem,
null).fontSize

which would break if the computed font size took zoom into account.

The attached patch is basically just like Henri's original patch.  It fixes the
other issue Henri pointed out as well and has some minor cleanup.
Attachment #48560 - Attachment is obsolete: true
fabian, jst, would you review?
Keywords: qawantedpatch, review
OS: MacOS X → All
Hardware: Macintosh → All
Comment on attachment 60899 [details] [diff] [review]
Patch to fix.

more bz goodness.
r=fabian
Attachment #60899 - Flags: review+
checked in under bug 116032 (reviews there).
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.