for several fonts, bold text expands when selected (mostly sans serif fonts?)

RESOLVED FIXED in Future

Status

()

P3
normal
RESOLVED FIXED
19 years ago
16 years ago

People

(Reporter: jruderman, Assigned: mjudge)

Tracking

({testcase, top100})

Trunk
Future
x86
Windows 98
testcase, top100
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(9 attachments, 13 obsolete attachments)

263 bytes, text/html
Details
1.73 KB, text/html
Details
1.71 KB, image/png
Details
1006 bytes, text/html
Details
3.97 KB, text/plain
Details
4.00 KB, application/octet-stream
Details
6.06 KB, image/png
Details
465 bytes, text/html
Details
9.60 KB, patch
rbs
: review+
Details | Diff | Splinter Review
(Reporter)

Description

19 years ago
[Splitting bug 50825]

Selecting text that is bold and in the ms sans serif font causes the text to 
expand horizontally.  It doesn't matter whether the styles are defined using 
HTML 3.2 or CSS.

Various things to try:
- Double-click on "for" to select the word.
- Put the cursor in the middle of a word and drag left and right.
- Select text around the colons.
(Reporter)

Comment 1

19 years ago
Created attachment 14664 [details]
testcase

Comment 2

19 years ago
setting to future.

anthonyd
Target Milestone: --- → Future

Comment 3

19 years ago
*** Bug 59080 has been marked as a duplicate of this bug. ***

Comment 4

19 years ago
Lucida sans and Impact also causes this problem.

Arial and Century Gothic are displayed fine.

In Japanese encoding every fonts have this problem.

Comment 5

19 years ago
I can't reproduce this on WinNT.

(Reporter)

Comment 6

19 years ago
*** Bug 61511 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 7

19 years ago
Copying top100 keyword from dup bug (happens at http://www.cnet.com/).
Keywords: top100
(Reporter)

Comment 8

19 years ago
The dup bug also has a neat testcase that uses various fonts and also uses 
italics (the text shifts by several pixels when you select it if it's italic as 
well as bold).
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19893
Keywords: testcase

Comment 9

19 years ago
Yep I like my test cases too. :-)

What is weird about this bug is that it only happens with specific bolded fonts
of the sans serif family (variable width) NOT all of them though.

It looks like a space is added to the beginning and end of the selected word(s).
 Strange though that if you select the whole line this bad rendering does not
occure.

Is this a regression or has this always been a problem? I will download a month
or 2 back to see... if no one else knows...
Severity: minor → normal
(Reporter)

Comment 10

19 years ago
Also happens with at least one serif font (ms serif).
Summary: bold, "ms sans serif" text expands when selected → for several fonts, bold text expands when selected (mostly sans serif fonts?)

Comment 11

18 years ago
hmm... seems like the only thing similar for the "problem" fonts is that they
are MS fonts?
(Reporter)

Comment 12

18 years ago
Fwiw, AIM 4.3.2229 has this bug too.  If you type this into aim, bold it, and 
then select part of it, the text moves a litte.
<FONT COLOR="#0000ff" FACE="Tempus Sans ITC" SIZE=2>foo

Comment 13

18 years ago
I've seen this problem with italic MS Sans Serif. I think the problem is
occuring because it is a bitmap font.

Comment 14

18 years ago
Created attachment 35687 [details]
Expanded testcase - bitmap fonts

Comment 15

18 years ago
Created attachment 35689 [details]
Image demonstrating undesirable behavior
It sounds like this bug and bug 91485 share the same underlying problem.
I could not reproduce this bug for the italic style using a TrueType font.
However I could reproduce this bug for the bold style, but only if there was no
separate bold font variant installed.
The bold italic style appears to be synthesized from the bold font variant in
preference.
Created attachment 48841 [details]
Is this the same problem?

Comment 19

18 years ago
Try the first testcase submitted on 09/13/00.

Select the bolded word and the selected text "shifts" or "expands" the letter
spacing out the right.

Another site that I have noticed this with is www.mutualfundsnet.com
Select any of the bold or italic text demonstrates this same "expanding" problem.

