Closed Bug 289217 Opened 16 years ago Closed 16 years ago

LOGFONT's lfHeight not converted to points accurately

Categories

(Core Graveyard :: GFX: Win32, defect)

x86
Windows NT
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file)

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.
Attached patch Proposed patchSplinter Review
Assignee: win32 → neil.parkwaycc.co.uk
Status: NEW → ASSIGNED
Attachment #179784 - Flags: superreview?(roc)
Attachment #179784 - Flags: review?(emaijala)
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.