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.

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().

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?

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!

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....

>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?

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.

fabian, jst, would you review?

Comment on attachment 60899 [details] [diff] [review] Patch to fix. more bz goodness. r=fabian

checked in under bug 116032 (reviews there).