The default bug view has changed. See this FAQ.

:first-letter incorrectly overwrites text-transform rule

RESOLVED FIXED in mozilla18

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Razor, Assigned: Thomasy)

Tracking

({testcase})

Trunk
mozilla18
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
Created attachment 561691 [details]
Reporter's testcase

Updated

6 years ago
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

Comment 2

6 years ago
> 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
No...
(Assignee)

Comment 5

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #651802 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 6

5 years ago
Created attachment 651811 [details]
Test case for more tags

Test case for more tags (fix p tag)
Attachment #651802 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #651811 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 7

5 years ago
Created attachment 651840 [details]
Mulitline test cases

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

Updated

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
I think it can refer to 

This change-set

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

and bug 389707 for more clues
(Assignee)

Comment 11

5 years ago
Created attachment 656175 [details] [diff] [review]
Fix 688382

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-
(Assignee)

Comment 13

5 years ago
Created attachment 657871 [details]
Test case for font-size is presented

If font-size is presented at :first-letter, the text transform work correct.
Attachment #656175 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #657871 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
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
Attachment #658993 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #658993 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
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
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+
(Assignee)

Comment 18

5 years ago
try server change-set

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

try server result

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

Comment 19

5 years ago
Help add "checkin-needed"

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.