Closed
Bug 289217
Opened 20 years ago
Closed 20 years ago
LOGFONT's lfHeight not converted to points accurately
Categories
(Core Graveyard :: GFX: Win32, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: neil, Assigned: neil)
Details
Attachments
(1 file)
1.72 KB,
patch
|
emaijala+moz
:
review+
roc
:
superreview+
dbaron
:
approval1.8b2+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
Assignee | ||
Updated•20 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•20 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•20 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•20 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•20 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 9•18 years ago
|
||
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.
Comment 10•18 years ago
|
||
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.
Comment 11•18 years ago
|
||
Ah, tmAscent has internal leading. You're right.
we should use |tm.tmAscent - tm.tmIndernalLeading + tm.tmDescent|...
Assignee | ||
Comment 12•18 years ago
|
||
Those positive LOGFONTs were an edge case anyway, they're normally negative.
Comment 13•18 years ago
|
||
neil:
What means "an edge case"? I cannot translate the words to Japanese...
Assignee | ||
Comment 14•18 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.
Comment 15•18 years ago
|
||
thank you, neil.
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•