Bug 76611

Created attachment 123284 [details] Patch

Comment on attachment 123284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123284&action=review It looks this is rather complicated than we originally thought... > Source/WebCore/ChangeLog:8 > + Children of an element having a shadow root were to be attached in Element::attach(), How about to say "Each child" instead of "Children"? > Source/WebCore/ChangeLog:12 > + a HTMLContentElement must detach them before attaching them. a -> an > Source/WebCore/ChangeLog:13 > + The point here is that we've been attaching each light child twice when it has a parent with shadow root. > Source/WebCore/ChangeLog:17 > + No new tests, no change in behavior. You mentioned above that this changes the behavior... > Source/WebCore/dom/ShadowRoot.cpp:157 > + Element* host = shadowHost(); We don't need the |host| local variable. > Source/WebCore/html/shadow/HTMLContentElement.cpp:81 > + Seeing this, now I start wondering whether this is correct or not. What does happen if we remove HTMLContentElement from the shadow tree? In that case, we will detach the selected nodes regardless that they are staying in the light DOM. That looks wrong. But it's not clear what is the right thing to do here... Apparently we need to do something here. But maybe it's not dettach(). But I'm not sure.

I reuse this bug to track of adding/removing/replacing content element dynamically in a shadow tree. When removing a content element in a shadow tree, the elements included in the content element are not detached. So they are rendered, though they are light children not included in a content element. We should reconstruct shadow tree when adding/removing/replacing content element in shadow tree. To track which elements are attached or not, I want to have light tree to be managed by a shadow root.

This bug does not care about adding/removing/replacing nodes in light child. I will attack the bug in Bug 76262.

Created attachment 123920 [details] (Fake) Patch for shadow tree recalc

