Bug 45986 - :first-letter should apply to "punctuation" after the first letter
Summary: :first-letter should apply to "punctuation" after the first letter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: HasReduction
: 47160 (view as bug list)
Depends on:
Blocks: 47141
  Show dependency treegraph
 
Reported: 2010-09-17 12:26 PDT by Joseph Pecoraro
Modified: 2010-10-06 19:38 PDT (History)
2 users (show)

See Also:


Attachments
[TEST] Test Page (266 bytes, text/html)
2010-09-17 12:26 PDT, Joseph Pecoraro
no flags Details
Patch (26.46 KB, patch)
2010-10-06 17:06 PDT, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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