Closed
Bug 52596
Opened 24 years ago
Closed 21 years ago
for several fonts, bold text expands when selected (mostly sans serif fonts?)
Categories
(Core :: DOM: Selection, defect, P3)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: jruderman, Assigned: mjudge)
References
()
Details
(Keywords: testcase, top100)
Attachments
(9 files, 13 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
rbs
:
review+
rbs
:
superreview+
asa
:
approval1.5b+
|
Details | Diff | Splinter Review |
[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•24 years ago
|
||
Comment 4•24 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•24 years ago
|
||
I can't reproduce this on WinNT.
Reporter | ||
Comment 7•24 years ago
|
||
Copying top100 keyword from dup bug (happens at http://www.cnet.com/).
Keywords: top100
Reporter | ||
Comment 8•24 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•24 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...
URL: http://www.cnet.com
Severity: minor → normal
Reporter | ||
Comment 10•24 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•24 years ago
|
||
hmm... seems like the only thing similar for the "problem" fonts is that they
are MS fonts?
Reporter | ||
Comment 12•24 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•23 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•23 years ago
|
||
Comment 15•23 years ago
|
||
Comment 16•23 years ago
|
||
It sounds like this bug and bug 91485 share the same underlying problem.
Comment 17•23 years ago
|
||
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•23 years ago
|
||
Comment 19•23 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•23 years ago
|
||
*** Bug 91485 has been marked as a duplicate of this bug. ***
Comment 21•23 years ago
|
||
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•23 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
Comment 23•23 years ago
|
||
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•23 years ago
|
||
Bug 115522 for a similar problem on Mac.
Comment 26•23 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?
Comment 27•23 years ago
|
||
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•23 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.
Comment 29•23 years ago
|
||
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•23 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•22 years ago
|
||
Comment 32•22 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•22 years ago
|
||
Comment 34•21 years ago
|
||
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•21 years ago
|
Flags: blocking1.4?
Comment 35•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
This patch is very inefficient. Cache what you want in InitMetricsFor() before
getting started.
Comment 39•21 years ago
|
||
rbs, thanks for your advice.
Attachment #126332 -
Attachment is obsolete: true
Comment 40•21 years ago
|
||
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•21 years ago
|
Attachment #126364 -
Attachment is obsolete: true
Comment 41•21 years ago
|
||
There was a careless mistake that causes a compile error.
Attachment #126746 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #126905 -
Flags: review?(rbs)
Comment 42•21 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•21 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•21 years ago
|
||
Attachment #126905 -
Attachment is obsolete: true
Comment 45•21 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•21 years ago
|
||
Comment 47•21 years ago
|
||
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•21 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•21 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•21 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•21 years ago
|
||
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•21 years ago
|
||
The function nsFontWin::nsExtTextOut was deleted, since it is same as the
function nsFontWin::nsExtTextOutA.
Attachment #127880 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #128017 -
Flags: review?(rbs)
Comment 53•21 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•21 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•21 years ago
|
||
> + 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•21 years ago
|
||
> + 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•21 years ago
|
Attachment #128056 -
Attachment is obsolete: true
Comment 57•21 years ago
|
||
rbs, can you review this patch again?
Attachment #128057 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #128546 -
Flags: review?(rbs)
Comment 59•21 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•21 years ago
|
||
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•21 years ago
|
Attachment #128546 -
Attachment is obsolete: true
Comment 61•21 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•21 years ago
|
||
> +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•21 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•21 years ago
|
||
Attachment #128824 -
Attachment is obsolete: true
Comment 65•21 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•21 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•21 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•21 years ago
|
Attachment #129023 -
Flags: superreview?(rbs)
Attachment #129023 -
Flags: review?(rbs)
Comment 68•21 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•21 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•21 years ago
|
||
Attachment #129023 -
Flags: superreview?(rbs)
Attachment #129023 -
Flags: superreview+
Attachment #129023 -
Flags: review?(rbs)
Attachment #129023 -
Flags: review+
Updated•21 years ago
|
Attachment #129023 -
Flags: approval1.5b?
Comment 71•21 years ago
|
||
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•21 years ago
|
||
Checked in on behalf of saito.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Attachment #128546 -
Flags: review?(rbs)
You need to log in
before you can comment on or make changes to this bug.
Description
•