(In reply to comment #5) > Created an attachment (id=123920) [details] > (Fake) Patch for shadow tree recalc I wanted to do this, but this doesn't pass my tests.

Created attachment 123922 [details] Patch

(In reply to comment #7) > Created an attachment (id=123922) [details] > Patch This passes all my tests.

Comment on attachment 123922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123922&action=review > Source/WebCore/dom/ContainerNode.cpp:790 > +void ContainerNode::attachWithoutChildren() Could you inline this? > Source/WebCore/dom/ContainerNode.cpp:806 > +} And this? > Source/WebCore/dom/ContainerNode.h:78 > + void detachWithoutChildren(); "WithoutChildren" sounds a bit confusing as many negative-adjectives are. Maybe attachAsNode() ? > Source/WebCore/dom/ShadowRoot.h:75 > + bool m_needsShadowTreeStyleRecalc; Let's turn these boolean variables bit-fields.

Created attachment 124516 [details] Patch

(In reply to comment #9) > (From update of attachment 123922 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123922&action=review > > > Source/WebCore/dom/ContainerNode.cpp:790 > > +void ContainerNode::attachWithoutChildren() > > Could you inline this? > > > Source/WebCore/dom/ContainerNode.cpp:806 > > +} > > And this? Done. > > > Source/WebCore/dom/ContainerNode.h:78 > > + void detachWithoutChildren(); > > "WithoutChildren" sounds a bit confusing as many negative-adjectives are. Maybe attachAsNode() ? Done. > > > Source/WebCore/dom/ShadowRoot.h:75 > > + bool m_needsShadowTreeStyleRecalc; > > Let's turn these boolean variables bit-fields. Done.

Comment on attachment 124516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124516&action=review > Source/WebCore/dom/Element.cpp:945 > + // When a shadow root exists, attaching all children is due for the shadow root. Did you mean to say: "When a shadow root exists, it does the work of attaching the children." perhaps? Otherwise, the comment is a bit unclear. > Source/WebCore/dom/Element.cpp:947 > + ContainerNode::attachAsNode(); Can you not just call Node::attach() here? > Source/WebCore/dom/Element.cpp:978 > + ContainerNode::detachAsNode(); Node::detach()? > Source/WebCore/dom/ShadowRoot.h:46 > + void setNeedsShadowTreeStyleRecalc() { m_needsShadowTreeStyleRecalc = true; } > + void clearNeedsShadowTreeStyleRecalc() { m_needsShadowTreeStyleRecalc = false; } > + bool needsShadowTreeStyleRecalc() { return m_needsShadowTreeStyleRecalc; } Usually, we implement inlines as full class function definition right below the class declaration.

Created attachment 124654 [details] Patch

(In reply to comment #12) > (From update of attachment 124516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124516&action=review > > > Source/WebCore/dom/Element.cpp:945 > > + // When a shadow root exists, attaching all children is due for the shadow root. > > Did you mean to say: "When a shadow root exists, it does the work of attaching the children." perhaps? Otherwise, the comment is a bit unclear. Done. > > > Source/WebCore/dom/Element.cpp:947 > > + ContainerNode::attachAsNode(); > > Can you not just call Node::attach() here? > > > Source/WebCore/dom/Element.cpp:978 > > + ContainerNode::detachAsNode(); > > Node::detach()? Done. > > > Source/WebCore/dom/ShadowRoot.h:46 > > + void setNeedsShadowTreeStyleRecalc() { m_needsShadowTreeStyleRecalc = true; } > > + void clearNeedsShadowTreeStyleRecalc() { m_needsShadowTreeStyleRecalc = false; } > > + bool needsShadowTreeStyleRecalc() { return m_needsShadowTreeStyleRecalc; } > > Usually, we implement inlines as full class function definition right below the class declaration. Done.

Created attachment 124705 [details] Patch

Attachment 124705 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit 148477e..09fbdf8 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 106352 = 148477e5d717c315b77d4ba9b95d975c9bae1c82 r106353 = c3d2bcbb9a713bb3fe06174be7b46d706f6ae407 r106354 = 09fbdf8bc1525cf6c284e230088f23103db232bd Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124705&action=review > Source/WebCore/ChangeLog:19 > + Attaches only child elements if they are not attached. "only" is in the wrong place here. Should be "Attaches child elements only if they are not attached". > Source/WebCore/ChangeLog:21 > + Detaches only child elements if they are attached. ditto. > Source/WebCore/dom/Element.cpp:950 > + attachChildrenIfNotAttached(); Why are we attaching children anyway here? Can you explain? > Source/WebCore/dom/Element.cpp:976 > +void Element::attachChildrenIfNotAttached() > +{ > + for (Node* child = firstChild(); child; child = child->nextSibling()) { > + if (!child->attached()) > + child->attach(); > + } > +} Unless we still need this callsite above, we can probably fold this into reattachChildrenAndShadow() > Source/WebCore/dom/Element.cpp:998 > +void Element::detachChildrenIfAttached() > +{ > + for (Node* child = firstChild(); child; child = child->nextSibling()) { > + if (child->attached()) > + child->detach(); > + } > +} Ditto. While it's nice to break things up into the functions, the API surface of Element is already ginormous. Perhaps a static local function? > Source/WebCore/dom/ShadowRoot.cpp:104 > + host()->reattachChildrenAndShadow(); reattachChildrenAndShadow() has one callsite and when invoked, reaches back to grab its callee. This seems weird. Can we just unroll this here? Or perhaps make it a local function?

Created attachment 124862 [details] Patch

(In reply to comment #17) > (From update of attachment 124705 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124705&action=review > > > Source/WebCore/ChangeLog:19 > > + Attaches only child elements if they are not attached. > > "only" is in the wrong place here. Should be "Attaches child elements only if they are not attached". > > > Source/WebCore/ChangeLog:21 > > + Detaches only child elements if they are attached. > > ditto. They were removed. > > > Source/WebCore/dom/Element.cpp:950 > > + attachChildrenIfNotAttached(); > > Why are we attaching children anyway here? Can you explain? I've added comments in the patch. We have to attach children. Some of children are attached in shadow tree (in content element), but there might be unattached children. So attach them here anyway. > > > Source/WebCore/dom/Element.cpp:976 > > +void Element::attachChildrenIfNotAttached() > > +{ > > + for (Node* child = firstChild(); child; child = child->nextSibling()) { > > + if (!child->attached()) > > + child->attach(); > > + } > > +} > > Unless we still need this callsite above, we can probably fold this into reattachChildrenAndShadow() Done. > > > Source/WebCore/dom/Element.cpp:998 > > +void Element::detachChildrenIfAttached() > > +{ > > + for (Node* child = firstChild(); child; child = child->nextSibling()) { > > + if (child->attached()) > > + child->detach(); > > + } > > +} > > Ditto. While it's nice to break things up into the functions, the API surface of Element is already ginormous. Perhaps a static local function? Done. > > > Source/WebCore/dom/ShadowRoot.cpp:104 > > + host()->reattachChildrenAndShadow(); > > reattachChildrenAndShadow() has one callsite and when invoked, reaches back to grab its callee. This seems weird. Can we just unroll this here? Or perhaps make it a local function? Done.

Comment on attachment 124862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124862&action=review > Source/WebCore/dom/Element.cpp:953 > + // In a shadow tree, some of light children may be attached by 'content' element. > + // However, when there is no content element or content element does not select > + // all light children, we have to attach the rest of light children here. Won't this cause the children to appear in rendering? This seems wrong.

(In reply to comment #20) > (From update of attachment 124862 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124862&action=review > > > Source/WebCore/dom/Element.cpp:953 > > + // In a shadow tree, some of light children may be attached by 'content' element. > > + // However, when there is no content element or content element does not select > > + // all light children, we have to attach the rest of light children here. > > Won't this cause the children to appear in rendering? This seems wrong. No, the children won't appear in rendering. In WebCore::NodeRenderingContext, rendering of these elements are skipped. You can find the code to skip them if m_phase is AttachContentLight. And let me mention that actually the light children have been attached before this change.

> And let me mention that actually the light children have been attached before this change. This means ContainerNode::attach(). It attaches all the children also even if they are light children.

(In reply to comment #22) > > And let me mention that actually the light children have been attached before this change. > > This means ContainerNode::attach(). It attaches all the children also even if they are light children. Ohhh, I remember now. Apologies.

Comment on attachment 124862 [details] Patch Clearing flags on attachment: 124862 Committed r106432: <http://trac.webkit.org/changeset/106432>

All reviewed patches have been landed. Closing bug.

Comment on attachment 124862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124862&action=review > Source/WebCore/dom/Element.cpp:947 > + Node::attach(); Wow, here bug is. This should be after parentPusher.push(). Could you write a test to capture this? > Source/WebCore/dom/ShadowRoot.cpp:177 > + for (Node* child = host()->firstChild(); child; child = child->nextSibling()) { Could you cache host() value for efficiency? > Source/WebCore/dom/ShadowRoot.h:77 > + bool m_needsShadowTreeStyleRecalc : 1; This flag name looks confusing. What we need is not only recalculating style, but also invoking reattach. The name should imply the fact. > Source/WebCore/dom/ShadowRoot.h:98 > + return m_needsShadowTreeStyleRecalc; hasContentElement() check could be moved here. > Source/WebCore/html/shadow/HTMLContentElement.cpp:99 > + if (root->shadowHost()) This change is also can be moved to the flag setter.

Since rolled out, reopen.

Created attachment 124924 [details] Patch

Comment on attachment 124924 [details] Patch Clearing flags on attachment: 124924 Committed r106465: <http://trac.webkit.org/changeset/106465>

All reviewed patches have been landed. Closing bug.