216670 - for several italicized bitmap fonts, the overhang value is not available

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Hideo Saito]

This bug derives from bug 52596.

The overhang value of the following italicized bitmap fonts is not available,
for example, 'Fixed Sys', 'System', 'Terminal'.

Although the following patch solves this problem, it may be hard to approve this
patch.

--- ./gfx/src/windows/nsFontMetricsWin.cpp2	Sat Aug  9 09:08:13 2003
+++ ./gfx/src/windows/nsFontMetricsWin.cpp	Sun Aug 17 22:57:31 2003
@@ -2290,7 +2290,18 @@
   ::GetTextMetrics(aDC, &metrics);
   aFont->mMaxAscent = NSToCoordRound(metrics.tmAscent * dev2app);
   aFont->mMaxDescent = NSToCoordRound(metrics.tmDescent * dev2app);
-  aFont->mOverhangCorrection = (IsWin95OrWin98() ? metrics.tmOverhang : 0);
+  aFont->mOverhangCorrection = 0;
+  if (IsWin95OrWin98()) {
+    aFont->mOverhangCorrection = metrics.tmOverhang;
+    if (metrics.tmItalic &&
+        !(metrics.tmPitchAndFamily & (TMPF_VECTOR | TMPF_TRUETYPE |
TMPF_DEVICE))) {
+      SIZE  size;
+      ::GetTextExtentPoint32(aDC, " ", 1, &size);
+      SIZE  size2;
+      ::GetTextExtentPoint32(aDC, "  ", 2, &size2);
+      aFont->mOverhangCorrection = size.cx * 2 - size2.cx;
+    }
+  }
   aFont->mMaxCharWidthMetric = metrics.tmMaxCharWidth;
   aFont->mMaxHeightMetric = metrics.tmHeight;
   aFont->mPitchAndFamily = metrics.tmPitchAndFamily;

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Here is test case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1745&action=view
and screenshot of above testcase.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1746&action=view

Comment 2

[rbs]

Why didn't you mention this problem in the course of dealing with the other bug?
I assumed you had done enough testing.

Comment 3

[Hideo Saito]

Sorry, my testing was not enough. As a result of my insufficient testing, this
bug was reported by our tester using latest trunk. However , a patch of bug
52596 was not enough but not wrong.

A patch was changed.

--- ./gfx/src/windows/nsFontMetricsWin.cpp2	Sat Aug  9 09:08:13 2003
+++ ./gfx/src/windows/nsFontMetricsWin.cpp	Fri Aug 22 00:16:08 2003
@@ -2290,7 +2290,22 @@
   ::GetTextMetrics(aDC, &metrics);
   aFont->mMaxAscent = NSToCoordRound(metrics.tmAscent * dev2app);
   aFont->mMaxDescent = NSToCoordRound(metrics.tmDescent * dev2app);
-  aFont->mOverhangCorrection = (IsWin95OrWin98() ? metrics.tmOverhang : 0);
+  aFont->mOverhangCorrection = 0;
+  if (IsWin95OrWin98()) {
+    aFont->mOverhangCorrection = metrics.tmOverhang;
+    if (metrics.tmItalic &&
+        !(metrics.tmPitchAndFamily & (TMPF_VECTOR | TMPF_TRUETYPE |
TMPF_DEVICE))) {
+      SIZE  size;
+      ::GetTextExtentPoint32(aDC, " ", 1, &size);
+      if (!(metrics.tmPitchAndFamily & TMPF_FIXED_PITCH)) {
+        aFont->mOverhangCorrection = size.cx - metrics.tmAveCharWidth;
+      } else {
+        SIZE  size2;
+        ::GetTextExtentPoint32(aDC, "  ", 2, &size2);
+        aFont->mOverhangCorrection = size.cx * 2 - size2.cx;
+      }
+    }
+  }
   aFont->mMaxCharWidthMetric = metrics.tmMaxCharWidth;
   aFont->mMaxHeightMetric = metrics.tmHeight;
   aFont->mPitchAndFamily = metrics.tmPitchAndFamily;

Comment 4

[rbs]

To verify that your estimator is good, you need to test that the value that you
compute agrees with those fonts where the built-in value is available.

i.e., if (metrics.tmOverhang), is the estimator close to metrics.tmOverhang?

Comment 5

[Hideo Saito]

A following list indicates a result of my testing for italicized fonts on Win98SE.
A sign of '*' indicates that the patch has influence on the font.
A sign of '#' indicates that the patch solves this problem, since
mOverhangCorrection is available although tmOverhang is zero.

        mOverhangCorrection tmOverhang mPitchAndFamily
Arial                   0          0         0x27
Arial Black             0          0         0x27
Arial Narrow            0          0         0x27
Century Gothic          0          0         0x27
Impact                  0          0         0x27
Tahoma                  0          0         0x27
Trebuchet MS            0          0         0x27
Verdana                 0          0         0x27
8514oem                 0          0         0x27
Book Antiqua            0          0         0x17
Bookman Old Style       0          0         0x17
Century                 0          0         0x17
Comic Snas MS           0          0         0x47
Garamond                0          0         0x17
MS Outlook              0          0         0x27
MS UI Gothic            0          0         0x37
Marlett                 0          0         0x27
Modern                  0          0         0x27
Symbol                  0          0         0x27
Times New Roman         0          0         0x17
Webdings                0          0         0x27
Wingdings               0          0         0x27
Bookshelf Symbol 1      0          0         0x07
Bookshelf Symbol 2      0          0         0x07
Bookshelf Symbol 3      0          0         0x27
Bookshelf Symbol 4      0          0         0x27
Bookshelf Symbol 5      0          0         0x27

Courier New             0          0         0x36
Lucida Console          0          0         0x36
OCRB                    0          0         0x36

