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

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Hideo Saito]

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.

Comment 1

[Hideo Saito]

Attachment: testcase

Comment 2

[Hideo Saito]

Attachment: patch

Comment 3

[Simon Montagu :smontagu]

Attachment: Testcase with surrogates

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

Comment 4

[Hermann Schwab]

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

Comment 5

[Hideo Saito]

Attachment: testcase

Comment 6

[Hideo Saito]

Attachment: patch

patch for small-caps and surogates

Comment 7

[Hideo Saito]

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?

Comment 8

[Hideo Saito]

Attachment: screen shot

Comment 9

[Simon Montagu :smontagu]

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

Comment 10

[Hideo Saito]

Attachment: screen shot using Williamson's font

Montagu, thanks for your advice. I could test your testcase.

Comment 11

[rbs]

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

Comment 12

[Hideo Saito]

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

[rbs]

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;

Comment 14

[Hideo Saito]

Attachment: patch

Comment 15

[Hideo Saito]

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

[rbs]

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

Comment 17

[Hideo Saito]

Attachment: patch

I understood your indications. A "half" surrogate pair is not justifiable
character.

Comment 18

[Hideo Saito]

Attachment: difference

It is the difference between the second patch and this patch.

Comment 19

[rbs]

Comment on attachment 182365 [details] [diff] [review]
patch

r=rbs

Comment 20

[rbs]

Comment on attachment 182365 [details] [diff] [review]
patch

r+sr=rbs

Comment 21

[Hideo Saito]

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.

Comment 22

[Asa Dotzler [:asa]]

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.

Comment 23

[Mike Shaver (:shaver emeritus)]

Comment on attachment 182365 [details] [diff] [review]
patch

a=shaver

Comment 24

[rbs]

Checked in.