construct-stylesheets icon indicating copy to clipboard operation
construct-stylesheets copied to clipboard

Should adopting from a Document into its associated inert template document (and vice versa) clear the adoptedStyleSheets?

Open rakina opened this issue 3 years ago • 11 comments

This came up on a recent Chromium bug. Currently adopting a shadow root with non-empty adoptedStyleSheets into a <template> document will clear the adoptedStyleSheets, and vice versa.

Say we have a Document X and it has a <template> element in it. The <template>'s contents' node document is the Document's associated-inert-template-document. This is a different Document than X, and adopting into or out of the <template>'s contents will clear the adoptedStyleSheets per our spec).

Maybe it's ok if we treat a Document and its associated-inert-template-document as the same document for adoptedStyleSheets related purposes, by adding a special case to the "adopting steps" to not clear adoptedStyleSheets if:

And I think the constructor document will never be an associated-inert-template-document itself because that document is inert and shouldn't be able to construct stylesheets via script, but I might be wrong.

The main reasons we disallowed reusing constructed stylesheets on different documents are the concern that a constructed stylesheet from a document might keep the document alive, and different documents might have different fetch groups. I think this should be ok with the new addition. The <template>'s contents lifetime should be bounded by the document it is in, unless it gets adopted to a different document, in which case the adoptedStyleSheets will be cleared.

rakina avatar Sep 02 '20 09:09 rakina

Treating template content documents specially seems like a rather inelegant hack from a theoretical purity perspective. But... it looks like it would significantly improve the lives of web developers. So, we should fix it in the way you suggest, theoretical purity be damned.

I agree that this won't have any of the negative consequences that fueled the original decision.

domenic avatar Sep 02 '20 21:09 domenic

Thanks! Yeah it's quite unfortunate. Also, when trying to update the spec, I found out that the adopting steps passes node and oldDocument but not the new document (the document that the node will be adopted into). Is this intentional? If so, I wonder if there's a way for us to check the equality of the new document and the oldDocument or the constructor document with just these two...

rakina avatar Sep 03 '20 11:09 rakina

This sounds like a reasonable proposal to me! It will definitely fix the potential footgun of moving nodes to a template and back. Does the solution here avoid the multi-document issues discussed on the other threads (e.g. 23, 15, and webcomponents/759-comment)?

mfreed7 avatar Sep 21 '20 17:09 mfreed7

This sounds like a reasonable proposal to me! It will definitely fix the potential footgun of moving nodes to a template and back. Does the solution here avoid the multi-document issues discussed on the other threads (e.g. 23, 15, and webcomponents/759-comment)?

Yeah, I think unlike the true multi-document case (moving betweeen documents in different frames), the associated-inert-template-document can be treated as the same as its "actual" document (the fetch groups etc is the same).

rakina avatar Sep 22 '20 10:09 rakina

Chromium reporter here. I just wondering what the timeline looks like on this fix? I was going to spend some time putting the workaround in, but it looks like we have some agreements on how to handle which may be introduced fairly soon. LMK TY

brandonseydel avatar Sep 23 '20 23:09 brandonseydel

Chromium reporter here. I just wondering what the timeline looks like on this fix? I was going to spend some time putting the workaround in, but it looks like we have some agreements on how to handle which may be introduced fairly soon. LMK TY

Heya, the spec and impl change is underway, it will be in Chrome M88 release. That won't hit Stable until January, so it's probably good to have the workaround. (from the bug, it seems simple enough?)

rakina avatar Oct 12 '20 10:10 rakina

I don't think this specification should be defining adopting steps for ShadowRoot. That should be directly done in the DOM Standard. Per class only one specification gets to define those after all. (That might mean HTML still needs to export something, but for a different reason.)

annevk avatar Oct 12 '20 12:10 annevk

@rakina - Yea the bug is a simple case, but the workaround would need to be applied to our HTML rendering core for Aurelia 2. I think we will probably be fine waiting as Aurelia 2 won't be RC till after that timeframe. Thank you!

brandonseydel avatar Oct 12 '20 14:10 brandonseydel

I don't think this specification should be defining adopting steps for ShadowRoot. That should be directly done in the DOM Standard. Per class only one specification gets to define those after all. (That might mean HTML still needs to export something, but for a different reason.)

Ah I see, didn't know that before, thanks! If we do that, we need the DOM spec to reference adoptedStyleSheets (because that's cleared on adoption), so I guess updating the spec for this issue is blocked on moving this spec to CSSOM. @mfreed7, do you know what's the latest status on that?

Anyways, I think the impl part in Blink can move forward for now, unless Mozilla folks has other opinions on this. cc @emilio @nordzilla

rakina avatar Oct 13 '20 06:10 rakina

So to be clear the proposal is to only allow adopts from a document's own template-owner document right? Not between arbitrary template/document pairs.

If so I think that should be ok... Otherwise you could use something like this to effectively move stuff across documents.

templateFromDocumentA.content.appendChild(hostFromDocumentA);
documentB.appendChild(hostFromDocumentA);

But yeah it seems ok, I think. A bit of an odd special case to be fair...

emilio avatar Oct 23 '20 18:10 emilio

So to be clear the proposal is to only allow adopts from a document's own template-owner document right? Not between arbitrary template/document pairs.

Yep, exactly! It is quite an odd case, but I think not clearing is less surprising than clearing in this case so hopefully this is a good change.

rakina avatar Oct 26 '20 05:10 rakina