Bug 130457 - Improper styling of pseudo element first-letter
Summary: Improper styling of pseudo element first-letter
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks:
 
Reported: 2014-03-19 09:54 PDT by Mario Sanchez Prada
Modified: 2022-11-02 06:16 PDT (History)
12 users (show)

See Also:


Attachments
Patch proposal (66.45 KB, patch)
2014-03-19 10:30 PDT, Mario Sanchez Prada
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (661.77 KB, application/zip)
2014-03-19 11:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (517.54 KB, application/zip)
2014-03-19 11:25 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2014-03-19 09:54:20 PDT
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
Comment 1 Mario Sanchez Prada 2014-03-19 10:30:25 PDT
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
Comment 2 Build Bot 2014-03-19 11:24:41 PDT
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
Comment 3 Build Bot 2014-03-19 11:24:47 PDT
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
Comment 4 Build Bot 2014-03-19 11:24:57 PDT
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
Comment 5 Build Bot 2014-03-19 11:25:02 PDT
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
Comment 6 Mario Sanchez Prada 2014-03-20 03:42:20 PDT
(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
Comment 7 Mario Sanchez Prada 2014-03-20 04:06:13 PDT
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.