Bug 79620 – Appending ShadowRoot into an element should not cause crash.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

This will cause an assertion failure.

var div1 = document.createElement('div');
var root = new WebKithShadowRoot(div1);

var div2 = document.createElement('div');
div2.appendChild(root);

[Hayato Ito]

I think we have two options in this case:

A). Throws DOM Exception
B). ShadowRoot acts like as DOCUMENT_FRAGMENT so every children of shadowRoot are appended.

It seems the spec says B implicitly because ShadowRoot's nodeType is DOCUMENT_FRAGMENT_NODE.
I thinks A is safer option, but we should file a bug for the spec if we choose A.

[Shinya Kawanaka]

I've filed a bug here.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16130

[Shinya Kawanaka]

Created attachment 128953 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #1)
> I think we have two options in this case:
> 
> A). Throws DOM Exception
> B). ShadowRoot acts like as DOCUMENT_FRAGMENT so every children of shadowRoot are appended.
> 
> It seems the spec says B implicitly because ShadowRoot's nodeType is DOCUMENT_FRAGMENT_NODE.
> I thinks A is safer option, but we should file a bug for the spec if we choose A.

I wrote a patch to reject appending shadow root.
If we should choose (B) option, I'll change the patch.

[Dimitri Glazkov (Google)]

Comment on attachment 128953 [details]
Patch

It should act as a document fragment.

[Shinya Kawanaka]

(In reply to comment #5)
> (From update of attachment 128953 [details])
> It should act as a document fragment.

OK. Thnaks, Dimitri. I'll update the patch.

[Shinya Kawanaka]

Created attachment 129624 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 129624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129624&action=review

> LayoutTests/fast/dom/shadow/shadow-root-append.html:25
> +    shouldBeNull('root.firstChild');

Should you be checking to see if the container has chlidren now?

[James Robinson]

Woah. the watchlist thought this patch touched Source/WebCore/platform/graphics/chromium/*. very weird!

[Shinya Kawanaka]

(In reply to comment #8)
> (From update of attachment 129624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129624&action=review
> 
> > LayoutTests/fast/dom/shadow/shadow-root-append.html:25
> > +    shouldBeNull('root.firstChild');
> 
> Should you be checking to see if the container has chlidren now?

Sure.

[Shinya Kawanaka]

Ah, I found that I missed adding Skipped list. I'll land this patch with Skipped list.

[Shinya Kawanaka]

Created attachment 129625 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 129625 [details]
Patch for landing

Rejecting attachment 129625 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 129625 [details]
Patch for landing

Clearing flags on attachment: 129625

Committed r109359: <http://trac.webkit.org/changeset/109359>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.