MS Sans Serif          19         19         0x21  *
MS Serif               19         19         0x11  *
System                 10          0         0x01  * #
FixedSys               10          0         0x00  * #
Small Fonts            17          0         0x30  * #
Terminal               17          0         0x30  * #
Courier                15         15         0x30  *

My testing is to confirm whether following italicized text expands when selected.
<body style="font-size: 36px;">
<i>abcdefghijklmnopqrstuvwxyz</i><br>
</body>

Comment 6

[rbs]

+    if (metrics.tmItalic &&
+        !(metrics.tmPitchAndFamily & (TMPF_VECTOR | TMPF_TRUETYPE |
TMPF_DEVICE))) {

it seems you need: if (!metrics.tmOverhang && metrics.tmItalic &&

+      SIZE  size;
+      ::GetTextExtentPoint32(aDC, " ", 1, &size);
+      if (!(metrics.tmPitchAndFamily & TMPF_FIXED_PITCH)) {

do you ever get inside this |if| {...}? It wasn't there in your previous patch.
When you have to add such non-intuitive extras, please document why you do so.
This way, the code will be kept robust. Once you are ready, care to attach your
patch for r+sr.

Comment 7

[Hideo Saito]

> it seems you need: if (!metrics.tmOverhang && metrics.tmItalic &&

For bold italic font, the value of tmOverhang is always one even if large font
is used, as shown in the following list.

        mOverhangCorrection tmOverhang mPitchAndFamily
MS Sans Serif          87         87         0x21  *
System                 72          1         0x01  * #
FixedSys               72          1         0x00  * #
Terminal               72          1         0x30  * #
Courier                70         70         0x30  *

<body style="font-size: 148px; font-weight: 900;">
<i>abcdefghijklmnopqrstuvwxyz</i>
</body>

I think that a program will be changed as follows.

if (metrics.tmOverhang < 2 && metrics.tmItalic &&
or
if (metrics.tmOverhang < 3 && metrics.tmItalic &&

Comment 8

[rbs]

That's okay... Just leave it, or it will add more special-case unnecessarily. It
would have been justfied if the internal value was good while the estimator was
off the mark. But from the data that you show, the estimate seems really good,
isn't it?

Comment 9

[Hideo Saito]

Attachment: patch

I confirmed that the bold and italic text using the bitmap fonts does not
expand when selected.

Comment 10

[rbs]

Comment on attachment 130536 [details] [diff] [review]
patch

+      if (!(metrics.tmPitchAndFamily & TMPF_FIXED_PITCH)) {
+	 aFont->mOverhangCorrection = size.cx - metrics.tmAveCharWidth;
+      }

what about this one? you didn't comment why you special-case (c.f. comment 6).

Comment 11

[Hideo Saito]

Attachment: patch

I add the next comment.

+      if (!(metrics.tmPitchAndFamily & TMPF_FIXED_PITCH)) {
+	 // for monospace fonts, we can optimize to call Windows API
+	 // since the value of tmAveCharWidth does not include the overhang.
+	 aFont->mOverhangCorrection = size.cx - metrics.tmAveCharWidth;
+      } else {

Comment 12

[rbs]

Comment on attachment 130578 [details] [diff] [review]
patch

r+sr=rbs

Comment 13

[Asa Dotzler [:asa]]

What's the risk of this patch and what kind of testing has been done?

Comment 14

[Hideo Saito]

> What's the risk of this patch and

1. for the italicized bitmap fonts on Windows9x, we need to call twice windows
API in order to compute the overhang value. But I think that the cost is not
expensive since it costs only to load the font.

2. for the weight of these font is bold, if the overhang value is greater than
or equal to 3, the overhang value is not computed correctly. But the actual
value of overhang is always one using bold italic bitmap fonts.

> what kind of testing has been done?

1. I confirmed that following the bold and italic text using the bitmap fonts
does not expand when selected.

<body style="font-size: 36px;">
<b><i>abcdefghijklmnopqrstuvwxyz</i></b>
</body>

2. I confirmed that following test case is drawn correctly using bitmap fonts.

http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1745&action=view

Comment 15

[rbs]

Comment on attachment 130578 [details] [diff] [review]
patch

+      if (!(metrics.tmPitchAndFamily & TMPF_FIXED_PITCH)) {
+	 // for monospace fonts, we can optimize to call Windows API
+	 // since the value of tmAveCharWidth does not include the overhang.
+	 aFont->mOverhangCorrection = size.cx - metrics.tmAveCharWidth;
+      }

Re-reading again, I wondered if, based on your comments, you meant if (metrics.
...), i.e., you don't need the "not" (!)?

Or your comment is incorrect.
[printf() inside the |if| helps...]

Comment 16

[Hideo Saito]

For a bit of TMPF_FIXED_PITCH of metrics.tmPitchAndFamily,
if this bit is set the font is a variable pitch font,
if this bit is clear the font is a fixed pitch font.

The following calculation is available for monospace fonts.
+	 aFont->mOverhangCorrection = size.cx - metrics.tmAveCharWidth;

Comment 17

[rbs]

Ah yes, you are right... 

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/fontext_7ss2.asp
The GDI documentation had the warning that: 
"TMPF_FIXED_PITCH 	If this bit is set the font is a variable pitch font. If this
bit is clear the font is a fixed pitch font. NOTE VERY CAREFULLY THAT THOSE
MEANINGS ARE THE OPPOSITE OF WHAT THE CONSTANT NAME IMPLIES." (emphasis is mine...)

I am satisfied with the patch, then.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 130578 [details] [diff] [review]
patch

Let's wait 'till 1.6a since we won't have a lot of testing between now and 1.5
reelease.

Comment 19

[rbs]

Checked in (1.6a trunk).