Bug 90661

Summary: Distributed nodes should not share styles.
Product: WebKit Reporter: Takashi Sakamoto <tasak>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, hayato, macpherson, menard, morrita, tasak, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 90017    
Attachments:
Description Flags
repro
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Takashi Sakamoto
Reported 2012-07-06 00:28:15 PDT
Created attachment 151026 [details] repro According to Shadow DOM spec, the child nodes of the shadow host that are assigned to the insertion point should inherit the styles from the parent of the insertion point. However sometimes such a child node don't inherit style from the parent of the insertion point. The following is a figure showing a DOM tree created in repro.html: div | +--div[id=host, style=color:red] ....... shadow-root | +---div[id=child-a] | +---div[id=child-b] shadow-root | +--- content[select='#child-a'] | +--- div[style=color:blue] | +---content[select='#child-b'] window.getComputedStyle(document.getElementById('child-b')).color should be 'rgb(0, 0, 255)', but 'rgb(255, 0, 0)'.
Attachments
repro (1.06 KB, text/html)
2012-07-06 00:28 PDT, Takashi Sakamoto 		no flags
Details
Patch (8.57 KB, patch)
2012-07-06 05:20 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (7.79 KB, patch)
2012-07-08 23:41 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (8.55 KB, patch)
2012-07-09 01:20 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (7.61 KB, patch)
2012-07-24 21:45 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (8.19 KB, patch)
2012-08-15 00:15 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (8.20 KB, patch)
2012-08-20 21:27 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (8.91 KB, patch)
2012-08-21 22:11 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (8.91 KB, patch)
2012-08-21 22:15 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (8.85 KB, patch)
2012-08-23 01:44 PDT, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Takashi Sakamoto
Comment 1 2012-07-06 05:20:58 PDT
Created attachment 151063 [details] Patch
Hayato Ito
Comment 2 2012-07-06 05:48:48 PDT
Comment on attachment 151063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151063&action=review > LayoutTests/fast/dom/shadow/user-modify-inheritance.html:65 > +shouldBeEqualToString('getUserModifyProperty("child-b")', 'read-only'); This is confusing since it should be 'read-write', not 'read-only', from the view of editing. Since this patch fixes an 'unexpected pass' here, the test should 'FAIL' for child-b. We should add 'FIXME' comment here with an appropriate bug id.
Takashi Sakamoto
Comment 3 2012-07-08 23:41:48 PDT
Created attachment 151192 [details] Patch
Takashi Sakamoto
Comment 4 2012-07-08 23:44:17 PDT
Comment on attachment 151192 [details] Patch I forgot to add "FIXME".
Takashi Sakamoto
Comment 5 2012-07-09 01:20:38 PDT
Created attachment 151208 [details] Patch
Takashi Sakamoto
Comment 6 2012-07-09 01:25:57 PDT
(In reply to comment #2) > (From update of attachment 151063 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151063&action=review > > > LayoutTests/fast/dom/shadow/user-modify-inheritance.html:65 > > +shouldBeEqualToString('getUserModifyProperty("child-b")', 'read-only'); > > This is confusing since it should be 'read-write', not 'read-only', from the view of editing. > Since this patch fixes an 'unexpected pass' here, the test should 'FAIL' for child-b. > We should add 'FIXME' comment here with an appropriate bug id. Done, but I reused bug 90017 for the FIXME. Best regards, Takashi Sakamoto
Dimitri Glazkov (Google)
Comment 7 2012-07-16 16:27:55 PDT
Comment on attachment 151208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151208&action=review > Source/WebCore/ChangeLog:3 > + [Shadow] distribute nodes don't inherit styles from parentNodeForRenderingAndStyle. distribute -> Distributed Perhaps a better title for this bug would be: Distributed nodes should not share styles > Source/WebCore/css/StyleResolver.h:485 > + bool m_elementDistributed; Do we really need a member here? Can we not infer this information somehow from parentStyle? I am not sure, just asking. Also -- if we do, the name needs to be more descriptive. It's hard to understand for someone who doesn't normally deal with Shadow DOM terminology. Can this be just a bit flag for disabling style sharing? Will this produce better code?
Takashi Sakamoto
Comment 8 2012-07-24 21:45:06 PDT
Created attachment 154245 [details] Patch
Takashi Sakamoto
Comment 9 2012-07-24 21:55:02 PDT
Thank you for reviewing. (In reply to comment #7) > (From update of attachment 151208 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151208&action=review > > > Source/WebCore/ChangeLog:3 > > + [Shadow] distribute nodes don't inherit styles from parentNodeForRenderingAndStyle. > > distribute -> Distributed > > Perhaps a better title for this bug would be: Distributed nodes should not share styles I see. Done. > > Source/WebCore/css/StyleResolver.h:485 > > + bool m_elementDistributed; > > Do we really need a member here? Can we not infer this information somehow from parentStyle? I am not sure, just asking. > > Also -- if we do, the name needs to be more descriptive. It's hard to understand for someone who doesn't normally deal with Shadow DOM terminology. Can this be just a bit flag for disabling style sharing? Will this produce better code? I see. I changed the code to use ElementShadow::insertionPointFor (i.e. not using NodeRenderingContext). As the NodeRenderingContext (and ComposedTreeWalker) is a little slow, I would like to avoid using the API again. However, ElementShadow::insertionPointFor is not as slow as ComposedTreeWalker (I think). So I can remove the member. I also moved the code from StyleResolver::locateSharedStyle to StyleResolver::styleForElement. Because I'm thinking of re-using the condition for another purpose, i.e. if an element is distributed, "user-modify" is inherited from dom parent (not rendering parent). Best regards, Takashi Sakamoto
Dimitri Glazkov (Google)
Comment 10 2012-08-10 09:34:55 PDT
Comment on attachment 154245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154245&action=review > Source/WebCore/css/StyleResolver.cpp:1717 > + bool elementIsDistributed = false; How about elementIsDistributed -> distributedToInsertionPoint? This may be a bit more clear to non-shadow DOM users. > Source/WebCore/css/StyleResolver.cpp:1720 > + if (Element* parentElement = element->parentElement()) > + if (ElementShadow* shadow = parentElement->shadow()) > + elementIsDistributed = !!shadow->insertionPointFor(element); Since this is only checked once, it might be better to just move it to a static free-standing function that is only called when the first part of the condition below is true.
Takashi Sakamoto
Comment 11 2012-08-15 00:15:49 PDT
Created attachment 158514 [details] Patch
Takashi Sakamoto
Comment 12 2012-08-15 02:03:36 PDT
Comment on attachment 154245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154245&action=review Thank you for reviewing. Best regards, Takashi Sakamoto >> Source/WebCore/css/StyleResolver.cpp:1717 >> + bool elementIsDistributed = false; > > How about elementIsDistributed -> distributedToInsertionPoint? > > This may be a bit more clear to non-shadow DOM users. Done. >> Source/WebCore/css/StyleResolver.cpp:1720 >> + elementIsDistributed = !!shadow->insertionPointFor(element); > > Since this is only checked once, it might be better to just move it to a static free-standing function that is only called when the first part of the condition below is true. I see. Done.
Takashi Sakamoto
Comment 13 2012-08-20 21:27:02 PDT
Created attachment 159616 [details] Patch
Hajime Morrita
Comment 14 2012-08-20 21:55:02 PDT
Sorry for my slow jumping in but - (In reply to comment #9) > > > Source/WebCore/css/StyleResolver.h:485 > > > + bool m_elementDistributed; > > > > Do we really need a member here? Can we not infer this information somehow from parentStyle? I am not sure, just asking. > > I see. > I changed the code to use ElementShadow::insertionPointFor (i.e. not using NodeRenderingContext). > > As the NodeRenderingContext (and ComposedTreeWalker) is a little slow, I would like to avoid using the API again. However, ElementShadow::insertionPointFor is not as slow as ComposedTreeWalker (I think). So I can remove the member. > Both insertionPointFor() and Element::shadow() needs one hashtable lookup for each. Considering that shared style is a "fast path" and style recalculation is one of the hottest place in WebKit land, I'd rather use what NodeRenderingContext told us than duplicate composed tree traversal logic there.
Dimitri Glazkov (Google)
Comment 15 2012-08-21 17:15:24 PDT
(In reply to comment #14) > Both insertionPointFor() and Element::shadow() needs one hashtable lookup for each. Considering that shared style is a "fast path" and style recalculation is one of the hottest place in WebKit land, I'd rather use what NodeRenderingContext told us than duplicate composed tree traversal logic there. Sounds good.
Takashi Sakamoto
Comment 16 2012-08-21 22:10:47 PDT
(In reply to comment #14) > Sorry for my slow jumping in but - > > (In reply to comment #9) > > > > Source/WebCore/css/StyleResolver.h:485 > > > > + bool m_elementDistributed; > > > > > > Do we really need a member here? Can we not infer this information somehow from parentStyle? I am not sure, just asking. > > > > I see. > > I changed the code to use ElementShadow::insertionPointFor (i.e. not using NodeRenderingContext). > > > > As the NodeRenderingContext (and ComposedTreeWalker) is a little slow, I would like to avoid using the API again. However, ElementShadow::insertionPointFor is not as slow as ComposedTreeWalker (I think). So I can remove the member. > > > Both insertionPointFor() and Element::shadow() needs one hashtable lookup for each. Considering that shared style is a "fast path" and style recalculation is one of the hottest place in WebKit land, I'd rather use what NodeRenderingContext told us than duplicate composed tree traversal logic there. I see. I think, this is done by my fist patch. So I modified my first patch (changing the name of the new member variable) and uploaded again. Best regards, Takashi Sakamoto
Takashi Sakamoto
Comment 17 2012-08-21 22:11:52 PDT
Created attachment 159858 [details] Patch
Takashi Sakamoto
Comment 18 2012-08-21 22:15:41 PDT
Created attachment 159859 [details] Patch
Hajime Morrita
Comment 19 2012-08-21 22:59:53 PDT
Comment on attachment 159859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159859&action=review Looks good. Please take this small fix. > LayoutTests/fast/dom/shadow/style-of-distributed-node.html:29 > +div.innerHTML = '<div id="child-a"></div><div id="child-b"></div>'; You can put this into markup instead of programatically.
Takashi Sakamoto
Comment 20 2012-08-23 01:44:45 PDT
Created attachment 160115 [details] Patch
WebKit Review Bot
Comment 21 2012-08-23 10:07:58 PDT
Comment on attachment 160115 [details] Patch Clearing flags on attachment: 160115 Committed r126442: <http://trac.webkit.org/changeset/126442>
WebKit Review Bot
Comment 22 2012-08-23 10:08:03 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.