This bug deals with text selection.
(Assignee)

Comment 20

17 years ago
*** Bug 91485 has been marked as a duplicate of this bug. ***
The result tested by Japanese MS Gothic is submitted to Bugzilla-jp. 
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=395

It seems that width +1px for which the character sequence needs 
GetTextExtentPoint32 (Win32API) by Windows9x in the case of 
a bold as a return value. 

Return values(width) of GetTextExtentPoint32A
Win98     Win2k
   N   B   N   B   (N...Normal,B...Bold)
  94 100  94  99 : AIUEO(Japanese Character,Hiragana)

  40  43  40  42 : AI
  54  58  54  57 : UEO
----------------------------------------
  94 101  94  99 

  20  22  20  21 : A
  20  22  20  21 : I
  15  17  15  16 : U
  19  21  19  20 : E
  20  22  20  21 : O
----------------------------------------
  94 104  94  99

Comment 22

17 years ago
Might be OS idiosyncrasies the problem is not reproducable on WinNT/2K. Here is
what is happening (from the MS GDI documentation):

"When necessary, the system synthesizes a font by changing the character 
bitmaps. To synthesize a character in a bold font, the system draws the 
character twice: at the starting point, and again one pixel to the right of the 
starting point. To synthesize a character in an italic font, the system draws 
two rows of pixels at the bottom of the character cell, moves the starting point 
one pixel to the right, draws the next two rows, and continues until the 
character has been drawn. By shifting pixels, each character appears to be 
sheared to the right. The amount of shear is a function of the height of the 
character."

More at: http://msdn.microsoft.com/library/en-us/gdi/fontext_8f6t.asp
Could this extract from rbs' link be the problem?

In this example, the GetTextExtentPoint32 function initializes the members of a
SIZE structure with the length and height of the specified string. The
GetTextMetrics function retrieves the overhang for the current font. Because the
overhang is zero if the font is a TrueType font, the overhang value does not
change the string placement. For raster fonts, however, it is important to use
the overhang value.

I only see the problem with raster fonts, not TrueType fonts...

Comment 24

17 years ago
Bug 115522 for a similar problem on Mac.

Comment 25

17 years ago
changing selection qa to tpreston.
QA Contact: blaker → tpreston

Comment 26

17 years ago
I'm not seeing the problem which was demonstrated in my first testcase any
longer in 2002021203 WXP. Can anyone double-check this on a Win98 platform?
Skewer wrote:

> I'm not seeing the problem which was demonstrated in my first testcase any
> longer in 2002021203 WXP.

That's because WXP doesn't have any bitmap fonts, they are all TrueType fonts
and therefore the font metrics never have an overhang value for Mozilla to ignore.

Comment 28

17 years ago
It sure seems like MS Serif and MS Sans Serif are still bitmaps... they're not
even affected by ClearType. I'd appreciate if a Win98 user would share their
results with a recent build, though.
Sorry, I didn't see them at first because MS had "hidden" them :-(

And the problem is still very much alive on both Windows 95 and 98.

Comment 30

17 years ago
I'd like to mention that the testcase at
http://bugzilla.mozilla.org/attachment.cgi?id=19893&action=view also affects me
with mozilla 0.98 on both Linux and Irix. Fails on Trebuchet, which also gets
corrupted when it has been selected and gets scrolled partitially out of view
and back in. This testcase is from bug 61511, which is marked as a duplicate of
this, but none of the testcases here show this effect for me. Nothing of all
this happens with Netscape 4.7*, so it's not a broken font.

I cannot find another bug that mentions font corruption when scrolling that
mentions slashdot's white headlines on green background as an example. I believe
that's a duplicate of this bug.

Since someone mentioned something strikingly similar on MacOS, I'd say
Platform/OS needs to be set to ALL/ALL.

Comment 31

17 years ago
Created attachment 103301 [details]
The program which gets the value of tmOverhang

Comment 32

17 years ago
This program of attachment 103301 [details] can get the value of tmOverhang of
GetTextMetrics function of API32. By the test, font weight is set to BOLD using
the font of Times New Roman.

