Closed Bug 431341 Opened 12 years ago Closed 11 years ago

use of the :first-letter selector causes text-transform:capitalize to capitalize the second letter too

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: zwol, Assigned: roc)

Details

(Keywords: regression, testcase, verified1.9.0.9)

Attachments

(6 files, 2 obsolete files)

Attached file test case
If you have a style sheet which applies text-transform:capitalize to an element and also applies some styling - it doesn't matter what - to the :first-letter pseudo-descendant of that element, then both the first and second letters of the first word in the element will be capitalized.

That's a bit confusing, I know, so have a look at the attached test case.  In both paragraphs, the first word reads "THe" when it should be "The".  (reference rendering to follow.)
Attached file reference rendering
Is this a regression from new text frame?  Or was it broken before, too (just with different code doing the breaking)?  Does the text run need to be told that it's not the beginning of the word?
FF2 does not display this bug, which I _assume_ means it's a new-text-frame regression.  (Note that FF2 gets the test case wrong, just in a different way.)

The code that's making the decisions here is nsLineBreaker::FlushCurrentWord() and its subroutine SetupCapitalization().  I suspect that FlushCurrentWord() actually has enough information to know when it's looking at only part of a word, but I got stuck trying to figure out when the various BREAK_* flags are set.
Flags: wanted1.9.1?
Priority: -- → P2
On further investigation, nsLineBreaker does *not* have sufficient information to know when it's looking at only part of a word.  In the test case, AppendText() gets called (for both paragraphs) with a long string that begins at the second letter of "the", and there's nothing to distinguish it in the flags.

The caller here is BuildTextRunScanner::SetupBreakSinksForTextRun, which *ought* to know, but I don't immediately see whether it *does*, and I have run out of time to spend on this bug right now.
Flags: wanted1.9.1? → wanted1.9.1+
This only happens with a floating first-letter, I assume. So yeah, your analysis is right.

The thing is that BuildTextRunScanner scans the line boxes of the block. But the text in the first-letter isn't in the line boxes. I suppose it should descend through placeholders that are first-letter floats.
I thought it might happen with unusual settings of display: or position: as well, but on casual testing I can't make it happen without float:  -- possibly because the other properties are just ignored without the float:, I dunno.
Attached patch fix (obsolete) — Splinter Review
Descending into the first-letter float works, but we have to be careful about how we do it.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #330848 - Flags: superreview?(smontagu)
Attachment #330848 - Flags: review?(smontagu)
Attachment #330848 - Flags: superreview?(smontagu)
Do I understand correctly that your revised CanTextRunCrossFrameBoundary is relying on ->CanContinueTextRun() to distinguish first-letter placeholders from other placeholders?
Yes, I admit that is dirty.
I was trying to work out a patch when I saw yours, and it pretty handily demonstrates that I don't know this area of the code yet, but it seems like a reasonable approach to me.  That function's logic is convoluted but I couldn't find a way to write it that was clearer.
Comment on attachment 330848 [details] [diff] [review]
fix

>+struct TextRunFrameTraversal {
>+  nsIFrame*    mDescendIntoFrame;
>+  PRPackedBool mDescendIntoFrameSiblings;

I find this naming makes the code confusing, since it looks as if these are two things on the same footing. Can you call the frame mFrameToDescendInto or something?

r=me apart from that.
Attachment #330848 - Flags: review?(smontagu) → review+
Pushed 2d36444242f2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: regression, testcase
Resolution: --- → FIXED
Backed out. I must have broken something at the last minute.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't know how to test this automatically, but we need to make sure that a floating first-letter doesn't try to form a ligature with the following text --- that would look bad. So we need to break textruns at the float boundary.
Attached patch fix v2 (obsolete) — Splinter Review
To break the textrun while leaving this bug fixed, we have to separate the results "break textrun" from "break nsLineBreaker".
Attachment #330848 - Attachment is obsolete: true
Attachment #331243 - Flags: superreview?(smontagu)
Attachment #331243 - Flags: review?(smontagu)
I tried to make a reftest reference like this:

<p><span style="float:left">f</span>ish fish</p>

... but your test case moves the f up a little relative to the general baseline and this doesn't.  Which seems like another bug in itself.  And there's no reason why a bug in the textrun boundaries wouldn't affect both of these equally, so it's not a great test case ... maybe do something dirty with position:absolute, ugh.
The f moves up because we replace the font ascent with the glyph's actual height (and the descent with the glyph's actual descent), so that drop caps look good. To stop the f from moving up, we'd have to either max the outer line's ascent into the f's ascent or do some kind of fancy baseline semi-alignment.
Floating first letter isn't actually supposed to follow the standard rules for 'float' (although it took quite a while before the spec actually said that).
Attached patch fix v3Splinter Review
I realized I could write a test using ZWNJ in the reference to suppress the ligature. But the test didn't work, because of a new bug: the width of the floating first-letter is computed using a textrun built when all the text was in the first-letter textframe, so it thinks there's a ligature for the first two characters.

