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

Status: RESOLVED FIXED

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

Description

[Zack Weinberg (:zwol)]

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

Comment 1

[Zack Weinberg (:zwol)]

Attachment: reference rendering

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 3

[Zack Weinberg (:zwol)]

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.

Comment 4

[Zack Weinberg (:zwol)]

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.

Comment 5

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

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.

Comment 6

[Zack Weinberg (:zwol)]

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.

Comment 7

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

Attachment: fix

Descending into the first-letter float works, but we have to be careful about how we do it.

Comment 8

[Zack Weinberg (:zwol)]

Do I understand correctly that your revised CanTextRunCrossFrameBoundary is relying on ->CanContinueTextRun() to distinguish first-letter placeholders from other placeholders?

Comment 9

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

Yes, I admit that is dirty.

Comment 10

[Zack Weinberg (:zwol)]

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 11

[Simon Montagu :smontagu]

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.

Comment 12

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

Pushed 2d36444242f2

Comment 13

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

Backed out. I must have broken something at the last minute.

Comment 14

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

Attachment: test that a floating first-letter can't form a ligature with the following text

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.

Comment 15

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

Attachment: fix v2

To break the textrun while leaving this bug fixed, we have to separate the results "break textrun" from "break nsLineBreaker".

Comment 16

[Zack Weinberg (:zwol)]

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.

Comment 17

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

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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 19

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

Attachment: fix v3

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.

Comment 20

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

Pushed 36b049ebc16d

Comment 21

[Daniel Holbert [:dholbert]]

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

Comment 22

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 23

[Daniel Holbert [:dholbert]]

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

Comment 24

[Zack Weinberg (:zwol)]

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?

Comment 25

[Al Billings [:abillings - ex-MoCo]]

1.9.0.7 and 1.9.0.8 both look like the attached reference rendering.

Comment 26

[Zack Weinberg (:zwol)]

How odd, I definitely see the bug here with 1.9.0.7.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

What OS?

Can you take a look with the 1.9.0.8 build from yesterday and see if it looks fixed?

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Attachment: My rendering of testcase in 1.9.0.7.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Attachment: My rendering of testcase in 1.9.0.8.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

and now I see the difference in the first 'h' after the giant T.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

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.