Please compile using cygwin.
g++ -o cfont cfont.cc -mwindows <return>

a result of execution on Win98:
tmWeight = 700
tmOverhang = 1

a result of execution on WinNT:
tmWeight = 700
tmOverhang = 0

An overhang value is usually 0 and it is thought that a problem occurs in other
than zero.
It is thought that an overhang value should subtract from the string length
which asked by GetTextExtentPoint32API.

Comment 33

17 years ago
Created attachment 103341 [details] [diff] [review]
patch for subtract an overhang value from the string length

Comment 34

16 years ago
Created attachment 126203 [details] [diff] [review]
patch

In win98SE, there are following serious bugs as to rendering a string.

1.  The dimensions of the string that is calculated by the GetTextExtentPoint32
function adds one pixel of tmOverhang of TEXTMETRIC structure.
2.  The ExtTextOut function draws a string shorter than the specified length,
if we use monospace fonts and bold simulation fonts and the value of tmOverhang
is not zero.
(for example, if the number of characters is 30, the last 3 characters cannot
be drawn.)

Please refer to fallowing URL.

manual of GetTextExtentPoint32:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/fontext_8smq.asp

manual of ExtTextOut:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/fontext_2ks4.asp
Attachment #103341 - Attachment is obsolete: true

Updated

16 years ago
Flags: blocking1.4?
This problem is very serious for the user of Win9x.
This problem is not minor.

See this case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1666&action=view
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1667&action=view

The bold text is overlaps with the next text when the bold text has letter-spacing.

See another case.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1650&action=view
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=1651&action=view

The text was lost around the selected text when the text is monospace font.

We have a report in Bugzilla-jp.
The reporter said, these problem is fixed by attachment 126203 [details] [diff] [review].

This problem should be fixed 1.4 final.
Comment on attachment 126203 [details] [diff] [review]
patch

>-    aBoundingMetrics.width = size.cx;
>-    aBoundingMetrics.rightBearing = size.cx - gm.gmCellIncX + gm.gmptGlyphOrigin.x + gm.gmBlackBoxX;
>+    TEXTMETRIC metrics;
>+    metrics.tmOverhang = 0;
>+    if (IsWin95OrWin98())
>+      ::GetTextMetrics(aDC, &metrics);
>+    aBoundingMetrics.width = size.cx - metrics.tmOverhang;
>+    aBoundingMetrics.rightBearing = size.cx - metrics.tmOverhang - gm.gmCellIncX + gm.gmptGlyphOrigin.x + gm.gmBlackBoxX;
>   }

If this is really the correct solution, logically, wouldn't it be cleaner to
add:

if (IsWin95OrWin98()) {
  TEXTMETRIC metrics;
  ::GetTextMetrics(aDC, &metrics);
  size.cx -= metrics.tmOverhang;
}

and then leave the existing code untouched?

However, why should this not be done for all platforms?  The documentation you
cite [1] says that there's an extra one pixel on those platforms for bold and
italic fonts.  However, [2] seems to suggest that |tmOverhang| is the right
value all the time.  If the extra pixel (or whatever amount) is not added on
Windows NT/2000, does that mean tmOverhang is zero on those platforms?

[1]http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/fontext
_8smq.asp
[2]http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/fontext
_7ss2.asp

Comment 37

16 years ago
Created attachment 126332 [details] [diff] [review]
patch

Thanks for your advice. The patch was changed according to your advice.

To be sure, in the case of WinNT/WinXP, tmOverhang in an outline font is zero.
However, in the case of the raster font (for example, ms sans serif), 
tmOverhang was 1. In order to avoid this problem, IsWin95OrWin98 () is called.
Attachment #126203 - Attachment is obsolete: true

Comment 38

16 years ago
This patch is very inefficient. Cache what you want in InitMetricsFor() before
getting started.

Comment 39

16 years ago
Created attachment 126364 [details] [diff] [review]
patch

rbs, thanks for your advice.
Attachment #126332 - Attachment is obsolete: true

Comment 40

16 years ago
Created attachment 126746 [details] [diff] [review]
patch

