Content element should be able to be dynamically added/removed/replaced in a shadow tree.

• Component: DOM
• Status: RESOLVED FIXED

[Shinya Kawanaka]

When a shadow root exists in an element, the order of attach is:
  (1) attaches all children of the element
  (2) attaches shadow root.

When the shadow tree has a content element, the elements selected by the content element is re-attached in the content element.
Maybe the light children of elements having a shadow tree should be attached only in the shadow tree.

[Shinya Kawanaka]

Created attachment 123284 [details]
Patch

[Hajime Morrita]

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.

[Shinya Kawanaka]

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.

[Shinya Kawanaka]

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

[Shinya Kawanaka]

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

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 123922 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #7)
> Created an attachment (id=123922) [details]
> Patch

This passes all my tests.

[Hajime Morrita]

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.

[Shinya Kawanaka]

Created attachment 124516 [details]
Patch

[Shinya Kawanaka]

(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.

[Dimitri Glazkov (Google)]

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.

[Shinya Kawanaka]

Created attachment 124654 [details]
Patch

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

Created attachment 124705 [details]
Patch

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

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?

[Shinya Kawanaka]

Created attachment 124862 [details]
Patch

[Shinya Kawanaka]

(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.

[Dimitri Glazkov (Google)]

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.

[Shinya Kawanaka]

(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.

[Shinya Kawanaka]

> 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.

[Dimitri Glazkov (Google)]

(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.

[WebKit Review Bot]

Comment on attachment 124862 [details]
Patch

Clearing flags on attachment: 124862

Committed r106432: <http://trac.webkit.org/changeset/106432>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hajime Morrita]

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.

[Shinya Kawanaka]

Since rolled out, reopen.

[Shinya Kawanaka]

Created attachment 124924 [details]
Patch

[WebKit Review Bot]

Comment on attachment 124924 [details]
Patch

Clearing flags on attachment: 124924

Committed r106465: <http://trac.webkit.org/changeset/106465>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.