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+

Joseph Pecoraro
Reported 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
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+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2010-09-17 12:40:28 PDT
I forgot to link to the spec: http://www.w3.org/TR/CSS2/selector.html#first-letter
Simon Fraser (smfr)
Comment 2 2010-10-05 20:22:48 PDT
This is the same as bug 47160.
Simon Fraser (smfr)
Comment 3 2010-10-06 16:52:19 PDT
*** Bug 47160 has been marked as a duplicate of this bug. ***
Simon Fraser (smfr)
Comment 4 2010-10-06 17:06:52 PDT
Created attachment 70015 [details] Patch
mitz
Comment 5 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.
Darin Adler
Comment 6 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.
Simon Fraser (smfr)
Comment 7 2010-10-06 19:38:03 PDT
http://trac.webkit.org/changeset/69270
Note You need to log in before you can comment on or make changes to this bug.