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)

x86
Windows 98
defect

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+
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.
Attached file testcase (deleted) —
setting to future. anthonyd
Target Milestone: --- → Future
*** Bug 59080 has been marked as a duplicate of this bug. ***
Lucida sans and Impact also causes this problem. Arial and Century Gothic are displayed fine. In Japanese encoding every fonts have this problem.
I can't reproduce this on WinNT.
*** Bug 61511 has been marked as a duplicate of this bug. ***
Copying top100 keyword from dup bug (happens at http://www.cnet.com/).
Keywords: top100
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
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
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?)
hmm... seems like the only thing similar for the "problem" fonts is that they are MS fonts?
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
I've seen this problem with italic MS Sans Serif. I think the problem is occuring because it is a bitmap font.
Attached file Expanded testcase - bitmap fonts (deleted) —
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.
Attached file Is this the same problem? (deleted) —
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.
*** 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
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...
Bug 115522 for a similar problem on Mac.
changing selection qa to tpreston.
QA Contact: blaker → tpreston
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.
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.
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.
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.
Attached patch patch (obsolete) (deleted) — Splinter Review
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
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
Attached patch patch (obsolete) (deleted) — Splinter Review
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
This patch is very inefficient. Cache what you want in InitMetricsFor() before getting started.
Attached patch patch (obsolete) (deleted) — Splinter Review
rbs, thanks for your advice.
Attachment #126332 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
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.
Attachment #126364 - Attachment is obsolete: true
Attached patch patch (obsolete) (deleted) — Splinter Review
There was a careless mistake that causes a compile error.
Attachment #126746 - Attachment is obsolete: true
Attachment #126905 - Flags: review?(rbs)
Depends on: 212723
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-
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.
Attached patch patch for Bug 212723 (obsolete) (deleted) — Splinter Review
Attachment #126905 - Attachment is obsolete: true
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?
Attached file a test program using cygwin (deleted) —
Attached image screen shot of test program (deleted) —
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);
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?
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; }
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.
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.
Attached patch patch for Bug 212723 (obsolete) (deleted) — Splinter Review
The function nsFontWin::nsExtTextOut was deleted, since it is same as the function nsFontWin::nsExtTextOutA.
Attachment #127880 - Attachment is obsolete: true
Attachment #128017 - Flags: review?(rbs)
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 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-
Attached patch patch for Bug 212723 (obsolete) (deleted) — Splinter Review
> + 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.
Attached patch patch for Bug 212723 (obsolete) (deleted) — Splinter Review
> + 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
Attachment #128056 - Attachment is obsolete: true
Attached patch patch for Bug 212723 (obsolete) (deleted) — Splinter Review
rbs, can you review this patch again?
Attachment #128057 - Attachment is obsolete: true
Attachment #128546 - Flags: review?(rbs)
mozilla1.4 shipped. unsetting blocking1.4 request.
Flags: blocking1.4?
> > 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.
Attached patch patch for Bug 212723 (obsolete) (deleted) — Splinter Review
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'.
Attachment #128546 - Attachment is obsolete: true
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.
Attached patch patch for Bug 212723 (obsolete) (deleted) — Splinter Review
> +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
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); }
Attached patch patch for Bug 212723 (deleted) — Splinter Review
Attachment #128824 - Attachment is obsolete: true
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.
Thank you for your much advice. Although the test of this patch was requested to Bugzilla-jp, it may take time.
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.
Attachment #129023 - Flags: superreview?(rbs)
Attachment #129023 - Flags: review?(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.
Only I tested. In order to test by bugzilla-jp, it is necessary to wait for the following major release mozilla-1.5.
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+
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+
Checked in on behalf of saito.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Blocks: 216670
Attachment #128546 - Flags: review?(rbs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: