Bug 80794

Summary: :first-line pseudo selector ignoring words created from :before
Product: WebKit Reporter: Marcos Zanona <marcos>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: arpitabahuguna, dbates, eric, esprehn, hyatt, inferno, jchaffraix, koivisto, mitz, rniwa, robert, simon.fraser, vijayan.bits, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://jsfiddle.net/Q8udZ/
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Marcos Zanona 2012-03-11 10:36:41 PDT
When creating a content and through css appending content through :before selector when styling it with :first-line selector it will ignore the content generated by :before, this is not true for :first-letter selector though. Please check the related URL for example and more details.
After testing it succeeded in Firefox and Opera lastest public releases but not for latest Chrome, Safari and Nightly Webkit.
Comment 1 Arpita Bahuguna 2012-06-13 02:55:25 PDT
Created attachment 147269 [details]
Patch
Comment 2 Arpita Bahuguna 2012-06-13 03:08:08 PDT
According to the CSS2.1 specification:

When the :first-letter and :first-line pseudo-elements are applied to an element having content generated using :before and :after, they apply to the first letter or line of the element including the generated content.

Have uploaded a patch for handling of :first-line style on content generated from :before/:after.
Comment 3 Robert Hogan 2012-07-10 11:49:31 PDT
Comment on attachment 147269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147269&action=review

