Bug 93925

Summary: :before/:after pseudo elements do not always apply to the proper element
Product: WebKit Reporter: Dylan <dylanryan>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, darin, eric, esprehn, hyatt, inferno, macpherson, menard, ojan.autocc, robert, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case for bug
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Dylan 2012-08-13 20:26:42 PDT
Couldn't think of a more descriptive title, so feel free to rename.

I have several web pages where I use css3 selectors based on &lt;a&gt; element href attributes to add a small icon before links (using :before pseudo-elements) to external sites. However, I have noticed that when multiple links to different sites (which would get different icons) are in the same block-level element, they all get the same icon. The test cast attached uses text instead, which shows the same behavior

The specific code I am using is a style sheet of the following form, but it does the same thing with :after as well as :before.

<code>
a:before{color:black}
a[href^="http://www.google.com"]:before{content:"Google"}
a[href^="http://www.wikipedia.com"]:before{content:"Wikipedia"}
</code>

And then in HTML, I have something like:

<code>
<p><a href="http://www.google.com">Google</a> <a href="http://www.wikipedia.com">Wikipedia</a></p>
</code>

What I expect to see is a link to Google, with the text Google, and the text Google prepended to it from the before (the link will be blue, the before will be in black). Likewise, for the wiki link, it should say Wikipedia twice. 

But what I see instead is that the wikipedia link ALSO has Google prepended to it, even though it does NOT match that style. However, if I wrap all the &lt;a&gt;'s in spans, then they all calculate correctly. Hovering over the links, however, displays the proper text. 

See attached file for test case. Have reproduced in Safari 6 on Lion, Chrome 21.x.x.x on both Lion and Ubuntu, and so I am guessing all platforms/OSs. I am not sure exactly WHEN this regressed, but it never happened to me in whatever the fully updated Safari 5.whatever was on lion (i had it fully updated, so whatever that was before the Safari 6 update). My friend with Chrome on Ubuntu thinks it broke several months ago but he can't be sure exactly when.


Code of the attached file, for those interested in reading it inline:

<code>

<!DOCTYPE html>
<html>
<head>
<style type="text/css">
	a:hover{color:red}
	a:before{color:black;}
	a[href^="http://www.google.com"]:before{content:"Google"}
	a[href^="http://www.wikipedia.com"]:before{content:"Wikipedia"}
	a[href^="http://www.apple.com"]:before{content:"Apple"}
</style>
<title>Test</title>
</head>
<body>
<p>1: Several links as children of the same p:<br><a href="http://www.google.com">Google</a> <a href="http://www.wikipedia.com">Wikipedia</a> <a href="http://www.apple.com">Apple</a> <a href="http://www.example.com">Example</a></p>
<p>2: Several links wrapped in spans, and the spans are all children of the same p:<br><span><a href="http://www.google.com">Google</a></span> <span><a href="http://www.wikipedia.com">Wikipedia</a></span> <span><a href="http://www.apple.com">Apple</a></span> <span><a href="http://www.example.com">Example</a></span></p>
</body>
</html>

</code>
Comment 1 Dylan 2012-08-13 20:29:18 PDT
Created attachment 158196 [details]
Test case for bug
Comment 2 Takashi Sakamoto 2012-08-31 00:01:30 PDT
Created attachment 161625 [details]
Patch
Comment 3 Darin Adler 2012-08-31 11:59:38 PDT
Comment on attachment 161625 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:2283
> +    // If we have first-letter, before or after pseudo style, do not share this style.
> +    if (style->hasPseudoStyle(FIRST_LETTER) || style->hasPseudoStyle(BEFORE) || style->hasPseudoStyle(AFTER))
>          style->setUnique();

Is removing the optimization the only practical way to fix this problem?
Comment 4 Takashi Sakamoto 2012-09-03 05:21:45 PDT
Created attachment 161903 [details]
Patch
Comment 5 Takashi Sakamoto 2012-09-03 05:22:30 PDT
Comment on attachment 161625 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/css/StyleResolver.cpp:2283
>>          style->setUnique();
> 
> Is removing the optimization the only practical way to fix this problem?

I see. I modified to canShareStyleWithElement to check whether pseudo style can be shared or not.
Comment 6 Takashi Sakamoto 2012-09-17 19:49:38 PDT
Created attachment 164480 [details]
Patch
Comment 7 Elliott Sprehn 2012-10-04 10:42:49 PDT
Comment on attachment 164480 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Disable sharing a style with sibligs if :after or :before pseudo style

sibligs => siblings

> Source/WebCore/rendering/style/RenderStyle.cpp:264
> +    if (!m_cachedPseudoStyles || !m_cachedPseudoStyles->size())

I don't think this size() check is needed since the loop would just never run. Can you remove it and combine this and the below if statement?
Comment 8 Takashi Sakamoto 2012-10-14 22:40:39 PDT
Created attachment 168626 [details]
Patch
Comment 9 Takashi Sakamoto 2012-10-14 22:41:33 PDT
Comment on attachment 164480 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/ChangeLog:8
>> +        Disable sharing a style with sibligs if :after or :before pseudo style
> 
> sibligs => siblings

Thanks. Done.

>> Source/WebCore/rendering/style/RenderStyle.cpp:264
>> +    if (!m_cachedPseudoStyles || !m_cachedPseudoStyles->size())
> 
> I don't think this size() check is needed since the loop would just never run. Can you remove it and combine this and the below if statement?

Sure. Done.
Comment 10 Eric Seidel (no email) 2013-01-04 00:51:09 PST
Attachment 168626 [details] was posted by a committer and has review+, assigning to Takashi Sakamoto for commit.
Comment 11 Takashi Sakamoto 2013-01-25 04:10:50 PST
Created attachment 184720 [details]
Patch
Comment 12 Takashi Sakamoto 2013-01-25 04:14:53 PST
Since a patch for Bug 101448 (Move childrenAffectedBy bits from RenderStyle to Element) was landed, renamed RenderStyle::pseudoStyleAffectedByUncommonAttributeSelectors to RenderStyle::hasUniquePseudoStyle.

Also updated StyleResolver.cpp to use hasUniquePseudoStyle instead of pseudoStyleAffectedByUncommonAttributeSelectors.
Comment 13 Takashi Sakamoto 2013-02-18 21:10:20 PST
Created attachment 188980 [details]
Patch for landing
Comment 14 Takashi Sakamoto 2013-02-18 21:12:21 PST
I would like to land this patch in a few days if there are no objections.
Comment 15 WebKit Review Bot 2013-02-18 23:33:25 PST
Comment on attachment 188980 [details]
Patch for landing

Clearing flags on attachment: 188980

Committed r143300: <http://trac.webkit.org/changeset/143300>
Comment 16 WebKit Review Bot 2013-02-18 23:33:29 PST
All reviewed patches have been landed.  Closing bug.