LOGFONT's lfHeight not converted to points accurately

RESOLVED FIXED

Status

--
minor
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

Trunk
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 179784 [details] [diff] [review]
Proposed patch
(Assignee)

Updated

14 years ago
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 2

14 years ago
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+
(Assignee)

Comment 3

14 years ago
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?
(Assignee)

Comment 5

14 years ago
(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).
(Assignee)

Comment 8

14 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 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|...
(Assignee)

Comment 12

12 years ago
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...
(Assignee)

Comment 14

12 years ago
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.