Bug 45986

Summary: :first-letter should apply to "punctuation" after the first letter
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Layout and RenderingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: joepeck, simon.fraser
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 47141    
Attachments:
Description Flags
[TEST] Test Page
none
Patch mitz: review+

Description Joseph Pecoraro 2010-09-17 12:26:31 PDT
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
Comment 1 Joseph Pecoraro 2010-09-17 12:40:28 PDT
I forgot to link to the spec:
http://www.w3.org/TR/CSS2/selector.html#first-letter
Comment 2 Simon Fraser (smfr) 2010-10-05 20:22:48 PDT
This is the same as bug 47160.
Comment 3 Simon Fraser (smfr) 2010-10-06 16:52:19 PDT
*** Bug 47160 has been marked as a duplicate of this bug. ***
Comment 4 Simon Fraser (smfr) 2010-10-06 17:06:52 PDT
Created attachment 70015 [details]
Patch
Comment 5 mitz 2010-10-06 17:18:26 PDT
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.
Comment 6 Darin Adler 2010-10-06 17:27:08 PDT
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.
Comment 7 Simon Fraser (smfr) 2010-10-06 19:38:03 PDT
http://trac.webkit.org/changeset/69270