Last Comment Bug 688382 - :first-letter incorrectly overwrites text-transform rule
: :first-letter incorrectly overwrites text-transform rule
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Thomasy
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-22 01:30 PDT by Razor
Modified: 2012-09-07 21:16 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reporter's testcase (178 bytes, text/html)
2011-09-22 02:56 PDT, j.j.
no flags Details
Test case for more tags (1.72 KB, text/html)
2012-08-14 09:49 PDT, Thomasy
no flags Details
Test case for more tags (1.72 KB, text/html)
2012-08-14 10:14 PDT, Thomasy
no flags Details
Mulitline test cases (2.64 KB, text/html)
2012-08-14 11:36 PDT, Thomasy
no flags Details
Fix 688382 (2.53 KB, patch)
2012-08-28 13:07 PDT, Thomasy
roc: review-
Details | Diff | Splinter Review
Test case for font-size is presented (286 bytes, text/html)
2012-09-03 09:17 PDT, Thomasy
no flags Details
Patch according to comment 14 (3.65 KB, patch)
2012-09-06 14:04 PDT, Thomasy
thomas: review+
Details | Diff | Splinter Review
Patch according to comment 14 (3.03 KB, patch)
2012-09-06 14:08 PDT, Thomasy
roc: review+
Details | Diff | Splinter Review

Description Razor 2011-09-22 01:30:40 PDT
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. 2011-09-22 02:56:59 PDT
Created attachment 561691 [details]
Reporter's testcase
Comment 2 j.j. 2011-09-22 03:16:48 PDT
> Additionally, copying the text gives the original unstyled "blah blah"
That's bug 12460
Comment 3 Boris Zbarsky [:bz] 2011-09-22 06:45:02 PDT
> 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) (Exited; email my personal email if necessary) 2011-09-22 17:23:20 PDT
No...
Comment 5 Thomasy 2012-08-14 09:49:03 PDT
Created attachment 651802 [details]
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 2012-08-14 10:14:33 PDT
Created attachment 651811 [details]
Test case for more tags

Test case for more tags (fix p tag)
Comment 7 Thomasy 2012-08-14 11:36:10 PDT
Created attachment 651840 [details]
Mulitline test cases

Multiline test cases show that the bug on affect the first line.
Comment 8 Boris Zbarsky [:bz] 2012-08-14 15:56:58 PDT
> 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 2012-08-28 09:46:24 PDT
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 2012-08-28 10:46:22 PDT
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 2012-08-28 13:07:05 PDT
Created attachment 656175 [details] [diff] [review]
Fix 688382

Check capitalization using aTextRun->CanBreakLineBefore(i)
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-28 19:02:47 PDT
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 2012-09-03 09:17:50 PDT
Created attachment 657871 [details]
Test case for font-size is presented

If font-size is presented at :first-letter, the text transform work correct.
Comment 14 Thomasy 2012-09-06 13:14:10 PDT
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 2012-09-06 14:04:29 PDT
Created attachment 658993 [details] [diff] [review]
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 2012-09-06 14:08:22 PDT
Created attachment 658995 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2012-09-06 15:05:17 PDT
Comment on attachment 658995 [details] [diff] [review]
Patch according to comment 14

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

Excellent, thanks!
Comment 18 Thomasy 2012-09-07 00:17:32 PDT
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 2012-09-07 00:21:25 PDT
Help add "checkin-needed"
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-09-07 13:24:43 PDT
(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] 2012-09-07 21:16:08 PDT
https://hg.mozilla.org/mozilla-central/rev/46a0f7a74d53

Note You need to log in before you can comment on or make changes to this bug.