Shadow DOM styles appear to be over-eagerly shared

[Dimitri Glazkov (Google)]

Check out the example in URL. BAR, the next sibling of FOO that has shadow DOM appears to acquire the same styles as FOO. That seems bad.

[Takashi Sakamoto]

I think, StyleResolver::canShareStyleWithElement should check whether a given element is distributed or not.

[Takashi Sakamoto]

Or modify StyleResolver::locateCousinList not to return children of a shadow host.

[Takashi Sakamoto]

Created attachment 190172 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 190172 [details]
Patch

Or just modifying WebCore::StyleResolver::locateCousinList,

line 887:
            if (currentNode->renderStyle() == parentStyle && currentNode->lastC\
hild()
!               && currentNode->isElementNode() && !parentElementPreventsSharin\
g(toElement(currentNode))) {
---
!                && currentNode->isElementNode() && !parentElementPreventsSharin\
g(toElement(currentNode))
+               && !toElement(currentNode)->shadow()) {

[Takashi Sakamoto]

Created attachment 190178 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 190178 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1102
> +    if (element->parentElement() && element->parentElement()->shadow() && element->parentElement()->shadow()->distributor().findInsertionPointFor(element))

Isn't this super-slow? We need to keep an eye out for better-performing way of doing this.

[Takashi Sakamoto]

Created attachment 190193 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 190178 [details]
Patch

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

>> Source/WebCore/css/StyleResolver.cpp:1102
>> +    if (element->parentElement() && element->parentElement()->shadow() && element->parentElement()->shadow()->distributor().findInsertionPointFor(element))
> 
> Isn't this super-slow? We need to keep an eye out for better-performing way of doing this.

Yeah, I think, we don't need to check whether the element is really distributed or not.
I moved the logic to locateCousinList and just checked whether a given element is a shadow host or not.

[Takashi Sakamoto]

Comment on attachment 190178 [details]
Patch

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

>>> Source/WebCore/css/StyleResolver.cpp:1102
>>> +    if (element->parentElement() && element->parentElement()->shadow() && element->parentElement()->shadow()->distributor().findInsertionPointFor(element))
>> 
>> Isn't this super-slow? We need to keep an eye out for better-performing way of doing this.
> 
> Yeah, I think, we don't need to check whether the element is really distributed or not.
> I moved the logic to locateCousinList and just checked whether a given element is a shadow host or not.

I mean,
- if a given element is distributed, we cannot share the element's style.
- if a given element is not distributed but a child of a shadow host, we don't render the element and we don't have the element's render style.

[Takashi Sakamoto]

Comment on attachment 190193 [details]
Patch

Thank you for reviewing.

[WebKit Review Bot]

Comment on attachment 190193 [details]
Patch

Clearing flags on attachment: 190193

Committed r144031: <http://trac.webkit.org/changeset/144031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.