There was a error at the following program of this patch.
The error is that the dimension between the starting points of a full size
character is same to the dimension between the starting points of a half size
character.

+PRBool
+nsFontWin::nsExtTextOutW(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT lpsc, LPCWSTR aString, UINT aLength, INT *lpDx)
+{
+  PRBool result;
+  INT dxMem[500];
+  INT* dx0 = lpDx;
+
+  if (!lpDx && mtmOverhang > 0 && !(mtmPitchAndFamily & TMPF_FIXED_PITCH)) {
+    dx0 = dxMem;
+    if (aLength > 500) {
+      dx0 = new INT[aLength];
+    }
+    PRUint32 idx;
+    for (idx = 0; idx < aLength; idx++)
+      dx0[idx] = mtmAveCharWidth;
+  }
+  result = ::ExtTextOutW(aDC, aX, aY, uOptions, lpsc, aString, aLength, dx0);
+  if (dx0 && (dx0 != lpDx) && (dx0 != dxMem)) {
+    delete [] dx0;
+  }
+  return result;
+}
+

I tested the new patch on Win98SE successfully.

Updated

16 years ago
Attachment #126364 - Attachment is obsolete: true

Comment 41

16 years ago
Created attachment 126905 [details] [diff] [review]
patch

There was a careless mistake that causes a compile error.
Attachment #126746 - Attachment is obsolete: true

Updated

16 years ago
Attachment #126905 - Flags: review?(rbs)

Updated

16 years ago
Depends on: 212723

Comment 42

16 years ago
Comment on attachment 126905 [details] [diff] [review]
patch

   nscoord	   mMaxAscent;
   nscoord	   mMaxDescent;
+  LONG 	   mtmOverhang;
+  LONG 	   mtmAveCharWidth;
+  LONG 	   mtmMaxCharWidth;
+  BYTE 	   mtmPitchAndFamily;


Change |mtm| to |m| as done next to you

 GetBoundingMetricsCommon(HDC aDC, const PRUnichar* aString, PRUint32 aLength, 
-  nsBoundingMetrics& aBoundingMetrics, PRUnichar* aGlyphStr)
+  nsBoundingMetrics& aBoundingMetrics, PRUnichar* aGlyphStr, LONG aOverhang)

Put |aOverhang| as second argument.
(A better name would be |aOverhangCorrection| as this is really what it does,
but it is too mouthful).

PRBool
+nsFontWin::nsExtTextOutA(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT lpsc, LPCSTR aString, UINT aLength, INT *lpDx)
+{
+  PRBool result;
+  INT dxMem[500];
+  INT* dx0 = lpDx;
+
+  if (!lpDx && mtmOverhang > 0 && !(mtmPitchAndFamily & TMPF_FIXED_PITCH)) {
+    dx0 = dxMem;
+    if (aLength > 500) {
+      dx0 = new INT[aLength];
+    }
+    PRUint32 idx;
+    for (idx = 0; idx < aLength; idx++)
+      dx0[idx] = mtmAveCharWidth;
+  }
+  result = ::ExtTextOutA(aDC, aX, aY, uOptions, lpsc, aString, aLength, dx0);
+  if (dx0 && (dx0 != lpDx) && (dx0 != dxMem)) {
+    delete [] dx0;
+  }
+  return result;
+}

You appear to be spacing the characters uniformly. Why do you want to
turn all fonts into a monospace appearance?


+PRBool
+nsFontWin::nsExtTextOutW(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT lpsc, LPCWSTR aString, UINT aLength, INT *lpDx)
+{
+  PRBool result;
+  INT dxMem[500];
+  INT* dx0 = lpDx;
+
+  if (!lpDx && mtmOverhang > 0 && !(mtmPitchAndFamily & TMPF_FIXED_PITCH)) {
+    dx0 = dxMem;
+    if (aLength > 500) {
+      dx0 = new INT[aLength];
+    }
+    UINT aLen = aLength;
+    PRUint32 idx;
+    for (idx = 0; idx < aLength;) {
+      SIZE size;
+      ::GetTextExtentPoint32W(aDC, &aString[idx], aLen, &size);
+      size.cx -= mtmOverhang;
+      if (aLen <= 1 || 
+	   size.cx == mtmAveCharWidth * aLen || 
+	   size.cx == mtmMaxCharWidth * aLen) {
+	 PRUint32 count;
+	 for (count = 0; count < aLen; count++, idx++)
+	   dx0[idx] = size.cx / aLen;
+	 aLen = aLength - idx;
+      } else
+	 aLen >>= 1;
+    }
+  }
+  result = ::ExtTextOutW(aDC, aX, aY, uOptions, lpsc, aString, aLength, dx0);
+  if (dx0 && (dx0 != lpDx) && (dx0 != dxMem)) {
+    delete [] dx0;
+  }
+  return result;
+}

What is going on here? Measuring each possible substring suffix? And what for? 
It is going to be terribly slow... I don't actually see why you need to tweak
the
drawing functions at all. The measuring functions should be leaving enough room
for the drawing to fit.

[Also sync with bug 212723 which is change that I will be making.]
Attachment #126905 - Flags: review?(rbs) → review-

Comment 43

16 years ago
I pointed out a bug of ExtTextOut function on Win98 at comment 34.

> In win98SE, there are following serious bugs as to rendering a string.
>
> 2.  The ExtTextOut function draws a string shorter than the specified length,
> if we use monospace fonts and bold simulation fonts and the value of tmOverhang
> is not zero.
> (for example, if the number of characters is 30, the last 3 characters cannot
> be drawn.)

By specifying array of spacing values, this problem is avoidable.

> What is going on here? Measuring each possible substring suffix? And what for? 
> It is going to be terribly slow... I don't actually see why you need to tweak
> the drawing functions at all. The measuring functions should be leaving enough
> room for the drawing to fit.

A Full size character and half size character are mixed with the string.
They are undistinguishable from Unicode. The method except measuring was not
able to be found.

Comment 44

16 years ago
Created attachment 127880 [details] [diff] [review]
patch for Bug 212723
Attachment #126905 - Attachment is obsolete: true

Comment 45

16 years ago
It sure looks like an overly expensive fix for the problem. Is there a MSDN
knowlege base article (or some other in-depth reference) about this issue?

Comment 46

16 years ago
Created attachment 127928 [details]
a test program using cygwin

Comment 47

16 years ago
Created attachment 127929 [details]
screen shot of test program

Sorry, I don't know any reference about this problem, but I append a test
program. If you have an environment of cygwin, please compile and execute it on
win98.
A screen shot is also appended. If the font of Lucida Console is displayed by
bold, the first line will be displayed shorter than the appointed number of
characters.

first line:
    ExtTextOut(hdc, 40, 30, 0, NULL,
"0000000000111111111122222222223333333333444444444455555555556666666666", 50,
NULL);
second line:
    ExtTextOut(hdc, 40, 60, 0, NULL,
"0000000000111111111122222222223333333333444444444455555555556666666666", 50,
dx0);

Comment 48

16 years ago
I can only test on Win2K. And it doesn't have this bug.

CC:ing timeless who has helped when looking at other Win98 weirdness in the
past. Any chance that you can chime in here and reproduce what was reported
througout the bug?

Comment 49

16 years ago
saito, care to try this change just to see if makes any difference...?
(it will activate a different code-path that has been there for Win95-Japanese)

Index: nsGfxFactoryWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/windows/nsGfxFactoryWin.cpp,v
retrieving revision 3.36
diff -u -r3.36 nsGfxFactoryWin.cpp
--- nsGfxFactoryWin.cpp 12 Sep 2002 04:43:08 -0000      3.36
+++ nsGfxFactoryWin.cpp 17 Jul 2003 06:51:02 -0000
@@ -95,7 +95,7 @@
     }
   }

-  return useAFunctions;
+  return 1;//useAFunctions;
 }

Comment 50

16 years ago
I will be able to compile and test it at home.
The problem about lack of the character of an ExtTextOut API may be a problem
peculiar to Japanese windows98. A display result of a test program in the
English version windows98 is desired.

Comment 51

16 years ago
Created attachment 127950 [details]
test case for testing lack of characters