> Source/WebCore/rendering/RenderObject.cpp:2511
> +    } else if (renderer->isBeforeOrAfterContent() && renderer->parent()->isBlockFlow()) {
> +        // For handling content generated from :before/:after pseudo-element.
> +        if (RenderBlock* firstLineBlock = renderer->parent()->firstLineBlock())
> +            style = firstLineBlock->getCachedPseudoStyle(FIRST_LINE, style);

Don't you have to patch RenderObject::uncachedFirstLineStyle() too?
Comment 4 Arpita Bahuguna 2012-07-11 03:03:20 PDT
Created attachment 151665 [details]
Patch
Comment 5 Arpita Bahuguna 2012-07-11 03:13:51 PDT
(In reply to comment #3)
> (From update of attachment 147269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147269&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:2511
> > +    } else if (renderer->isBeforeOrAfterContent() && renderer->parent()->isBlockFlow()) {
> > +        // For handling content generated from :before/:after pseudo-element.
> > +        if (RenderBlock* firstLineBlock = renderer->parent()->firstLineBlock())
> > +            style = firstLineBlock->getCachedPseudoStyle(FIRST_LINE, style);
> 
> Don't you have to patch RenderObject::uncachedFirstLineStyle() too?
Agree! I missed out on that. Have uploaded another patch with the necessary changes.
Thanks for the review Robert. :)
Comment 6 Eric Seidel (no email) 2012-09-10 20:06:28 PDT
Thoughts, elliot?
Comment 7 Elliott Sprehn 2012-09-11 16:06:16 PDT
Comment on attachment 151665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151665&action=review

This seems okay, but the amount of duplication between uncachedFirstLineStyle and firstLineStyleSlowCase seems bad. Can you express one in terms of the other, or add a helper?

> Source/WebCore/rendering/RenderObject.cpp:2514
> +        // For handling content generated from :before/:after pseudo-element.

Not sure these comments are needed. You have the isBeforeOrAfterContent which seems clear.
Comment 8 Elliott Sprehn 2012-09-11 16:06:17 PDT
Comment on attachment 151665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151665&action=review

This seems okay, but the amount of duplication between uncachedFirstLineStyle and firstLineStyleSlowCase seems bad. Can you express one in terms of the other, or add a helper?

> Source/WebCore/rendering/RenderObject.cpp:2514
> +        // For handling content generated from :before/:after pseudo-element.

Not sure these comments are needed. You have the isBeforeOrAfterContent which seems clear.
Comment 9 Arpita Bahuguna 2012-09-12 23:28:21 PDT
Created attachment 163788 [details]
Patch
Comment 10 Arpita Bahuguna 2012-09-12 23:41:27 PDT
(In reply to comment #8)
> (From update of attachment 151665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151665&action=review
> 
Thank-you for the review Elliott.

> This seems okay, but the amount of duplication between uncachedFirstLineStyle and firstLineStyleSlowCase seems bad. Can you express one in terms of the other, or add a helper?
> 
Have added a static helper function containing the duplicate code. Have also introduced an enum for differentiating between the cached and uncached style handling and to avoid passing a boolean as an input param.

> > Source/WebCore/rendering/RenderObject.cpp:2514
> > +        // For handling content generated from :before/:after pseudo-element.
> 
> Not sure these comments are needed. You have the isBeforeOrAfterContent which seems clear.
Removed the comment.
Comment 11 Elliott Sprehn 2012-10-01 18:28:35 PDT
Comment on attachment 163788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163788&action=review

> Source/WebCore/rendering/RenderObject.cpp:2608
> +    UnCached

Uncached

> Source/WebCore/rendering/RenderObject.cpp:2611
> +static PassRefPtr<RenderStyle> getFirstLineStyle(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)

Can we make this a

> Source/WebCore/rendering/RenderObject.cpp:2620
> +                result = firstLineBlock->getUncachedPseudoStyle(FIRST_LINE, style, firstLineBlock == renderer ? style : 0);

These should use early returns:

if (type == Cached)
  return ...;
return ...;

It's a lot shorter that way too.

> Source/WebCore/rendering/RenderObject.cpp:2637
> +                result = firstLineBlock->getUncachedPseudoStyle(FIRST_LINE, style, firstLineBlock == renderer ? style : 0);

Use early returns and get rid of the RefPtr<RenderStyle> result

> Source/WebCore/rendering/RenderObject.cpp:2657
> +    if (RefPtr<RenderStyle> style = getFirstLineStyle(Cached, isText() ? parent() : this, m_style.get()))

This is super weird that the Cached case is considered the slow one. Any idea what's up with that?
Comment 12 Elliott Sprehn 2012-10-01 18:28:36 PDT
Comment on attachment 163788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163788&action=review

> Source/WebCore/rendering/RenderObject.cpp:2608
> +    UnCached

Uncached

> Source/WebCore/rendering/RenderObject.cpp:2611
> +static PassRefPtr<RenderStyle> getFirstLineStyle(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)

Can we make this a

> Source/WebCore/rendering/RenderObject.cpp:2620
> +                result = firstLineBlock->getUncachedPseudoStyle(FIRST_LINE, style, firstLineBlock == renderer ? style : 0);

These should use early returns:

if (type == Cached)
  return ...;
return ...;

It's a lot shorter that way too.

> Source/WebCore/rendering/RenderObject.cpp:2637
> +                result = firstLineBlock->getUncachedPseudoStyle(FIRST_LINE, style, firstLineBlock == renderer ? style : 0);

Use early returns and get rid of the RefPtr<RenderStyle> result

> Source/WebCore/rendering/RenderObject.cpp:2657
> +    if (RefPtr<RenderStyle> style = getFirstLineStyle(Cached, isText() ? parent() : this, m_style.get()))

This is super weird that the Cached case is considered the slow one. Any idea what's up with that?
Comment 13 Elliott Sprehn 2012-10-01 18:29:38 PDT
(In reply to comment #12)
> (From update of attachment 163788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163788&action=review
> 

> > Source/WebCore/rendering/RenderObject.cpp:2611
> > +static PassRefPtr<RenderStyle> getFirstLineStyle(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)
> 
> Can we make this a

Ignore this comment.
Comment 14 Daniel Bates 2012-10-01 20:45:32 PDT
Comment on attachment 163788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163788&action=review

Please remove the tab characters from files LayoutTests/fast/css/first-line-style-for-before-after-{content, content-expected}.html. In particular, lines 13, 19, 20, and 21 contain tab characters in file first-line-style-for-before-after-content.html and lines 16, 22, 23, 24 contain tab characters in file first-line-style-for-before-after-content-expected.html.

> LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:23
> +</html>

Nit: Missing a newline character at the end of this file.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:1
> +<!DOCTYPE html>

The name of this test is disingenuous as it only tests the combination of :first-line, :first-letter, and :before pseudo-elements. That is, it doesn't test the combination :first-line, :first-letter, and :after pseudo elements. Even though we have a single code path for handling before and after content (via the check for Render::isBeforeOrAfterContent()), I suggest that we either rename this test and its descriptive to make it explicit that it's only testing :first-line, :first-letter, and :before pseudo elements OR, even better, add a test that makes use of :first-line, :first-letter, and :after pseudo elements.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:26
> +</html>

Nit: Missing a newline character at the end of this file.
Comment 15 Arpita Bahuguna 2012-10-03 02:53:43 PDT
Created attachment 166838 [details]
Patch
Comment 16 Elliott Sprehn 2012-10-03 03:08:04 PDT
Comment on attachment 166838 [details]
Patch

Looks good to me, but I'm not a reviewer so someone else would need to r+ :)
Comment 17 Arpita Bahuguna 2012-10-03 04:38:57 PDT
(In reply to comment #16)
> (From update of attachment 166838 [details])
> Looks good to me, but I'm not a reviewer so someone else would need to r+ :)

Hi Elliott, thanks for your reviews. :) Have incorporated the changes as suggested by you in the latest patch. Thanks again!
Comment 18 Arpita Bahuguna 2012-10-03 04:42:03 PDT
(In reply to comment #14)
> (From update of attachment 163788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163788&action=review
> 
Thanks for the review Daniel. Have uploaded another patch as per your comments.
Have added another case for verifying :first-line, :first-letter, and :after pseudo elements behavior as well.
Comment 19 Daniel Bates 2012-10-03 10:15:18 PDT
Comment on attachment 166838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166838&action=review

> Source/WebCore/rendering/RenderObject.cpp:2607
> +static PassRefPtr<RenderStyle> getFirstLineStyle(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)

This is a very minor nit. We tend to prefix functions with the word "get" when it returns one or more values through out arguments (*). Notice that this function doesn't return any value through an out argument. Although we have functions that don't follow this convention, including RenderObject::{getCachedPseudoStyle, getUncachedPseudoStyle}(), it would be great if we could come up with a descriptive name for this function that omitted the prefix "get".

(*) See <http://www.webkit.org/coding/coding-style.html#names-out-argument>.

> Source/WebCore/rendering/RenderObject.cpp:2618
> +            // A first-line style is in effect. Cache a first-line style for ourselves.

I suggest that we move this comment inside the block associated with the "if (type == Cached)" statement (below) so as to scope it to that code.

> Source/WebCore/rendering/RenderObject.cpp:2629
> +            return firstLineBlock->getUncachedPseudoStyle(FIRST_LINE, style, firstLineBlock == renderer ? style : 0);

Can you give an example when firstLineBlock == renderer evaluates to true?
Comment 20 Arpita Bahuguna 2012-10-04 04:17:53 PDT
Created attachment 167070 [details]
Patch
Comment 21 Arpita Bahuguna 2012-10-04 22:57:26 PDT
Created attachment 167255 [details]
Patch
Comment 22 Arpita Bahuguna 2012-10-04 23:19:32 PDT
(In reply to comment #19)
> (From update of attachment 166838 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166838&action=review
> 
Thanks for the review Daniel.

> > Source/WebCore/rendering/RenderObject.cpp:2607
> > +static PassRefPtr<RenderStyle> getFirstLineStyle(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)
> 
> This is a very minor nit. We tend to prefix functions with the word "get" when it returns one or more values through out arguments (*). Notice that this function doesn't return any value through an out argument. Although we have functions that don't follow this convention, including RenderObject::{getCachedPseudoStyle, getUncachedPseudoStyle}(), it would be great if we could come up with a descriptive name for this function that omitted the prefix "get".
> 
> (*) See <http://www.webkit.org/coding/coding-style.html#names-out-argument>.
> 
I tried to come up with other alternative names such as, firstLineStyleForCacheType, deriveFirstLineStyle etc. but firstLineStyle() seemed to be the most appropriate. Since that is already in use, have currently changed the name to firstLineStyleForCachedUncachedType since I understand prefixing the function name with 'get' here would not be correct.

> > Source/WebCore/rendering/RenderObject.cpp:2618
> > +            // A first-line style is in effect. Cache a first-line style for ourselves.
> 
> I suggest that we move this comment inside the block associated with the "if (type == Cached)" statement (below) so as to scope it to that code.
> 
Done.

> > Source/WebCore/rendering/RenderObject.cpp:2629
> > +            return firstLineBlock->getUncachedPseudoStyle(FIRST_LINE, style, firstLineBlock == renderer ? style : 0);
> 
> Can you give an example when firstLineBlock == renderer evaluates to true?

After verification of the condition I realized that we need to be checking for firstLineBlock == renderer->parent() for this particular case, if required at all. This lead to further checking with various other scenarios and unfortunately my fix was still not handling the following case correctly:

<p id="firstline"><span id="before">Content</span> </p>
Here our <p> tag has :first-line specified for it and our <span> has :before specified for it.

For such a case, my previous fix was insufficient as it would only check for the parent renderer to be a blockflow. But in case (as shown above) our parent renderer (for the generated :before/:after content) is a RenderInline, we would still not apply the first-line style to this content.

Thus, it basically implies that if in case our renderer has before or after content, we should use its parent and carry out the same checks as were done previously, i.e. for both block flow as well as inline.

Have made the changes accordingly in the latest patch; hope this shall be more appropriate.
Comment 23 Elliott Sprehn 2012-10-05 02:02:31 PDT
Comment on attachment 167255 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167255&action=review

> Source/WebCore/rendering/RenderObject.cpp:2607
> +static PassRefPtr<RenderStyle> firstLineStyleForCachedUncachedType(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)

This method name is confusing. Instead rename firstLineStyleSlowCase to cachedFirstLineStyle and fix the one usage of it in the header and then rename this firstLineStyle(type,renderer,style)
Comment 24 Arpita Bahuguna 2012-10-05 05:09:28 PDT
(In reply to comment #23)
> (From update of attachment 167255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167255&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:2607
> > +static PassRefPtr<RenderStyle> firstLineStyleForCachedUncachedType(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)
> 
> This method name is confusing. Instead rename firstLineStyleSlowCase to cachedFirstLineStyle and fix the one usage of it in the header and then rename this firstLineStyle(type,renderer,style)

Hi Elliott. I agree the new name doesn't seem right but had used it anyway for want of a better name, since firstLineStyle() is already in use (as part of RenderObject).
I understand the newly added function is a static and hence I can still get away with using the same name for it as well i.e. firstLineStyle but just wanted to confirm once whether that would be the correct way to go about it.

Also, firstLineStyleSlowCase name should be changed as well and cachedFirstLineStyle definitely seems more appropriate but should that be spawned off as a separate bug and not be made a part of this patch? Just wondering.
Comment 25 Elliott Sprehn 2012-10-05 11:08:09 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 167255 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=167255&action=review
> > 
> > > Source/WebCore/rendering/RenderObject.cpp:2607
> > > +static PassRefPtr<RenderStyle> firstLineStyleForCachedUncachedType(StyleCacheState type, const RenderObject* renderer, RenderStyle* style)
> > 
> > ...
> 
> Also, firstLineStyleSlowCase name should be changed as well and cachedFirstLineStyle definitely seems more appropriate but should that be spawned off as a separate bug and not be made a part of this patch? Just wondering.

Seems reasonable to fix that stuff in a separate patch. Lets get this landed!
Comment 26 Daniel Bates 2012-10-06 15:28:10 PDT
Comment on attachment 167255 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167255&action=review

> Source/WebCore/ChangeLog:30
> +        helper function getFirstLineStyle().

getFirstLineStyle() => firstLineStyleForCachedUncachedType()

> LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:13
> +p:first-line {
> +    color: green;
> +}
> +p:first-letter {
> +    color: blue;
> +}	

It bothers me that we are using :first-line and :first-letter in this expected results file given that this patch fixes an issue with generated content/pseudo-elements. Although this expected results file uses existing machinery, including machinery we preserved in this patch, it is preferred that we construct the expected results without the use of the :first-letter and :first-line pseudo-elements so as to further differentiate the code path taken to generate the actual and expected results of the test, respectively. Can we come up with a way to generate these expected results without using :first-line and :first-letter? Maybe construct the contents of the HTML p elements (below) using stylized <span>s?

> LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:14
> +</style>

Please remove the tab character at the end of the previous line.

> LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:19
> +    <br/>

Nit: No need to explicitly terminate the br element in an HTML5 document. We can write this as <br>.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:25
> +}	

Please remove the tab character at the end of this line.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:31
> +    <br/>

Nit: No need to explicitly terminate the br element in an HTML5 document. We can write this as <br>.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:37
> +    <p><span id="before"> of this paragraph should be displayed in green color.</span></p>

This is invalid markup by uniqueness of the HTML attribute id, <http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the-id-attribute>. That is, the HTML attribute id for the <span> must be unique, but it isn't. The first occurrence of this id was on line 34.

> LayoutTests/fast/css/first-line-style-for-before-after-content.html:43
> +    <p><span id="after">Display </span></p>

This is invalid markup by uniqueness of the HTML attribute id, <http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the-id-attribute>. That is, the HTML attribute id for the <span> must be unique, but it isn't. The first occurrence of this id was on line 40.
Comment 27 Arpita Bahuguna 2012-10-07 02:27:59 PDT
Created attachment 167477 [details]
Patch
Comment 28 Arpita Bahuguna 2012-10-07 05:01:03 PDT
(In reply to comment #26)
> (From update of attachment 167255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167255&action=review
> 
Thanks for the review Daniel. Have uploaded another patch with the following changes.

> > Source/WebCore/ChangeLog:30
> > +        helper function getFirstLineStyle().
> 
> getFirstLineStyle() => firstLineStyleForCachedUncachedType()
> 
Done.
> > LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:13
> > +p:first-line {
> > +    color: green;
> > +}
> > +p:first-letter {
> > +    color: blue;
> > +}	
> 
> It bothers me that we are using :first-line and :first-letter in this expected results file given that this patch fixes an issue with generated content/pseudo-elements. Although this expected results file uses existing machinery, including machinery we preserved in this patch, it is preferred that we construct the expected results without the use of the :first-letter and :first-line pseudo-elements so as to further differentiate the code path taken to generate the actual and expected results of the test, respectively. Can we come up with a way to generate these expected results without using :first-line and :first-letter? Maybe construct the contents of the HTML p elements (below) using stylized <span>s?
> 
Have used <span>s with style now for obtaining the expected result.

> > LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:14
> > +</style>
> 
> Please remove the tab character at the end of the previous line.
> 
> > LayoutTests/fast/css/first-line-style-for-before-after-content-expected.html:19
> > +    <br/>
> 
> Nit: No need to explicitly terminate the br element in an HTML5 document. We can write this as <br>.
> 
> > LayoutTests/fast/css/first-line-style-for-before-after-content.html:25
> > +}	
> 
> Please remove the tab character at the end of this line.
> 
> > LayoutTests/fast/css/first-line-style-for-before-after-content.html:31
> > +    <br/>
> 
> Nit: No need to explicitly terminate the br element in an HTML5 document. We can write this as <br>.
> 
Done.

> > LayoutTests/fast/css/first-line-style-for-before-after-content.html:37
> > +    <p><span id="before"> of this paragraph should be displayed in green color.</span></p>
> 
> This is invalid markup by uniqueness of the HTML attribute id, <http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the-id-attribute>. That is, the HTML attribute id for the <span> must be unique, but it isn't. The first occurrence of this id was on line 34.
> 
> > LayoutTests/fast/css/first-line-style-for-before-after-content.html:43
> > +    <p><span id="after">Display </span></p>
> 
> This is invalid markup by uniqueness of the HTML attribute id, <http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the-id-attribute>. That is, the HTML attribute id for the <span> must be unique, but it isn't. The first occurrence of this id was on line 40.

I missed out on this. Have changed the id attr to class so as to make the style reusable by other elements.
Comment 29 Daniel Bates 2012-10-07 21:15:46 PDT
Comment on attachment 167477 [details]
Patch

Thanks Arpita for updating the patch.

r=me
Comment 30 WebKit Review Bot 2012-10-07 21:42:20 PDT
Comment on attachment 167477 [details]
Patch

Clearing flags on attachment: 167477

Committed r130616: <http://trac.webkit.org/changeset/130616>
Comment 31 WebKit Review Bot 2012-10-07 21:42:26 PDT
All reviewed patches have been landed.  Closing bug.