Bug 80794 – :first-line pseudo selector ignoring words created from :before

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Marcos Zanona]

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.

[Arpita Bahuguna]

Created attachment 147269 [details]
Patch

[Arpita Bahuguna]

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.

[Robert Hogan]

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?

[Arpita Bahuguna]

Created attachment 151665 [details]
Patch

[Arpita Bahuguna]

(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. :)

[Eric Seidel (no email)]

Thoughts, elliot?

[Elliott Sprehn]

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.

[Elliott Sprehn]

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.

[Arpita Bahuguna]

Created attachment 163788 [details]
Patch

[Arpita Bahuguna]

(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.

[Elliott Sprehn]

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?

[Elliott Sprehn]

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?

[Elliott Sprehn]

(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.

[Daniel Bates]

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.

[Arpita Bahuguna]

Created attachment 166838 [details]
Patch

[Elliott Sprehn]

Comment on attachment 166838 [details]
Patch

Looks good to me, but I'm not a reviewer so someone else would need to r+ :)

[Arpita Bahuguna]

(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!

[Arpita Bahuguna]

(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.

[Daniel Bates]

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?

[Arpita Bahuguna]

Created attachment 167070 [details]
Patch

[Arpita Bahuguna]

Created attachment 167255 [details]
Patch

[Arpita Bahuguna]

(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.

[Elliott Sprehn]

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)

[Arpita Bahuguna]

(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.

[Elliott Sprehn]

(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!

[Daniel Bates]

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.

[Arpita Bahuguna]

Created attachment 167477 [details]
Patch

[Arpita Bahuguna]

(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.

[Daniel Bates]

Comment on attachment 167477 [details]
Patch

Thanks Arpita for updating the patch.

r=me

[WebKit Review Bot]

Comment on attachment 167477 [details]
Patch

Clearing flags on attachment: 167477

Committed r130616: <http://trac.webkit.org/changeset/130616>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.