German character ß and surrogate pair characters can not be rendered using proper font

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: hsaito54, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

14 years ago
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

14 years ago
Created attachment 181430 [details]
testcase
(Reporter)

Comment 2

14 years ago
Created attachment 181431 [details] [diff] [review]
patch
Created attachment 181433 [details]
Testcase with surrogates

This shows a similar problem with surrogate characters which I came across
working on bug 210501.

Comment 4

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

Comment 5

14 years ago
Created attachment 181548 [details]
testcase
Attachment #181430 - Attachment is obsolete: true
(Reporter)

Comment 6

14 years ago
Created attachment 181550 [details] [diff] [review]
patch

patch for small-caps and surogates
Attachment #181431 - Attachment is obsolete: true
(Reporter)

Comment 7

14 years ago
In changes, after the previous texts were rendered using different font, &szlig
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

14 years ago
Created attachment 181605 [details]
screen shot
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

14 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

14 years ago
Created attachment 181612 [details]
screen shot using Williamson's font

Montagu, thanks for your advice. I could test your testcase.
(Reporter)

Updated

14 years ago
Attachment #181550 - Flags: review?(rbs)

Comment 11

14 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

14 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

14 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

14 years ago
Created attachment 182263 [details] [diff] [review]
patch
Attachment #181550 - Attachment is obsolete: true
Attachment #182263 - Flags: review?(rbs)
(Reporter)

Comment 15

14 years ago
Created attachment 182264 [details]
difference between two patches

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

14 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

14 years ago
Created attachment 182365 [details] [diff] [review]
patch

I understood your indications. A "half" surrogate pair is not justifiable
character.
Attachment #182264 - Attachment is obsolete: true
Attachment #182263 - Attachment is obsolete: true
Attachment #182365 - Flags: review?(rbs)
(Reporter)

Comment 18

14 years ago
Created attachment 182366 [details]
difference

It is the difference between the second patch and this patch.
(Reporter)

Updated

14 years ago
Attachment #182263 - Flags: review?(rbs)

Comment 19

14 years ago
Comment on attachment 182365 [details] [diff] [review]
patch

r=rbs
Attachment #182365 - Flags: review?(rbs) → review+
(Reporter)

Updated

14 years ago
Attachment #182365 - Flags: superreview?(rbs)

Comment 20

14 years ago
Comment on attachment 182365 [details] [diff] [review]
patch

r+sr=rbs
Attachment #182365 - Flags: superreview?(rbs) → superreview+
(Reporter)

Comment 21

14 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

14 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

14 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.