93925 – :before/:after pseudo elements do not always apply to the proper element

RESOLVED FIXED 93925

:before/:after pseudo elements do not always apply to the proper element

https://bugs.webkit.org/show_bug.cgi?id=93925

Summary :before/:after pseudo elements do not always apply to the proper element

Dylan

Reported 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 <a> 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> <a href="http://www.google.com">Google</a> <a href="http://www.wikipedia.com">Wikipedia</a> </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 <a>'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> 1: Several links as children of the same p: <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> 2: Several links wrapped in spans, and the spans are all children of the same p: <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> </body> </html> </code>

Attachments
Test case for bug (892 bytes, text/html) 2012-08-13 20:29 PDT, Dylan	no flags	Details
Patch (4.37 KB, patch) 2012-08-31 00:01 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (5.85 KB, patch) 2012-09-03 05:21 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (5.77 KB, patch) 2012-09-17 19:49 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (5.74 KB, patch) 2012-10-14 22:40 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (5.48 KB, patch) 2013-01-25 04:10 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch for landing (5.52 KB, patch) 2013-02-18 21:10 PST, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Dylan

Comment 1 2012-08-13 20:29:18 PDT

Created attachment 158196 [details] Test case for bug

Takashi Sakamoto

Comment 2 2012-08-31 00:01:30 PDT

Created attachment 161625 [details] Patch

Darin Adler

Comment 3 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?

Takashi Sakamoto

Comment 4 2012-09-03 05:21:45 PDT

Created attachment 161903 [details] Patch

Takashi Sakamoto

Comment 5 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.

Takashi Sakamoto

Comment 6 2012-09-17 19:49:38 PDT

Created attachment 164480 [details] Patch

Elliott Sprehn

Comment 7 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?

Takashi Sakamoto

Comment 8 2012-10-14 22:40:39 PDT

Created attachment 168626 [details] Patch

Takashi Sakamoto

Comment 9 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.

Eric Seidel (no email)

Comment 10 2013-01-04 00:51:09 PST

Attachment 168626 [details] was posted by a committer and has review+, assigning to Takashi Sakamoto for commit.

Takashi Sakamoto

Comment 11 2013-01-25 04:10:50 PST

Created attachment 184720 [details] Patch

Takashi Sakamoto

Comment 12 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.

Takashi Sakamoto

Comment 13 2013-02-18 21:10:20 PST

Created attachment 188980 [details] Patch for landing

Takashi Sakamoto

Comment 14 2013-02-18 21:12:21 PST

I would like to land this patch in a few days if there are no objections.

WebKit Review Bot

Comment 15 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>

WebKit Review Bot

Comment 16 2013-02-18 23:33:29 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component CSS

Assignee

Takashi Sakamoto

Reported

2012-08-13 20:26 PDT

Modified

2013-02-18 23:33 PST History

CC List

13 users Show

URL

Keywords

Depends on

Blocks