90661 – Distributed nodes should not share styles.

RESOLVED FIXED 90661

Distributed nodes should not share styles.

https://bugs.webkit.org/show_bug.cgi?id=90661

Summary Distributed nodes should not share styles.

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	Details Formatted Diff Diff
Patch (7.79 KB, patch) 2012-07-08 23:41 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.55 KB, patch) 2012-07-09 01:20 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (7.61 KB, patch) 2012-07-24 21:45 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.19 KB, patch) 2012-08-15 00:15 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.20 KB, patch) 2012-08-20 21:27 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.91 KB, patch) 2012-08-21 22:11 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.91 KB, patch) 2012-08-21 22:15 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.85 KB, patch) 2012-08-23 01:44 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Takashi Sakamoto

Reported

2012-07-06 00:28 PDT

Modified

2012-08-23 10:08 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks

90017

Dependencies

tree graph