Bug 130457 – Improper styling of pseudo element first-letter

• Component: Layout and Rendering
• Status: NEW

[Mario Sanchez Prada]

This bug is tracks down an issue as reported for Blink as issue 340688[1]. Description of the issue as reported there is as follows:

What steps will reproduce the problem?
1. Create an element who matches the the following letter-space-ampersand-space.  Other variations probably exist, but this is how I encountered it.
2. Style the element using pseudo-class first-letter.
In this case:
.subcat-categories h3{
	font-size:12px;
	font-weight:bold;color:#ae2525;
}
.subcat-categories h3:first-letter{font-size:135%;}

What is the expected result?
The first letter should be styled with the pseudo class.

What happens instead?
The first letter AND the ampersand are styled with the pseudo class.


[1] https://code.google.com/p/chromium/issues/detail?id=340688
[2] https://src.chromium.org/viewvc/blink?revision=169547&view=revision
[3] trac.webkit.org/wiki/CSS21Results

[Mario Sanchez Prada]

Created attachment 227193 [details]
Patch proposal

Attaching the patch as backported from Blink. Please notice that I needed to backport also some additional code (from blink revision r159205[1]) in order to make sure that the behaviour is consistent, because otherwise we would get weird effects when no first-letter would be created at all. Specifically about patch from Blink r159205, I haven't backported the changes for the tests added/modified there because:

- editing/selection/extend-by-word-002.html: it's written in a different way in WebKit and, apparently, it does not need updating (at least for the GTK port)
- fast/css-generated-content/quote-first-letter.html: didn't need updating at all
- fast/css-generated-content/empty-first-letter-with-columns-crash.html: WebKit was not crashing before nor after applying this patch, so there's no point on adding I guess

So, that's why I only took the code from r159205 :)

Last, I run the CSS2.1 test suite manually [2] and I'm happy to say this patch did NOT introduce any regression in the first-letter* tests there, so please review.

Thanks

[1] https://src.chromium.org/viewvc/blink?revision=159205&view=revision
[2] http://trac.webkit.org/wiki/CSS21Results

[Build Bot]

Comment on attachment 227193 [details]
Patch proposal

Attachment 227193 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4627727839657984

New failing tests:
fast/css-generated-content/quote-first-letter.html
editing/selection/extend-by-word-002.html

[Build Bot]

Created attachment 227201 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 227193 [details]
Patch proposal

Attachment 227193 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5597308116795392

New failing tests:
fast/css-generated-content/quote-first-letter.html
editing/selection/extend-by-word-002.html

[Build Bot]

Created attachment 227202 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

(In reply to comment #4)
> (From update of attachment 227193 [details])
> Attachment 227193 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/5597308116795392
> 
> New failing tests:
> fast/css-generated-content/quote-first-letter.html
> editing/selection/extend-by-word-002.html

It seems there were some changes needed after all. I'll work on those

[Mario Sanchez Prada]

Comment on attachment 227193 [details]
Patch proposal

After investigating this issue a bit more, it seems to me that the difference in the results comes from the fact that the backported code came with a problem, inherited from Blink r159205:

+  if (descendant->isText()) {
+     // FIXME: If there is leading punctuation in a different RenderText than
+     // the first letter, we'll not apply the correct style to it.
+     length = firstLetterLength(toRenderText(*descendant).originalText());
+     if (length)
+         break;

For instance, in editing/selection/extend-by-word-002.html, for the following code (where we have defined "ul.menu li:first-letter { font-size:20px;line-height:10px; }"):

  <li>&middot; <a href="detail.asp?cat=13">Soups & Salads</a></li>

According to the spec, the UA does not need to create a first-letter pseudoelement in this case, which is what Opera and Firefox do. However, the present patch applies the first-letter style to the "A" after the dot, which is wrong. Even worse, I've tried the following code:

  <li>&middot; Starter <a href="detail.asp?cat=13">Soups & Salads</a></li>

And I get the list element rendered applying the first letter style over the "S" of "Soups", instead of doing nothing (as there's a space after the dot. If however, I remove the space between the dot and "Starter":

  <li>&middot;Starter <a href="detail.asp?cat=13">Soups & Salads</a></li>

I get consistent rendering across all browsers, applying the style over the "S" of "Starter". It's a pity I didn't detect this first in Blink, but the way how extend-by-words-002.html is tested there didn't catch this issue

So, I think I'll get back to blink, try to fix these issues there first and then will come back here. Now removing flags.