Bug 110797

Summary: Shadow DOM styles appear to be over-eagerly shared
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, esprehn+autocc, esprehn, macpherson, menard, ojan.autocc, tasak, webcomponents-bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://jsfiddle.net/AwXD8/
Bug Depends on:    
Bug Blocks: 72352    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Dimitri Glazkov (Google)
Reported 2013-02-25 14:11:44 PST
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.
Attachments
Patch (4.75 KB, patch)
2013-02-25 18:18 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (4.78 KB, patch)
2013-02-25 18:29 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (5.01 KB, patch)
2013-02-25 20:20 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Takashi Sakamoto
Comment 1 2013-02-25 15:34:05 PST
I think, StyleResolver::canShareStyleWithElement should check whether a given element is distributed or not.
Takashi Sakamoto
Comment 2 2013-02-25 15:39:10 PST
Or modify StyleResolver::locateCousinList not to return children of a shadow host.
Takashi Sakamoto
Comment 3 2013-02-25 18:18:13 PST
Created attachment 190172 [details] Patch
Takashi Sakamoto
Comment 4 2013-02-25 18:21:38 PST
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
Comment 5 2013-02-25 18:29:45 PST
Created attachment 190178 [details] Patch
Dimitri Glazkov (Google)
Comment 6 2013-02-25 19:02:23 PST
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
Comment 7 2013-02-25 20:20:52 PST
Created attachment 190193 [details] Patch
Takashi Sakamoto
Comment 8 2013-02-25 20:22:58 PST
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 9 2013-02-25 20:26:48 PST
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 10 2013-02-25 20:27:43 PST
Comment on attachment 190193 [details] Patch Thank you for reviewing.
WebKit Review Bot
Comment 11 2013-02-26 02:53:49 PST
Comment on attachment 190193 [details] Patch Clearing flags on attachment: 190193 Committed r144031: <http://trac.webkit.org/changeset/144031>
WebKit Review Bot
Comment 12 2013-02-26 02:53:54 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.