Bug 110825

Summary: [Shadow]: left side of ::-webkit-distributed selector not working as expected
Product: WebKit Reporter: Steve Orvell <sorvell>
Component: DOMAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, ericbidelman, esprehn+autocc, hayato, macpherson, menard, ojan.autocc, webcomponents-bugzilla, webkit.review.bot
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 59827    
Attachments:
Description Flags
reduction
none
Fix.
none
Addressed. none

Steve Orvell
Reported 2013-02-25 17:20:40 PST
Created attachment 190164 [details] reduction Styling distributed nodes with ::-webkit-distributed is working correctly when the left side of the expression is either content or * but not when it includes a class or attribute. For example, this is working: content::-webkit-distributed(*) { background: green; } but this is not: content.foo::-webkit-distributed(*) { background: green; }
Attachments
reduction (1.35 KB, text/html)
2013-02-25 17:20 PST, Steve Orvell 		no flags
Details
Fix. (8.78 KB, patch)
2013-03-12 01:54 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Addressed. (12.12 KB, patch)
2013-03-14 00:01 PDT, Hayato Ito 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hayato Ito
Comment 1 2013-03-12 01:54:43 PDT
Created attachment 192670 [details] Fix.
Dimitri Glazkov (Google)
Comment 2 2013-03-12 09:47:17 PDT
Comment on attachment 192670 [details] Fix. View in context: https://bugs.webkit.org/attachment.cgi?id=192670&action=review > Source/WebCore/css/CSSParser.cpp:11300 > + if (CSSParserSelector* distributedPseudoElementSelector = specifiers->findDistributedPseudoElementSelector()) { Sounds like we're calling findDistributedPseudoElementSelector twice: once in rewriteSpecifiersWithNamespaceIfNeeded(CSSParserSelector* specifiers), and once here. > Source/WebCore/css/CSSParser.cpp:11306 > + end->clearTagHistory(); Why are we chopping things off here? > Source/WebCore/css/CSSParserValues.cpp:234 > +CSSParserSelector* CSSParserSelector::findDistributedPseudoElementSelector() const Can you help me understand why we need to reach into to the selector history and look for this? Might be good to have a diagram in a ChangeLog too. > LayoutTests/fast/dom/shadow/distributed-pseudo-element-specifiers-in-left-side.html:12 > +shadowStyle.innerHTML = 'content.non-exist::-webkit-distributed(div) { color: red; }'; > +shadowStyle.innerHTML += 'content.content-class::-webkit-distributed(div) { color: green; }'; > +shadowStyle.innerHTML += '#content-id::-webkit-distributed(div) { color: blue; }'; This is pretty poor form of building a style string. It's not a big deal, but it's better to first build a string, then set textContent on shadowStyle.
Hayato Ito
Comment 3 2013-03-14 00:01:15 PDT
Created attachment 193078 [details] Addressed.
Hayato Ito
Comment 4 2013-03-14 00:05:46 PDT
Comment on attachment 192670 [details] Fix. Thank you for the review. View in context: https://bugs.webkit.org/attachment.cgi?id=192670&action=review >> Source/WebCore/css/CSSParser.cpp:11300 >> + if (CSSParserSelector* distributedPseudoElementSelector = specifiers->findDistributedPseudoElementSelector()) { > > Sounds like we're calling findDistributedPseudoElementSelector twice: once in rewriteSpecifiersWithNamespaceIfNeeded(CSSParserSelector* specifiers), and once here. I factored the functions so that we can avoid calling it twice. >> Source/WebCore/css/CSSParserValues.cpp:234 >> +CSSParserSelector* CSSParserSelector::findDistributedPseudoElementSelector() const > > Can you help me understand why we need to reach into to the selector history and look for this? Might be good to have a diagram in a ChangeLog too. Sure. That would be helpful since this kind of manipulation of CSSParserSelector is very tricky and hard to understand. I added an explanation to the ChangeLog. >> LayoutTests/fast/dom/shadow/distributed-pseudo-element-specifiers-in-left-side.html:12 >> +shadowStyle.innerHTML += '#content-id::-webkit-distributed(div) { color: blue; }'; > > This is pretty poor form of building a style string. It's not a big deal, but it's better to first build a string, then set textContent on shadowStyle. Ops. My bad. I didn't notice this before submitting the patch. It should be used only in debugging time. Done.
WebKit Review Bot
Comment 5 2013-03-14 19:04:32 PDT
Comment on attachment 193078 [details] Addressed. Clearing flags on attachment: 193078 Committed r145865: <http://trac.webkit.org/changeset/145865>
WebKit Review Bot
Comment 6 2013-03-14 19:04:37 PDT
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.