Please test whether characters lack on Windows98 of an English version.

> saito, care to try this change just to see if makes any difference...?

I checked that the shift of the string did not occur by selection about two
test cases attachment 14664 [details] and attachment 35687 [details] on Win98. However, the shift
by selection occurred in Japanese fonts. The problem which lacks a character
also occurred.

Comment 52

16 years ago
Created attachment 128017 [details] [diff] [review]
patch for Bug 212723

The function nsFontWin::nsExtTextOut was deleted, since it is same as the
function nsFontWin::nsExtTextOutA.
Attachment #127880 - Attachment is obsolete: true

Updated

16 years ago
Attachment #128017 - Flags: review?(rbs)

Comment 53

16 years ago
i'm on vacation. i'll be back around the middle of next week. i won't have a 
w32 build env for a while after that (currently residing in beos) although my 
laptop is w98...

Comment 54

16 years ago
Comment on attachment 128017 [details] [diff] [review]
patch for Bug 212723

+  aFont->mAveCharWidthMetric = metrics.tmAveCharWidth;
+  aFont->mMaxCharWidthMetric = metrics.tmMaxCharWidth;
+  aFont->mPitchAndFamilyMetric = metrics.tmPitchAndFamily;

Just call these |mAveCharWidth|, etc.

BTW, note that I still cannot review this patch. The functions that you are
modifying are precisely those that show up in the perf radars. And without a
proper justification of the extra cost (which looks excessive -- n^2), it is
hard to be convinced that patch is indeed the right way to address the problem.
Attachment #128017 - Flags: review?(rbs) → review-

Comment 55

16 years ago
Created attachment 128056 [details] [diff] [review]
patch for Bug 212723

> +  aFont->mAveCharWidthMetric = metrics.tmAveCharWidth;
> +  aFont->mMaxCharWidthMetric = metrics.tmMaxCharWidth;
> +  aFont->mPitchAndFamilyMetric = metrics.tmPitchAndFamily;
> 
> Just call these |mAveCharWidth|, etc.

What is the meaning?

I changed the function of nsFontWin::nsExtTextOutW by clipping out the string.
My all test cases are drawn successfully on Win98se.
I will ask to test this patch to Bugzilla-jp.

Comment 56

16 years ago
Created attachment 128057 [details] [diff] [review]
patch for Bug 212723

> +  aFont->mAveCharWidthMetric = metrics.tmAveCharWidth;
> +  aFont->mMaxCharWidthMetric = metrics.tmMaxCharWidth;
> +  aFont->mPitchAndFamilyMetric = metrics.tmPitchAndFamily;
> 
> Just call these |mAveCharWidth|, etc.

What is the meaning?

I changed the function of nsFontWin::nsExtTextOutW by clipping out the string.
My all test cases are drawn successfully on Win98se.
I will ask to test this patch to Bugzilla-jp.
Attachment #128017 - Attachment is obsolete: true

Updated

16 years ago
Attachment #128056 - Attachment is obsolete: true

Comment 57

16 years ago
Created attachment 128546 [details] [diff] [review]
patch for Bug 212723

rbs, can you review this patch again?
Attachment #128057 - Attachment is obsolete: true

Updated

16 years ago
Attachment #128546 - Flags: review?(rbs)
mozilla1.4 shipped. unsetting blocking1.4 request.
Flags: blocking1.4?

Comment 59

16 years ago
> > Just call these |mAveCharWidth|, etc.
>
> What is the meaning?

I mean that the other parameters next to you are not called mMaxAscentMetric,
mMaxDescentMetric, etc. So you don't need to introduce _Metric.

>I changed the function of nsFontWin::nsExtTextOutW by clipping out the string.

It is intriguing that just specifying a clipping rectangle fixes the problem.
One would think that the default (infinite) rect should have leave enough room.

Can you try a rectangle that doesn't involve re-measuring the string, e.g.,

+    clipRect.top = aY - maxAscent;
+    clipRect.bottom = aY + maxDescent; 
+    clipRect.left = aX;
+    clipRect.right = aX + maxCharAdvance * aLength;

