From http://www.w3.org/TR/CSS21/selector.html#first-letter : 'Punctuation (i.e, characters defined in Unicode in the "open" (Ps), "close" (Pe), "initial" (Pi). "final" (Pf) and "other" (Po) punctuation classes), that precedes or follows the first letter should be included' Currently we include punctuation preceding the first letter, but not following. Not a regression, but requesting blocking1.9 for standards compliance.

Just for the record, CSS2 only says 'precedes the first letter'.

Comment 1 refers to CSS2.0, which is no longer what "CSS2" means aiui.

Should probably advance to the end of the cluster containing any trailing punctuation. Should probably call FindClusterEnd to do this (and use that to instead of the find-end-of-cluster-loop already in the FindFirstLetterRange).

This passes all the reftests in layout/reftests/first-letter without assertions, plus some extra ones I added to cover trailing punctuation, and the testcase in bug 329069 (which we should really add a reftest for). I want to test more scenarios before asking for review, but interim comments are always welcome.

Comment on attachment 285290 [details] [diff] [review] Patch >+ // consume clusters that start with whitespace or punctuation > for (i = 0; i < length; ++i) { >+ if (IsTrimmableSpace(aFrag, aOffset + i) || >+ nsTextFrameUtils::IsPunctuationMark(aFrag->CharAt(aOffset + i))) { >+ iter.SetSkippedOffset(aOffset + i); >+ FindClusterEnd(aTextRun, aOffset + length, &iter); >+ i = iter.GetSkippedOffset() - aOffset; >+ } else { > break; >+ } I'm not sure about this part of the algorithm. Do we really want to include punctuation interspersed with whitespace? Would this be better? for (i = GetTrimmableWhitespaceCount(aFrag, aOffset, length, 1); i < length; ++i) { if (nsTextFrameUtils::IsPunctuationMark(aFrag->CharAt(aOffset + i))) { I'm also not sure if I should be using iter.[GS]etSkippedOffset or [GS]etOriginalOffset

> Do we really want to include punctuation interspersed with whitespace? I think we might as well. I think instead of making i the key variable, we should just use iter as the key variable. Just compare iter.GetOriginalOffset() to aOffset + aLength.

Simon, were you planning to finish this?

After fiddling with this for some time, I'm fairly sure that we don't want to include punctuation interspersed with whitespace. Consider this case: "*" is an asterisk. The first-letter style would be applied to '"*" i', which seems totally wrong. A question that bothers me more, though, is: when is a letter not a letter? CSS doesn't explicitly define what "letter" means in this context, though it does go out of its way to specify that the ':first-letter' also applies if the first letter is in fact a digit. To me this implies that if the first letter is in fact a punctuation mark (other than those specified to be included) or a symbol character, :first-letter should not apply.

This does what I believe is right, and only applies first-letter style to letters and numbers (with preceding and following punctuation). It also adds support for surrogate characters to IsPunctuationMark(). I'll be adding lots of reftests too.

+ static nsCOMPtr<nsIUGenCategory> genCatService = + do_GetService(NS_UNICHARCATEGORY_CONTRACTID); You might as well cache this with the other services that nsContentUtils caches. + // If the next character is not a letter or number, there is no first-letter. + // Return PR_TRUE so that we don't go on looking, but set aLength to 0. So this means that '<span>"</span> F' does get first-letter style applied to the punctuation. That's unfortunate but I guess anything better is a lot harder, so I'm OK with it. The code for "consume cluster that starts with punctuation" could be shared. Looks good other than that.

Checked in

Note: if we take this on the 3.0 branch, the branch patch should include the fix to bug 445626