Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rakina opened this issue Sep 2, 2020 · 11 comments

Comments

@rakina
Copy link
Member

rakina commented Sep 2, 2020

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 rakina changed the title Should adopting document into its associated inert template document (and vice versa) clear the sheets Should adopting from a Document into its associated inert template document (and vice versa) clear the adoptedStyleSheets? Sep 2, 2020
@domenic
Copy link
Contributor

domenic commented Sep 2, 2020

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.

@rakina
Copy link
Member Author

rakina commented Sep 3, 2020

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

@mfreed7
Copy link

mfreed7 commented Sep 21, 2020

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)?

@rakina
Copy link
Member Author

rakina commented Sep 22, 2020

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

@brandonseydel
Copy link

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

@rakina
Copy link
Member Author

rakina commented Oct 12, 2020

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?)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 12, 2020
…nt <-> template

As discussed in WICG/construct-stylesheets#133,
it should be safe to keep the adoptedStyleSheets when the shadow root
is moved between a document and a <template> within that document. We
should still clear the adoptedStyleSheets when moving a shadow root
between documents outside of that case (e.g. between iframes).

Bug: 1111864
Change-Id: I803e75e6339f9228289188e4ed7d3f14c6e2432c
@annevk
Copy link

annevk commented Oct 12, 2020

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

@brandonseydel
Copy link

@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!

@rakina
Copy link
Member Author

rakina commented Oct 13, 2020

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

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 19, 2020
…nt <-> template

As discussed in WICG/construct-stylesheets#133,
it should be safe to keep the adoptedStyleSheets when the shadow root
is moved between a document and a <template> within that document. We
should still clear the adoptedStyleSheets when moving a shadow root
between documents outside of that case (e.g. between iframes).

Bug: 1111864
Change-Id: I803e75e6339f9228289188e4ed7d3f14c6e2432c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 19, 2020
…nt <-> template

As discussed in WICG/construct-stylesheets#133,
it should be safe to keep the adoptedStyleSheets when the shadow root
is moved between a document and a <template> within that document. We
should still clear the adoptedStyleSheets when moving a shadow root
between documents outside of that case (e.g. between iframes).
I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/W5-EGUySQgc/m/HxrySSpsBAAJ

Bug: 1111864
Change-Id: I803e75e6339f9228289188e4ed7d3f14c6e2432c
@emilio
Copy link

emilio commented Oct 23, 2020

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

@rakina
Copy link
Member Author

rakina commented Oct 26, 2020

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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 1, 2020
…nt <-> template

As discussed in WICG/construct-stylesheets#133,
it should be safe to keep the adoptedStyleSheets when the shadow root
is moved between a document and a <template> within that document. We
should still clear the adoptedStyleSheets when moving a shadow root
between documents outside of that case (e.g. between iframes).
I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/W5-EGUySQgc/m/HxrySSpsBAAJ

Bug: 1111864
Change-Id: I803e75e6339f9228289188e4ed7d3f14c6e2432c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464519
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822999}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 1, 2020
…nt <-> template

As discussed in WICG/construct-stylesheets#133,
it should be safe to keep the adoptedStyleSheets when the shadow root
is moved between a document and a <template> within that document. We
should still clear the adoptedStyleSheets when moving a shadow root
between documents outside of that case (e.g. between iframes).
I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/W5-EGUySQgc/m/HxrySSpsBAAJ

Bug: 1111864
Change-Id: I803e75e6339f9228289188e4ed7d3f14c6e2432c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464519
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822999}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 4, 2020
…ng shadow root between document <-> template, a=testonly

Automatic update from web-platform-tests
Don't clear adoptedStyleSheets when moving shadow root between document <-> template

As discussed in WICG/construct-stylesheets#133,
it should be safe to keep the adoptedStyleSheets when the shadow root
is moved between a document and a <template> within that document. We
should still clear the adoptedStyleSheets when moving a shadow root
between documents outside of that case (e.g. between iframes).
I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/W5-EGUySQgc/m/HxrySSpsBAAJ

Bug: 1111864
Change-Id: I803e75e6339f9228289188e4ed7d3f14c6e2432c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464519
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822999}

--

wpt-commits: 36a5c51e57ac8808bbaf52dd495783fd926b7547
wpt-pr: 26080
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Nov 5, 2020
…ng shadow root between document <-> template, a=testonly

Automatic update from web-platform-tests
Don't clear adoptedStyleSheets when moving shadow root between document <-> template

As discussed in WICG/construct-stylesheets#133,
it should be safe to keep the adoptedStyleSheets when the shadow root
is moved between a document and a <template> within that document. We
should still clear the adoptedStyleSheets when moving a shadow root
between documents outside of that case (e.g. between iframes).
I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/W5-EGUySQgc/m/HxrySSpsBAAJ

Bug: 1111864
Change-Id: I803e75e6339f9228289188e4ed7d3f14c6e2432c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464519
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822999}

--

wpt-commits: 36a5c51e57ac8808bbaf52dd495783fd926b7547
wpt-pr: 26080
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 26, 2022
… to its owner or vice versa.

See WICG/construct-stylesheets#133, which is
where this change was discussed (and then made to Chromium apparently).

I filed w3c/csswg-drafts#7229 to put some
actual spec text here. Also, tweak the wpt which is supposed to test so
that it fails without this change.

Differential Revision: https://phabricator.services.mozilla.com/D144564

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1737455
gecko-commit: d23de6842cb7948a7d8a69ba19717e52c2bbb3a9
gecko-reviewers: edgar
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 27, 2022
…plate document to its owner or vice versa. r=edgar

See WICG/construct-stylesheets#133, which is
where this change was discussed (and then made to Chromium apparently).

I filed w3c/csswg-drafts#7229 to put some
actual spec text here. Also, tweak the wpt which is supposed to test so
that it fails without this change.

Differential Revision: https://phabricator.services.mozilla.com/D144564
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 27, 2022
… to its owner or vice versa.

See WICG/construct-stylesheets#133, which is
where this change was discussed (and then made to Chromium apparently).

I filed w3c/csswg-drafts#7229 to put some
actual spec text here. Also, tweak the wpt which is supposed to test so
that it fails without this change.

Differential Revision: https://phabricator.services.mozilla.com/D144564

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1737455
gecko-commit: d23de6842cb7948a7d8a69ba19717e52c2bbb3a9
gecko-reviewers: edgar
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this issue Apr 27, 2022
… to its owner or vice versa.

See WICG/construct-stylesheets#133, which is
where this change was discussed (and then made to Chromium apparently).

I filed w3c/csswg-drafts#7229 to put some
actual spec text here. Also, tweak the wpt which is supposed to test so
that it fails without this change.

Differential Revision: https://phabricator.services.mozilla.com/D144564

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1737455
gecko-commit: d23de6842cb7948a7d8a69ba19717e52c2bbb3a9
gecko-reviewers: edgar
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Apr 28, 2022
…plate document to its owner or vice versa. r=edgar

See WICG/construct-stylesheets#133, which is
where this change was discussed (and then made to Chromium apparently).

I filed w3c/csswg-drafts#7229 to put some
actual spec text here. Also, tweak the wpt which is supposed to test so
that it fails without this change.

Differential Revision: https://phabricator.services.mozilla.com/D144564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants