[Shadow]: left side of ::-webkit-distributed selector not working as expected

• Component: DOM
• Status: RESOLVED FIXED

[Steve Orvell]

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;
}

[Hayato Ito]

Created attachment 192670 [details]
Fix.

[Dimitri Glazkov (Google)]

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]

Created attachment 193078 [details]
Addressed.

[Hayato Ito]

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 on attachment 193078 [details]
Addressed.

Clearing flags on attachment: 193078

Committed r145865: <http://trac.webkit.org/changeset/145865>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.