So to fix that, this patch changes nsTextFrame::Reflow so that for first-letter frames, we set the frame's length to the first-letter length and then reconstruct the textrun so that the textrun sees the correct first-letter boundary.

There's also a problem that the line container in nsLineLayout seen by nsTextFrame::Reflow for the floating first-letter text is the nsFirstLetterFrame. We shouldn't use that as a scope for textrun construction, though, so I've modified BuildTextRuns to call FindLineContainer ignoring aLineContainer when a real line is not provided, and I've modified FindLineContainer to not treat nsFirstLetterFrame as a line container.

This is a bit more than I really wanted to do here but it seems OK.
Attachment #331243 - Attachment is obsolete: true
Attachment #331251 - Flags: review?(smontagu)
Attachment #331243 - Flags: superreview?(smontagu)
Attachment #331243 - Flags: review?(smontagu)
Attachment #331251 - Flags: review?(smontagu) → review+
Pushed 36b049ebc16d
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
I just checked in a backported version of this bug's patch to the 1.9.0.x branch, as part of the fix for bug 431260.
==========
Checking in layout/generic/nsLineLayout.h;                                                                   
/cvsroot/mozilla/layout/generic/nsLineLayout.h,v  <--  nsLineLayout.h                                        
new revision: 3.120; previous revision: 3.119                                                                
done                                                                                                         
Checking in layout/generic/nsTextFrameThebes.cpp;                                                            
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.186; previous revision: 3.185
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/431341-1-ref.html,v
done
Checking in layout/reftests/bugs/431341-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/431341-1-ref.html,v  <--  431341-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/431341-1.html,v
done
Checking in layout/reftests/bugs/431341-1.html;
/cvsroot/mozilla/layout/reftests/bugs/431341-1.html,v  <--  431341-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/431341-2-ref.html,v
done
Checking in layout/reftests/bugs/431341-2-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/431341-2-ref.html,v  <--  431341-2-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/431341-2.html,v
done
Checking in layout/reftests/bugs/431341-2.html;
/cvsroot/mozilla/layout/reftests/bugs/431341-2.html,v  <--  431341-2.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.470; previous revision: 1.469
done
Keywords: fixed1.9.0.8
Verifying this in 1.9.0.7 versus the 1.9.0.8 nightly (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre), I see no difference in rendering between 1.9.0.7 and 1.9.0.8 with either testcase.

Does this bug actively appear in 1.9.0.x?
not sure - i can't test right now because i'm traveling & using my phone for internet, but it's definitely possible that this regressed since 190x branched off.  It looks like we don't have a regression source or date identified for this yet.  But comment 3 specifically mentions that this bug isn't in FF2, which sorta implies that this *was* broken in FF3 / 190x . . .
As the original reporter: yes, this was broken in FF3 as of when I reported it.  Al: when you say "I see no difference in rendering" -- are they both right or both wrong?
1.9.0.7 and 1.9.0.8 both look like the attached reference rendering.
How odd, I definitely see the bug here with 1.9.0.7.
What OS?

Can you take a look with the 1.9.0.8 build from yesterday and see if it looks fixed?
and now I see the difference in the first 'h' after the giant T.
Verified for 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre even though I can't see the obvious.
You need to log in before you can comment on or make changes to this bug.