Bug 45986 – :first-letter should apply to "punctuation" after the first letter

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Joseph Pecoraro]

Created attachment 67933 [details]
[TEST] Test Page

The CSS 2.1 Spec states:

>  Punctuation (i.e, characters defined in Unicode [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 [...]

So for:

  <style> p:first-letter { color:red; }</style>
  <p>!!!H!!!ello</p>

We should expect the first 7 characters to be red. Currently just the first 4 are red.

Other Browsers:

  - Mac - Firefox 4b6 handles this correctly
  - Mac - Opera 10.60 handles this correctly

[Joseph Pecoraro]

I forgot to link to the spec:
http://www.w3.org/TR/CSS2/selector.html#first-letter

[Simon Fraser (smfr)]

This is the same as bug 47160.

[Simon Fraser (smfr)]

*** Bug 47160 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Created attachment 70015 [details]
Patch

Comment on attachment 70015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70015&action=review

> WebCore/rendering/RenderBlock.cpp:5235
> +inline bool isPunctForFirstLetter(UChar c)

A “uation” here won’t hurt.

> WebCore/rendering/RenderBlock.cpp:5384
> +        unsigned scanLength = length;
> +        while (scanLength < oldText->length()) {

I’d write this as a for() loop and so scope scanLength.

[Darin Adler]

Comment on attachment 70015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70015&action=review

>> WebCore/rendering/RenderBlock.cpp:5235
>> +inline bool isPunctForFirstLetter(UChar c)
> 
> A “uation” here won’t hurt.

Since this is in a .cpp file, not a header, it should say “static inline” so this gets internal linkage.

> WebCore/rendering/RenderBlock.cpp:5245
> +inline bool skipForFirstLetter(UChar c)

I would name this shouldSkipForFirstLetter.

Since this is in a .cpp file, not a header, it should say “static inline” so this gets internal linkage.

> WebCore/rendering/RenderBlock.cpp:5247
> +    return isSpaceOrNewline(c) || c == noBreakSpace || isPunctForFirstLetter(c);

It looks like you added noBreakSpace, not treated as a space by the old code. I assume that’s a bug fix. But not covered by a test case. You should add one.

Also, another annoying thought about shortcomings of the code not directly related to your patch: Is this definition of space the right one? It’s a pretty fancy rule based on Unicode direction, which might make it handle unusual space characters correctly, but for newline it unconditionally treats it as a space, but some newlines are not collapsed into spaces, so it probably will get that wrong and include punctuation after a newline if the whitespace mode is a “pre”-type mode.

> WebCore/rendering/RenderBlock.cpp:5397
>          // NOTE: this might empty

This confusing comment is right next to the code you are modifying. Maybe you could fix it.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/69270