Also, I am not yet agreeable with the fact that you space the characters
uniformly in the A variant. This means that (non-Japanese) fonts will have a
monospace appearance.

Comment 60

16 years ago
Created attachment 128698 [details] [diff] [review]
patch for Bug 212723

I changed both functions nsFontWin::nsExtTextOutA and nsFontWin::nsExtTextOutW
by clipping the string. Even if the large rectangle is specified, the problem
is avoidable. Since I wanted to distinguish from the variable of the nscoord
type, I added 'px' to the variable name instead of 'Metric'.

Updated

16 years ago
Attachment #128546 - Attachment is obsolete: true

Comment 61

16 years ago
It is looking better performance-wise, albeit it is strange that just specifying
an arbitrary ETO_CLIPPED rectangle has fixed the truncation issue... But again,
this is a work-around a GDI bug.

==================================
+nsFontWin::nsExtTextOutA(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT lpsc, LPCSTR aString, UINT aLength, INT *lpDx)
+{
+  if (uOptions == 0 &&

Don't you need a more precise |if (!(uOptions & ETO_CLIPPED))| instead? Did you
test that the problem doesn't appear when simply |uOptions != 0|? [If that was
true, the fix could have been to force uOptions to be a dummy non-zero....]

==================================
> I wanted to distinguish from the variable of the nscoord type

You are right. Please revert to _Metric and use a comment to avoid further
confusion:

// Note: these are in device units (pixels) -- not twips like the others
+  LONG            mOverhangCorrection;
+  LONG            mMaxCharWidthMetric;
+  LONG            mMaxHeightMetric;
+  BYTE            mPitchAndFamily;

==================================
+  PRBool nsExtTextOutA(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions, LPCRECT
lpsc, LPCSTR aString, UINT aLength, INT *lpDx);
+  PRBool nsExtTextOutW(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions, LPCRECT
lpsc, LPCWSTR aString, UINT aLength, INT *lpDx);

Also rename these as |PRBool ClipExtTextOutA| and |PRBool ClipExtTextOutW| so
that it is clear why they were introduced.

==================================
> I will ask to test this patch to Bugzilla-jp.

Please do, and report any finding that arises.

Comment 62

16 years ago
Created attachment 128824 [details] [diff] [review]
patch for Bug 212723

> +nsFontWin::nsExtTextOutA(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions,
> LPCRECT lpsc, LPCSTR aString, UINT aLength, INT *lpDx)
> +{
> +  if (uOptions == 0 &&
> 
> Don't you need a more precise |if (!(uOptions & ETO_CLIPPED))| instead? Did
you
> test that the problem doesn't appear when simply |uOptions != 0|? [If that
was
> true, the fix could have been to force uOptions to be a dummy non-zero....]

I changed the program so that the flags ETO_CLIPPED and ETO_OPAQUE are checked,
since a rectangle is used for clipping or opaquing.

> +  PRBool nsExtTextOutA(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT
> lpsc, LPCSTR aString, UINT aLength, INT *lpDx);
> +  PRBool nsExtTextOutW(HDC aDC, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT
> lpsc, LPCWSTR aString, UINT aLength, INT *lpDx);
> 
> Also rename these as |PRBool ClipExtTextOutA| and |PRBool ClipExtTextOutW| so

> that it is clear why they were introduced.

Since it is an unavoidable policy for avoiding a problem, I want to hide the
name of 'Clip' as much as possible. I changed again both functions
nsFontWin::nsExtTextOutA and nsFontWin::nsExtTextOutW by adding the new
function nsFontWin::ClipRectangle.  Does this change have criticism?

> > I will ask to test this patch to Bugzilla-jp.
> 
> Please do, and report any finding that arises.

I will try again to do so, if there is no other change.
Attachment #128698 - Attachment is obsolete: true

Comment 63

16 years ago
Care to change as indicated below: a helper member function |FillClipRect| that
doesn't try to do too much... and two static functions. (I think this will make
the code easier to maintain and robust/harder to break when other hackers add
their stuff... Witness the mystery bug I am having at bug 213390).

=============================
PRBool
nsFontWin::FillClipRect(PRInt32 aX, PRInt32 aY, UINT aLength, UINT uOptions,
RECT& clipRect)
{
  if (!(uOptions & (ETO_CLIPPED | ETO_OPAQUE)) &&
      mOverhangCorrection > 0 && !(mPitchAndFamily & TMPF_FIXED_PITCH)) {
    // bug 52596 - although the clipping rectangle is said to be optional, we
    // have to use a clipping rectange to work around a GDI bug on
    // Win9X-Japanese that causes the text to be truncated incorrectly.
    clipRect.top = aY - mMaxHeightMetric;
    clipRect.bottom = aY + mMaxHeightMetric;
    clipRect.left = aX;
    clipRect.right = aX + mMaxCharWidthMetric * aLength;
    return PR_TRUE;
  }
  return PR_FALSE;
}

static PRBool
NS_ExtTextOutA(HDC aDC, nsFontWin* aFont, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT lprc, LPCSTR aString, UINT aLength, INT *lpDx)
{
  RECT clipRect;
  if (!lpDx && !lprc && aFont->FillClipRect(aX, aY, aLength, uOptions, clipRect)) {
    lprc = &clipRect;
    uOptions |= ETO_CLIPPED;
  }
  return ::ExtTextOutA(aDC, aX, aY, uOptions, lprc, aString, aLength, lpDx);
}

static PRBool
NS_ExtTextOutW(HDC aDC, nsFontWin* aFont, PRInt32 aX, PRInt32 aY, UINT uOptions,
LPCRECT lprc, LPCWSTR aString, UINT aLength, INT *lpDx)
{
  RECT clipRect;
  if (!lpDx  && !lprc && aFont->FillClipRect(aX, aY, aLength, uOptions, clipRect)) {
    lprc = &clipRect;
    uOptions |= ETO_CLIPPED;
  }
  return ::ExtTextOutW(aDC, aX, aY, uOptions, lprc, aString, aLength, lpDx);
}

Comment 64

16 years ago
Created attachment 129023 [details] [diff] [review]
patch for Bug 212723
Attachment #128824 - Attachment is obsolete: true

Comment 65

16 years ago
Comment on attachment 129023 [details] [diff] [review]
patch for Bug 212723

Looks good. You can now go ahead and ask for testing at Bugzilla-jp. Once I
hear back from you, I will stamp r+sr.

Comment 66

16 years ago
Thank you for your much advice.
Although the test of this patch was requested to Bugzilla-jp, it may take time.

Comment 67

16 years ago
About a previous patch without clipping a rectangle, a exhibited building of
wazilla-1.4 was tested by Bugzilla-jp, and there is no report about this bug.
About the newest patch, although it is one test of mine as a result, the problem
was not discovered. I ask r/sr.

Updated

16 years ago
Attachment #129023 - Flags: superreview?(rbs)
Attachment #129023 - Flags: review?(rbs)

Comment 68

16 years ago
Was the latest patch (attachment 29023 [details] [diff] [review]) tested successfuly in bugzilla-jp, or
are you saying that it was tested only by you.

Comment 69

16 years ago
Only I tested. In order to test by bugzilla-jp, it is necessary to wait for the
following major release mozilla-1.5.

Comment 70

16 years ago
Comment on attachment 129023 [details] [diff] [review]
patch for Bug 212723

r+sr=rbs
Attachment #129023 - Flags: superreview?(rbs)
Attachment #129023 - Flags: superreview+
Attachment #129023 - Flags: review?(rbs)
Attachment #129023 - Flags: review+

Updated

16 years ago
Attachment #129023 - Flags: approval1.5b?
Comment on attachment 129023 [details] [diff] [review]
patch for Bug 212723

a=asa (on behalf of drivers) for checkin to Mozilla 1.5beta.
Attachment #129023 - Flags: approval1.5b? → approval1.5b+

Comment 72

16 years ago
Checked in on behalf of saito.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Blocks: 216670

Updated

16 years ago
Attachment #128546 - Flags: review?(rbs)
You need to log in before you can comment on or make changes to this bug.