Closed
Bug 291321
Opened 19 years ago
Closed 19 years ago
German character ß and surrogate pair characters can not be rendered using proper font
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
People
(Reporter: hsaito54, Unassigned)
References
Details
Attachments
(6 files, 5 obsolete files)
335 bytes,
text/html
|
Details | |
536 bytes,
text/html
|
Details | |
26.19 KB,
image/jpeg
|
Details | |
12.95 KB,
image/jpeg
|
Details | |
8.07 KB,
patch
|
rbs
:
review+
rbs
:
superreview+
asa
:
approval1.8b2-
shaver
:
approval1.8b3+
|
Details | Diff | Splinter Review |
5.35 KB,
text/plain
|
Details |
When the text style is small-caps and two 'S' are rendered for ß, first 'S' is not rendered using small-caps font. However, the case of ß follows lower-case character or it is selected, 'S' is rendered using small-caps font. I think that first 'S' should be always rendered using small-caps font.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Comment 3•19 years ago
|
||
This shows a similar problem with surrogate characters which I came across working on bug 210501.
Comment 4•19 years ago
|
||
*** Bug 291323 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 5•19 years ago
|
||
Attachment #181430 -
Attachment is obsolete: true
Reporter | ||
Comment 6•19 years ago
|
||
patch for small-caps and surogates
Attachment #181431 -
Attachment is obsolete: true
Reporter | ||
Comment 7•19 years ago
|
||
In changes, after the previous texts were rendered using different font, ß and surrogates are rendered. The changes for GetTextDimensionsOrLength prevent the selected surrogates from moving. Montagu, could you test this patch as to surrogates?
Reporter | ||
Comment 8•19 years ago
|
||
Comment 9•19 years ago
|
||
The patch seems to be working well. By the way, I am using Mark Williamson's Andagii font for testing: http://www.i18nguy.com/unicode/unicode-font.html
Reporter | ||
Updated•19 years ago
|
Summary: German character ß can not be rendered using small-caps font → German character ß and surrogate pair characters can not be rendered using proper font
Reporter | ||
Comment 10•19 years ago
|
||
Montagu, thanks for your advice. I could test your testcase.
Reporter | ||
Updated•19 years ago
|
Attachment #181550 -
Flags: review?(rbs)
Comment 11•19 years ago
|
||
Again... Care to explain how your patch works, and how it differs from what the old code used do. In particular, why you had to duplicate this |if| rather than trying to group things as the old code was doing. if (aTextStyle.mSmallCaps && (IsLowerCase(ch) || (ch == kSZLIG))) { ...
Reporter | ||
Comment 12•19 years ago
|
||
In the original code, both first character 'S' of &szilg; and first character of surrogate pair are rendered using last font. However, the last font is unfixed, since it is dependent on last processing. In changes, both first characters are rendered using proper font(that is next font) after the previous texts were rendered using different font. First following |if| decides next font, and second |if| renders them. if (aTextStyle.mSmallCaps && (IsLowerCase(ch) || (ch == kSZLIG))) { nextFont = aTextStyle.mSmallFont;
Comment 13•19 years ago
|
||
Comment on attachment 181550 [details] [diff] [review] patch I applied your patch to see a bit more clearly what was going on. I suggest the following simplifications to make it more readable/understandable: --- nsTextFrame-saito.cpp 2005-04-29 13:19:48.000000000 +1000 +++ nsTextFrame.cpp 2005-04-30 07:27:11.640625000 +1000 @@ -2834,7 +2834,7 @@ aRenderingContext.GetColor(textColor); for (; --aLength >= 0; aBuffer++) { nsIFontMetrics* nextFont; - nscoord glyphWidth; + nscoord glyphWidth = 0; PRUnichar ch = *aBuffer; if (aTextStyle.mSmallCaps && (IsLowerCase(ch) || (ch == kSZLIG))) { @@ -2843,15 +2843,6 @@ else { nextFont = aTextStyle.mNormalFont; } - glyphWidth = 0; - if (justifying && (!isEndOfLine || aLength > 0) - && IsJustifiableCharacter(ch, isCJ)) { - glyphWidth += aTextStyle.mExtraSpacePerJustifiableCharacter; - if ((PRUint32)--aTextStyle.mNumJustifiableCharacterToRender - < (PRUint32)aTextStyle.mNumJustifiableCharacterReceivingExtraJot) { - glyphWidth++; - } - } if (nextFont != lastFont) { pendingCount = bp - runStart; if (0 != pendingCount) { @@ -2877,8 +2868,7 @@ aRenderingContext.SetFont(nextFont); lastFont = nextFont; } - if (aTextStyle.mSmallCaps && - (IsLowerCase(ch) || (ch == kSZLIG))) { + if (nextFont == aTextStyle.mSmallFont) { PRUnichar upper_ch; // German szlig should be expanded to "SS". if (ch == kSZLIG) @@ -2886,15 +2876,14 @@ else upper_ch = ToUpperCase(ch); aRenderingContext.GetWidth(upper_ch, charWidth); + glyphWidth += charWidth + aTextStyle.mLetterSpacing; if (ch == kSZLIG) //add an additional 'S' here. { - nscoord glyphWidth = charWidth + aTextStyle.mLetterSpacing; *bp++ = upper_ch; if (spacing) *sp++ = glyphWidth; width += glyphWidth; } - glyphWidth += charWidth + aTextStyle.mLetterSpacing; ch = upper_ch; } else if (ch == ' ') { @@ -2921,6 +2910,14 @@ aRenderingContext.GetWidth(ch, charWidth); glyphWidth += charWidth + aTextStyle.mLetterSpacing; } + if (justifying && (!isEndOfLine || aLength > 0) + && IsJustifiableCharacter(ch, isCJ)) { + glyphWidth += aTextStyle.mExtraSpacePerJustifiableCharacter; + if ((PRUint32)--aTextStyle.mNumJustifiableCharacterToRender + < (PRUint32)aTextStyle.mNumJustifiableCharacterReceivingExtraJot) { + glyphWidth++; + } + } *bp++ = ch; if (spacing) *sp++ = glyphWidth;
Attachment #181550 -
Flags: review?(rbs) → review-
Reporter | ||
Comment 14•19 years ago
|
||
Attachment #181550 -
Attachment is obsolete: true
Attachment #182263 -
Flags: review?(rbs)
Reporter | ||
Comment 15•19 years ago
|
||
I think that increase of |glyphWidth| for justifying should be calculated before calculating space for surrogate pair, since second character's |glyphWidth| is set to zero. In changes for |GetTextDimensionsOrLength|, I add |prevLength|, since first surrogated character can not selected.
Comment 16•19 years ago
|
||
> I think that increase of |glyphWidth| for justifying should be calculated > before calculating space for surrogate pair, since second character's > |glyphWidth| is set to zero. Is this a possible scenario? Can IsJustifiableCharacter(ch) be true for |ch|, i.e., a "half" surrogate pair (re: IS_HIGH_SURROGATE(ch) && aLength > 0 && IS_LOW_SURROGATE(*(aBuffer+1))? nsTextFrame is already way too complex as it is. Further complications tend to pile up over time. Please submit a simpler patch reorganized per my previous comment -- unless the surrogate change is a real issue -- which you should then explain more and include a testcase. > In changes for |GetTextDimensionsOrLength|, I add |prevLength|, since first > surrogated character can not selected. OK.
Reporter | ||
Comment 17•19 years ago
|
||
I understood your indications. A "half" surrogate pair is not justifiable character.
Attachment #182263 -
Attachment is obsolete: true
Attachment #182264 -
Attachment is obsolete: true
Attachment #182365 -
Flags: review?(rbs)
Reporter | ||
Comment 18•19 years ago
|
||
It is the difference between the second patch and this patch.
Reporter | ||
Updated•19 years ago
|
Attachment #182263 -
Flags: review?(rbs)
Comment 19•19 years ago
|
||
Comment on attachment 182365 [details] [diff] [review] patch r=rbs
Attachment #182365 -
Flags: review?(rbs) → review+
Reporter | ||
Updated•19 years ago
|
Attachment #182365 -
Flags: superreview?(rbs)
Comment 20•19 years ago
|
||
Comment on attachment 182365 [details] [diff] [review] patch r+sr=rbs
Attachment #182365 -
Flags: superreview?(rbs) → superreview+
Reporter | ||
Comment 21•19 years ago
|
||
Comment on attachment 182365 [details] [diff] [review] patch A fault which displays a pair of characters using a different font was fixed. I think that the patch is low-risk and the changes cause the source code to be more readable.
Attachment #182365 -
Flags: approval1.8b2?
Comment 22•19 years ago
|
||
Comment on attachment 182365 [details] [diff] [review] patch let's push this out until the next release. we're down to endgame on 1.8b2 and shouldn't be taking non-blockers at this point.
Attachment #182365 -
Flags: approval1.8b3?
Attachment #182365 -
Flags: approval1.8b2?
Attachment #182365 -
Flags: approval1.8b2-
Comment on attachment 182365 [details] [diff] [review] patch a=shaver
Attachment #182365 -
Flags: approval1.8b3? → approval1.8b3+
Comment 24•19 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•