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)'.
Created attachment 151063 [details] Patch
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.
Created attachment 151192 [details] Patch
Comment on attachment 151192 [details] Patch I forgot to add "FIXME".
Created attachment 151208 [details] Patch
(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
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?
Created attachment 154245 [details] Patch
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
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.
Created attachment 158514 [details] Patch
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.
Created attachment 159616 [details] Patch
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.
(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.
(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
Created attachment 159858 [details] Patch
Created attachment 159859 [details] Patch
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.
Created attachment 160115 [details] Patch
Comment on attachment 160115 [details] Patch Clearing flags on attachment: 160115 Committed r126442: <http://trac.webkit.org/changeset/126442>
All reviewed patches have been landed. Closing bug.