Closed
Bug 354451
Opened 15 years ago
Closed 15 years ago
reprise bug 96423 : german character ß - buggy capitalization (text-transform: uppercase)
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
VERIFIED
FIXED
People
(Reporter: antje.lehmann, Assigned: roc)
References
Details
(4 keywords)
Attachments
(2 files, 2 obsolete files)
344 bytes,
text/html
|
Details | |
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
Comment 1•15 years ago
|
||
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
Comment 2•15 years ago
|
||
Updated•15 years ago
|
Assignee | ||
Comment 3•15 years ago
|
||
Works on trunk, right? sigh
Comment 4•15 years ago
|
||
(In reply to comment #3) > Works on trunk, right? sigh No, not for me with current trunk build on windows.
Comment 5•15 years ago
|
||
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.
Updated•15 years ago
|
Version: 1.0 Branch → Trunk
Assignee | ||
Comment 6•15 years ago
|
||
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
Assignee | ||
Comment 7•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #240407 -
Flags: superreview? → superreview?(rbs)
The new patch shouldn't regress bug 291176 either. Does it?
Assignee | ||
Comment 9•15 years ago
|
||
It doesn't regress 291176.
Comment 10•15 years ago
|
||
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-
Assignee | ||
Comment 11•15 years ago
|
||
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.
Comment 12•15 years ago
|
||
(In reply to comment #11) > Why? Do you have a testcase that it breaks? http://www.w3.org/Style/CSS/Test/CSS2.1/current/t051202-c24-first-lttr-00-b.xht
Assignee | ||
Comment 13•15 years ago
|
||
Ah. nsTextTransformer does not apply that bound consistently. I'll fix that.
Assignee | ||
Comment 14•15 years ago
|
||
Attachment #240407 -
Attachment is obsolete: true
Attachment #240544 -
Flags: superreview?(rbs)
Attachment #240544 -
Flags: review?(smontagu)
Attachment #240407 -
Flags: superreview?(rbs)
Comment 15•15 years ago
|
||
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())
Assignee | ||
Comment 16•15 years ago
|
||
I'll remove the bidi #ifdefs, but I'm killing nsTextTransformer on trunk so I'll skip the assertions if that's OK.
Comment 17•15 years ago
|
||
Comment on attachment 240544 [details] [diff] [review] more comprehensive fix sr=rbs, looks ok to me.
Attachment #240544 -
Flags: superreview?(rbs) → superreview+
Updated•15 years ago
|
Attachment #240544 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 18•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•15 years ago
|
||
This seems to have regressed Tp on btek. I can't really see how that could happen :-(
Assignee | ||
Comment 20•15 years ago
|
||
Hmm, after more cycles, it appears to not have been a real regression.
Updated•15 years ago
|
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Comment 21•14 years ago
|
||
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+
Assignee | ||
Comment 22•14 years ago
|
||
This patch should pretty much work.
Assignee | ||
Updated•14 years ago
|
Attachment #240544 -
Flags: approval1.8.1.1?
Attachment #240544 -
Flags: approval1.8.0.9?
Comment 23•14 years ago
|
||
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+
Updated•14 years ago
|
Keywords: fixed1.8.1 → fixed1.8.1.1
Comment 25•14 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•