Closed Bug 354451 Opened 13 years ago Closed 13 years ago

reprise bug 96423 : german character ß - buggy capitalization (text-transform: uppercase)

Categories

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

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: antje.lehmann, Assigned: roc)

References

Details

(4 keywords)

Attachments

(2 files, 2 obsolete files)

testcase
344 bytes, text/html
Details
more comprehensive fix
22.61 KB, patch
smontagu
: review+
rbs
: superreview+
dveditz
: approval1.8.0.9+
dveditz
: approval1.8.1.1+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7

with the new update 1.5.0.7:
If you use "text-transform: uppercase;" on an element holding text which contains "ß", that letter transforms to "SS" (correct), but than, under certain circumstances (has to be one word or the second of two), the last letter is missing.

firefox 1.5.0.6 still was ok

Reproducible: Always

Steps to Reproduce:
<html>
<head><title>TextTransformer: Capitalize Bug</title></head>
<body>
  <span style="text-transform: uppercase;">Soße</span><br>
  <span style="text-transform: uppercase;">Soße </span>
</body>
</html>

Actual Results:  
the result in both cases should be SOSSE. In the buggy first case the last letter 'E' is missing
Regression from bug 345071
Blocks: 345071
Component: General → Layout: Fonts and Text
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Hardware: PC → All
Version: unspecified → 1.0 Branch
Attached file testcase (obsolete) —
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Works on trunk, right? sigh
(In reply to comment #3)
> Works on trunk, right? sigh

No, not for me with current trunk build on windows.

I should really have noticed this when reviewing bug 345071. There's even a great fat //XXX comment saying "doesn't support the case where the first-letter expands, e.g., with text-transform:capitalize, the German szlig; becomes SS."

My thinking was that since the comment was already there, we weren't making things worse than the status quo, but the status quo only applied to first-letter frames, and I don't think that ß ever occurs at the beginning of a word.
Version: 1.0 Branch → Trunk
Attached file testcase — 
This testcase works better for me. I think there's a charset problem when viewing the previous testcase directly in bugzilla.
Attachment #240306 - Attachment is obsolete: true
Attached patch fix (obsolete) — — Splinter Review 
Okay. This fixes the problem a better way. What we really want to do is to limit the amount of input text that the texttransformer looks at to the text that's mapped by this frame. It turns out that texttransformer is already set up to do that for bidi reasons. We just want to do that all the time.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #240407 - Flags: superreview?
Attachment #240407 - Flags: review?(smontagu)
Attachment #240407 - Flags: superreview? → superreview?(rbs)
The new patch shouldn't regress bug 291176 either. Does it?
It doesn't regress 291176.
Comment on attachment 240407 [details] [diff] [review]
fix

If you do this you're going to need to restore the original special casing for first-letter.
Attachment #240407 - Flags: review?(smontagu) → review-
Why? Do you have a testcase that it breaks?

For first-letter this should work the same as for other situations where the frame maps part of a word; we're telling nsTextTransformer only to look at the content actually mapped by the frame.
Ah. nsTextTransformer does not apply that bound consistently. I'll fix that.
Attachment #240407 - Attachment is obsolete: true
Attachment #240544 - Flags: superreview?(rbs)
Attachment #240544 - Flags: review?(smontagu)
Attachment #240407 - Flags: superreview?(rbs)
Comment on attachment 240544 [details] [diff] [review]
more comprehensive fix

With the patch assuming the #ifdef IBMBIDI below, you might as well undef it:

PRUnichar*
nsTextTransformer::GetNextWord(PRBool aInWord,
                               PRInt32* aWordLenResult,
                               PRInt32* aContentLenResult,
                               PRBool* aIsWhiteSpaceResult,
                               PRBool* aWasTransformed,
                               PRBool aResetTransformBuf,
                               PRBool aForLineBreak,
                               PRBool aIsKeyboardSelect)
{
  const nsTextFragment* frag = mFrag;
  PRInt32 fragLen = frag->GetLength();
#ifdef IBMBIDI
  if (*aWordLenResult > 0 && *aWordLenResult < fragLen) {
    fragLen = *aWordLenResult;
  }
#endif
=============

Also, would be safer to catch bad changes and ease maintenance (down the track as we know with this picky text frame code), and a way of documentation to
replace those:

-  PRInt32 fragLen = frag->GetLength();

whith NS_ASSERTION(aFragLen<=frag->GetLength())
I'll remove the bidi #ifdefs, but I'm killing nsTextTransformer on trunk so I'll skip the assertions if that's OK.
Comment on attachment 240544 [details] [diff] [review]
more comprehensive fix

sr=rbs, looks ok to me.
Attachment #240544 - Flags: superreview?(rbs) → superreview+
Attachment #240544 - Flags: review?(smontagu) → review+
Checked in.

We should probably take this on branches to fix regressions from bug 345071, which landed there. But we can wait for a while for things to bake on trunk first. Someone remind me :-)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This seems to have regressed Tp on btek. I can't really see how that could happen :-(
Hmm, after more cycles, it appears to not have been a real regression.
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Do we need a different or merged patch for the 1.8.0/1.8 branches?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
This patch should pretty much work.
Attachment #240544 - Flags: approval1.8.1.1?
Attachment #240544 - Flags: approval1.8.0.9?
Comment on attachment 240544 [details] [diff] [review]
more comprehensive fix

approved for 1.8/1.8.0 branches, a=dveditz for drivers. Please land soon.
Attachment #240544 - Flags: approval1.8.1.1?
Attachment #240544 - Flags: approval1.8.1.1+
Attachment #240544 - Flags: approval1.8.0.9?
Attachment #240544 - Flags: approval1.8.0.9+
Verified fixed on 1.8.0.9/ 1.8.1.1 with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.1pre) Gecko/20061129 BonEcho/2.0.0.1pre
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.9pre) Gecko/20061130 Firefox/1.5.0.9pre also Vista and Fedora FC 6
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
You need to log in before you can comment on or make changes to this bug.