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

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P3)

Description

[Jesse Ruderman]

[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.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[anthonyd]

setting to future.

anthonyd

Comment 3

[Koike Kazuhiko]

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

Comment 4

[Koike Kazuhiko]

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

[Koike Kazuhiko]

I can't reproduce this on WinNT.

Comment 6

[Jesse Ruderman]

*** Bug 61511 has been marked as a duplicate of this bug. ***

Comment 7

[Jesse Ruderman]

Copying top100 keyword from dup bug (happens at http://www.cnet.com/).

Comment 8

[Jesse Ruderman]

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

Comment 9

[James Lariviere]

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...

Comment 10

[Jesse Ruderman]

Also happens with at least one serif font (ms serif).

Comment 11

[James Lariviere]

hmm... seems like the only thing similar for the "problem" fonts is that they
are MS fonts?

Comment 12

[Jesse Ruderman]

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

[Skewer]

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

Comment 14

[Skewer]

Attachment: Expanded testcase - bitmap fonts

Comment 15

[Skewer]

Attachment: Image demonstrating undesirable behavior

Comment 16

[jag (Peter Annema)]

It sounds like this bug and bug 91485 share the same underlying problem.

Comment 17

[[e-mail]]

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.

Comment 18

[[e-mail]]

Attachment: Is this the same problem?

Comment 19

[James Lariviere]

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.

Comment 20

[mjudge]

*** Bug 91485 has been marked as a duplicate of this bug. ***

Comment 21

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

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

[rbs]

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

Comment 23

[[e-mail]]

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

[Koike Kazuhiko]

Bug 115522 for a similar problem on Mac.

Comment 25

[Blake Ross]

changing selection qa to tpreston.

Comment 26

[Skewer]

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?

Comment 27

[[e-mail]]

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

[Skewer]

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.

Comment 29

[[e-mail]]

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

[rainer canavan]

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

[Hideo Saito]

Attachment: The program which gets the value of tmOverhang

Comment 32

[Hideo Saito]

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

[Hideo Saito]

Attachment: patch for subtract an overhang value from the string length

Comment 34

[Hideo Saito]

Attachment: 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

Comment 35

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

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 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Hideo Saito]

Attachment: 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.

Comment 38

[rbs]

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

Comment 39

[Hideo Saito]

Attachment: patch

rbs, thanks for your advice.

Comment 40

[Hideo Saito]

Attachment: 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.

Comment 41

[Hideo Saito]

Attachment: patch

There was a careless mistake that causes a compile error.

Comment 42

[rbs]

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.]

Comment 43

[Hideo Saito]

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

[Hideo Saito]

Attachment: patch for Bug 212723

Comment 45

[rbs]

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

[Hideo Saito]

Attachment: a test program using cygwin

Comment 47

[Hideo Saito]

Attachment: 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

[rbs]

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

[rbs]

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

[Hideo Saito]

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

[Hideo Saito]

Attachment: 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

[Hideo Saito]

Attachment: patch for Bug 212723

The function nsFontWin::nsExtTextOut was deleted, since it is same as the
function nsFontWin::nsExtTextOutA.

Comment 53

[timeless]

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

[rbs]

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.

Comment 55

[Hideo Saito]

Attachment: 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

[Hideo Saito]

Attachment: 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 57

[Hideo Saito]

Attachment: patch for Bug 212723

rbs, can you review this patch again?

Comment 58

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.

Comment 59

[rbs]

> > 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

[Hideo Saito]

Attachment: 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'.

Comment 61

[rbs]

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

[Hideo Saito]

Attachment: 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.

Comment 63

[rbs]

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

[Hideo Saito]

Attachment: patch for Bug 212723

Comment 65

[rbs]

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

[Hideo Saito]

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

Comment 67

[Hideo Saito]

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.

Comment 68

[rbs]

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

[Hideo Saito]

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

[rbs]

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

r+sr=rbs

Comment 71

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to Mozilla 1.5beta.

Comment 72

[rbs]

Checked in on behalf of saito.