Closed Bug 289217 Opened 16 years ago Closed 16 years ago
Height not converted to points accurately
If you examine a typical UI font in DOM Inspector its height will show up as something like 10.6667px although in fact it is 11px. The code also does not correctly deal with positive lfHeight values although themes generally don't use any fonts that exhibit this problem but I decided to fix it anyway.
Attachment #179784 - Flags: superreview?(roc) → superreview+
Comment on attachment 179784 [details] [diff] [review] Proposed patch Please prepend the Win api calls with the usual :: for consistency if not anything else. With that done, r=emaijala
Attachment #179784 - Flags: review?(emaijala) → review+
Comment on attachment 179784 [details] [diff] [review] Proposed patch Correction to the computed system font size e.g. as displayed in DOM Inspector.
Attachment #179784 - Flags: approval1.8b2?
Well, I wouldn't quite summarize it that way. You're changing the way we get the font size of CSS system fonts so that it's hopefully more accurate. It could potentially affect the display of such fonts, although hopefully it's just been rounded back the way it came. Have you tested with a bunch of different fonts that we are still displaying them the same size they show up in the control panel and in other apps?
(In reply to comment #4) >Well, I wouldn't quite summarize it that way. You're changing the way we get >the font size of CSS system fonts so that it's hopefully more accurate. It >could potentially affect the display of such fonts, although hopefully it's just >been rounded back the way it came. I assumed somehow the old inaccurate value was being rounded back, yes. >Have you tested with a bunch of different fonts that we are still displaying >them the same size they show up in the control panel and in other apps? Just 11px and 13px so far... how big is a bunch?
Comment on attachment 179784 [details] [diff] [review] Proposed patch The point was really that if you don't understand what the patch should be changing, we really can't trust you to be testing your patches appropriately and probably shouldn't be approving your patches to core code during freeze periods. That said, the patch looks good to me, so a=dbaron.
Attachment #179784 - Flags: approval1.8b2? → approval1.8b2+
Regarding the testing: it's worth testing both at different font sizes and (more importantly, probably) at different logical resolutions ("Normal fonts" == 96 dpi, "Large Fonts" == 120dpi, and numbers that aren't quite so round).
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Neil and Ere: I found a strange code in thebes. That comes from this patch. I don't know whether the my question is valid. Therefore, I comment here instead of filing new bug. > + pixelHeight = tm.tmAscent; The patch sets only tmAscent for actual height. But in the document of Microsoft, the character height is the sum of ascent and descent. http://support.microsoft.com/kb/32667/en So the patch has a bug if the document is correct.
Please note that TEXTMETRIC is come from LOGFONT whose lfHeight is positive. Positive lfHeight means Character Height + Internal Leading. We want Character Height only. Internal Leading of many Western fonts has the same value as Descent. So calculation will match. But I'm not sure it's also correct for all other languages.
Ah, tmAscent has internal leading. You're right. we should use |tm.tmAscent - tm.tmIndernalLeading + tm.tmDescent|...
Those positive LOGFONTs were an edge case anyway, they're normally negative.
neil: What means "an edge case"? I cannot translate the words to Japanese...
I mean that although they're documented as permissible I don't know of any cases (at least using Windows 2000 or later) for which they occur within Gecko.
thank you, neil.
You need to log in before you can comment on or make changes to this bug.