Bug 76611 - Content element should be able to be dynamically added/removed/replaced in a shadow tree.
Summary: Content element should be able to be dynamically added/removed/replaced in a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 76262 77529
Blocks: 75301
  Show dependency treegraph
 
Reported: 2012-01-19 01:56 PST by Shinya Kawanaka
Modified: 2012-02-01 05:50 PST (History)
7 users (show)

See Also:


Attachments
Patch (7.81 KB, patch)
2012-01-20 04:08 PST, Shinya Kawanaka
morrita: review-
Details | Formatted Diff | Diff
(Fake) Patch for shadow tree recalc (2.30 KB, patch)
2012-01-25 04:22 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (26.60 KB, patch)
2012-01-25 04:27 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (25.88 KB, patch)
2012-01-30 03:31 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (25.42 KB, patch)
2012-01-30 18:41 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (26.26 KB, patch)
2012-01-31 04:09 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (25.36 KB, patch)
2012-01-31 17:54 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (25.89 KB, patch)
2012-02-01 03:58 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-01-19 01:56:25 PST
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.
Comment 1 Shinya Kawanaka 2012-01-20 04:08:13 PST
Created attachment 123284 [details]
Patch
Comment 2 Hajime Morrita 2012-01-20 08:45:05 PST
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.
Comment 3 Shinya Kawanaka 2012-01-25 01:49:23 PST
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.
Comment 4 Shinya Kawanaka 2012-01-25 01:51:14 PST
This bug does not care about adding/removing/replacing nodes in light child. I will attack the bug in Bug 76262.
Comment 5 Shinya Kawanaka 2012-01-25 04:22:32 PST
Created attachment 123920 [details]
(Fake) Patch for shadow tree recalc
Comment 6 Shinya Kawanaka 2012-01-25 04:24:56 PST
(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.
Comment 7 Shinya Kawanaka 2012-01-25 04:27:00 PST
Created attachment 123922 [details]
Patch
Comment 8 Shinya Kawanaka 2012-01-25 04:31:12 PST
(In reply to comment #7)
> Created an attachment (id=123922) [details]
> Patch

This passes all my tests.
Comment 9 Hajime Morrita 2012-01-29 20:16:24 PST
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.
Comment 10 Shinya Kawanaka 2012-01-30 03:31:24 PST
Created attachment 124516 [details]
Patch
Comment 11 Shinya Kawanaka 2012-01-30 03:36:20 PST
(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 12 Dimitri Glazkov (Google) 2012-01-30 09:36:44 PST
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.
Comment 13 Shinya Kawanaka 2012-01-30 18:41:33 PST
Created attachment 124654 [details]
Patch
Comment 14 Shinya Kawanaka 2012-01-30 18:42:08 PST
(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.
Comment 15 Shinya Kawanaka 2012-01-31 04:09:44 PST
Created attachment 124705 [details]
Patch
Comment 16 WebKit Review Bot 2012-01-31 04:12:44 PST
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 17 Dimitri Glazkov (Google) 2012-01-31 10:13:28 PST
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?
Comment 18 Shinya Kawanaka 2012-01-31 17:54:29 PST
Created attachment 124862 [details]
Patch
Comment 19 Shinya Kawanaka 2012-01-31 18:02:05 PST
(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 20 Dimitri Glazkov (Google) 2012-01-31 19:28:52 PST
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.
Comment 21 Shinya Kawanaka 2012-01-31 20:02:32 PST
(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.
Comment 22 Shinya Kawanaka 2012-01-31 20:03:52 PST
> 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.
Comment 23 Dimitri Glazkov (Google) 2012-01-31 20:06:27 PST
(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 24 WebKit Review Bot 2012-01-31 22:39:25 PST
Comment on attachment 124862 [details]
Patch

Clearing flags on attachment: 124862

Committed r106432: <http://trac.webkit.org/changeset/106432>
Comment 25 WebKit Review Bot 2012-01-31 22:39:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Hajime Morrita 2012-02-01 02:06:43 PST
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.
Comment 27 Shinya Kawanaka 2012-02-01 03:56:55 PST
Since rolled out, reopen.
Comment 28 Shinya Kawanaka 2012-02-01 03:58:32 PST
Created attachment 124924 [details]
Patch
Comment 29 WebKit Review Bot 2012-02-01 05:50:41 PST
Comment on attachment 124924 [details]
Patch

Clearing flags on attachment: 124924

Committed r106465: <http://trac.webkit.org/changeset/106465>
Comment 30 WebKit Review Bot 2012-02-01 05:50:50 PST
All reviewed patches have been landed.  Closing bug.