Don't allow adopting DocumentFragment of a template element

[rniwa]

Right now, we allow adopting a DocumentFragment of a template element because DocumentFragment never have a parent.

The subtree of such a DocumentFragment is still associated with the template element yet its owner document is of the adopted document. As far as I know, this is only case in which such an oddity arises because ShadowRoot is forbidden from being adopted into another tree, and every other node type would have a parent as long as it's part of a document and therefore will be removed before the adoption.

I don't think we should allow this. https://github.com/web-platform-tests/wpt/blob/master/html/semantics/scripting-1/the-template-element/template-element/template-content-hierarcy.html explicitly tests this.

[rniwa]

I suspect throwing is probably not web compatible at this point so I'd suggest silently returning early.

[annevk]

I'm not sure it's that simple. E.g., replace all can be passed a ShadowRoot and will adopt, from what I can tell. It will end up empty too, but still probably not what we want?

I think the problem is with DocumentFragment's host is that we only tried to prevent truly broken cases and did not analyze other side effects enough.

[annevk]

Ah great, Firefox throws on div2.appendChild(shadow); and Chrome and Safari do not change the node document of shadow:

test(() => {
  const doc1 = new Document(),
        doc2 = new Document(),
        div1 = doc1.appendChild(document.createElement("div")),
        div2 = doc2.appendChild(document.createElement("div")),
        shadow = div1.attachShadow({mode: "closed"}),
        shadowChild = shadow.appendChild(new Text());
  div2.appendChild(shadow);
  assert_equals(div2.firstChild, shadowChild);
  assert_equals(shadow.ownerDocument, doc2);
});

(My replace all example is a little misleading as currently that can only be invoked with newly created nodes at the moment, but append/pre-insert has the same issue.)

[rniwa]

We definitely don't implement such a semantic for ShadowRoot so that's just a spec bug.

[annevk]

It's not "just" a specification bug if implementations are doing different things. As far as I know this wasn't fully considered either so it's not clear to me what the resolution ought to be.

[rniwa]

It's not "just" a specification bug if implementations are doing different things. As far as I know this wasn't fully considered either so it's not clear to me what the resolution ought to be.

Well, we clearly don't have any implementation that just adopts the node, and I don't think we ever want that anyway. So it's an oversight that we never spec'ed this behavior. So I agree there is a disagreement as to what exactly happens when a ShadowRoot gets adopted but I don't think the answer to that can never be it gets successfully adopted. That would make no sense.

[annevk]

If I insert a node that has an attached ShadowRoot into a different document, it wouldn't be adopted? I suspect you didn't mean to say that.

I suppose what we need to do is that when you adopt a node that has a host, that needs to be handled in a special way. But if you hit such a node due to recursion, that should not be handled in a special way.

[rniwa]

If I insert a node that has an attached ShadowRoot into a different document, it wouldn't be adopted? I suspect you didn't mean to say that.

Obviously, we should adopt ShadowRoot if its host is adopted. What we don't want to allow is adopting ShadowRoot without adopting its host.

I suppose what we need to do is that when you adopt a node that has a host, that needs to be handled in a special way.

That might be a neat idea to handle both document fragment types. But really, the only place we need a check is https://dom.spec.whatwg.org/#dom-document-adoptnode because all other public DOM APIs would adopt a node either throws on ShadowRoot or takes the children of DocumentFragment so there is no danger of adopting a node.

On a completely unrelated note, I wish DOM / HTML spec had spec assertions like ECMA. That would clarify a lot of subtle conditions like this.

[annevk]

I don't follow. adoptNode() throws on ShadowRoot. It's the other places that are potentially problematic.

We can use assertions and you are free to add them via PRs (or request them to be added during review of a PR): https://infra.spec.whatwg.org/#assertions.

[rniwa]

@annevk : The case I'm concerned about is the content DocumentFragment of a HTMLTemplateElement. As far as I can tell, ShadowRoot doesn't have this issue.

[annevk]

Adopt entry points:

• adoptNode(documentFragmentWithNonNullHost): ShadowRoot has a guard, <template>'s contents do not; ideally these would have the same guard but per OP this might not be web-compatible. So we'd throw for ShadowRoot and return early for the other...
• appendChild(documentFragmentWithNonNullHost): Firefox has a guard, others do not, weird handling as per Don't allow adopting DocumentFragment of a template element #744 (comment)
• replaceChild(documentFragmentWithNonNullHost): ?
• replace all; no primitive that can trigger this yet, but Proposal: ParentNode.replaceAll() / ParentNode.replaceChildren() #478 might add one.

[annevk]

Maybe in part the misunderstanding here is that the specification is wrong for DocumentFragment in general when it comes to those other points and adoption should never happen for it.