688382 - :first-letter incorrectly overwrites text-transform rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Razor]

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.

Comment 1

[j.j.]

Attachment: Reporter's testcase

Comment 2

[j.j.]

> Additionally, copying the text gives the original unstyled "blah blah"
That's bug 12460

Comment 3

[Boris Zbarsky [:bzbarsky]]

> That's bug 12460

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

roc, any idea what's up here?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No...

Comment 5

[Thomasy]

Attachment: Test case for more tags

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

Comment 6

[Thomasy]

Attachment: Test case for more tags

Test case for more tags (fix p tag)

Comment 7

[Thomasy]

Attachment: Mulitline test cases

Multiline test cases show that the bug on affect the first line.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Thomasy]

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

Comment 10

[Thomasy]

I think it can refer to 

This change-set

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

and bug 389707 for more clues

Comment 11

[Thomasy]

Attachment: Fix 688382

Check capitalization using aTextRun->CanBreakLineBefore(i)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[Thomasy]

Attachment: Test case for font-size is presented

If font-size is presented at :first-letter, the text transform work correct.

Comment 14

[Thomasy]

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.

Comment 15

[Thomasy]

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

Comment 16

[Thomasy]

Attachment: Patch according to comment 14

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 658995 [details] [diff] [review]
Patch according to comment 14

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

Excellent, thanks!

Comment 18

[Thomasy]

try server change-set

http://hg.mozilla.org/try/rev/54e273ae092c

try server result

https://tbpl.mozilla.org/?tree=Try&rev=54e273ae092c

Comment 19

[Thomasy]

Help add "checkin-needed"

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46a0f7a74d53