Closed Bug 688382 Opened 13 years ago Closed 12 years ago

:first-letter incorrectly overwrites text-transform rule

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: razorwarden-mozilla, Assigned: thomas)

Details

(Keywords: testcase)

Attachments

(5 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
Build ID: 20110902133214

Steps to reproduce:

http://jsfiddle.net/Razor/acB7t/


Actual results:

Output is "Blah blah" - note how the second "b" is not capitalized.


Expected results:

Output should be "Blah Blah", just like chrome 15.0.874/IE8 does.
Additionally, copying the text gives the original unstyled "blah blah" - IE8 shows the same behaviour, while chrome copies the shown text correctly.
Attached file Reporter's testcase
Component: General → Style System (CSS)
Keywords: testcase
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → style-system
Hardware: x86 → All
Summary: :first-letter incorrectly overwrites text-trasform rule → :first-letter incorrectly overwrites text-transform rule
Version: 6 Branch → Trunk
> Additionally, copying the text gives the original unstyled "blah blah"
That's bug 12460
> That's bug 12460

It's not, actually, but is arguably correct behavior in any case.

roc, any idea what's up here?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file Test case for more tags (obsolete) —
h1-h6, div, li, p tags are affected while h7, h8, span, font are not affected.
And Chrome 21 is correct for all these cases
Attachment #651802 - Attachment mime type: text/plain → text/html
Test case for more tags (fix p tag)
Attachment #651802 - Attachment is obsolete: true
Attachment #651811 - Attachment mime type: text/plain → text/html
Attached file Mulitline test cases
Multiline test cases show that the bug on affect the first line.
Attachment #651840 - Attachment mime type: text/plain → text/html
> h1-h6, div, li, p tags are affected while h7, h8, span, font are not affected.

h7, h8, span, and font are all inlines, and hence :first-letter doesn't apply to them at all.
Go depth into reporter's test case

The text frame "blah blah"

http://hg.mozilla.org/mozilla-central/annotate/992588c2eab6/layout/generic/nsTextRunTransformations.cpp#l543

I dump some variable for debug

Text frame "blah blah"
style is   "UCCCCCCCC"
where U is NS_STYLE_TEXT_TRANSFORM_UPPERCASE and C is NS_STYLE_TEXT_TRANSFORM_CAPITALIZE


value of aTextRun->mCapitalize[i] 
Text frame "blah blah"
value is   " 11111011"

mCapitalize is modify by SetCapitalization but the text frame do not call that function

and the aTextRun->mCapitalize.Length() is 0

while using my testcase the result is similar to the above but the second line is correct
I think it can refer to 

This change-set

http://hg.mozilla.org/mozilla-central/rev/be6c0d9a897f

and bug 389707 for more clues
Attached patch Fix 688382 (obsolete) — Splinter Review
Check capitalization using aTextRun->CanBreakLineBefore(i)
Attachment #656175 - Flags: review?(dbaron)
Attachment #656175 - Flags: review?(dbaron) → review?(roc)
Comment on attachment 656175 [details] [diff] [review]
Fix 688382

Review of attachment 656175 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextRunTransformations.cpp
@@ +845,5 @@
>          capitalizeDutchIJ = false;
>          break;
>        }
>        capitalizeDutchIJ = false;
> +      if ((i < aTextRun->mCapitalize.Length() && aTextRun->mCapitalize[i]) || aTextRun->CanBreakLineBefore(i)) {

I don't think this is right. mCapitalize should have the correct value here. If it doesn't, we need to figure out why not and fix that.
Attachment #656175 - Flags: review?(roc) → review-
If font-size is presented at :first-letter, the text transform work correct.
Attachment #656175 - Attachment is obsolete: true
Attachment #657871 - Attachment mime type: text/plain → text/html
I  figure out that it is because 

The first-letter frame will be merge if it is  "ContinueTextRunAcrossFrames"

http://hg.mozilla.org/mozilla-central/annotate/ca630290d89d/layout/generic/nsTextFrameThebes.cpp#l1580

>    if (mLastFrame) {
>      if (!ContinueTextRunAcrossFrames(mLastFrame, frame)) {
>        FlushFrames(false, false);
>      } else {
>        if (mLastFrame->GetContent() == frame->GetContent()) {
>          AccumulateRunInfo(frame);
>          return;
>        }
>      }
>    }


/layout/generic/nsTextFrameThebes.cpp will check font, so font is correct for the case above

http://hg.mozilla.org/mozilla-central/annotate/ca630290d89d/layout/generic/nsTextFrameThebes.cpp#l1547

> return fontStyle1->mFont.BaseEquals(fontStyle2->mFont) &&
> sc1->GetStyleFont()->mLanguage == sc2->GetStyleFont()->mLanguage &&
> nsLayoutUtils::GetTextRunFlagsForStyle(sc1, fontStyle1, letterSpacing1) ==
> nsLayoutUtils::GetTextRunFlagsForStyle(sc2, fontStyle2, letterSpacing2);
>} 

Add some logic near layout/generic/nsTextFrameThebes.cpp#l1547 can fix this issue.
And I think I can take this bug.
Attached patch Patch according to comment 14 (obsolete) — Splinter Review
Add some logic near layout/generic/nsTextFrameThebes.cpp#l1547 for text-transform checking. Avoid first-letter text-frame merge with the rest text-frame
Attachment #658993 - Flags: review+
Attachment #658993 - Attachment is obsolete: true
Patch according to comment 14

Add some logic near layout/generic/nsTextFrameThebes.cpp#l1547 for text-transform checking. Avoid first-letter text-frame merge with the rest text-frame
Attachment #658995 - Flags: review?(roc)
Comment on attachment 658995 [details] [diff] [review]
Patch according to comment 14

Review of attachment 658995 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks!
Attachment #658995 - Flags: review?(roc) → review+
Help add "checkin-needed"
Assignee: nobody → thomas
Status: NEW → ASSIGNED
Keywords: checkin-needed
(In reply to Thomasy from comment #18)
> https://tbpl.mozilla.org/?tree=Try&rev=54e273ae092c

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/46a0f7a74d53

Thanks for the patch, Thomasy! One request - to make life easier for those checking in on your behalf, please make sure that your future patches include all the necessary metadata needed for checkin per the directions below. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/46a0f7a74d53
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.