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; }
Created attachment 192670 [details] Fix.
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.
Created attachment 193078 [details] Addressed.
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.
Comment on attachment 193078 [details] Addressed. Clearing flags on attachment: 193078 Committed r145865: <http://trac.webkit.org/changeset/145865>
All reviewed patches have been landed. Closing bug.