Set registry on null registry element on adopt instead of on insert#1423
Conversation
There was a problem hiding this comment.
This matches my understanding of the first set of changes proposed in #1413 (comment) and agreed to at TPAC. Good job!
One minor point: it seems like since insert 7.1 does adopt, perhaps insert 7.7.3.1 and 7.7.4.1 are no longer needed?
|
@rniwa One thing I'd like to clarify from your PR: One of the test files added is phrased as "append does not upgrade". However, is it correct that we still want to run upgrade during append/insertion steps? It is possible that a null registry element gets a registry when it's appended cross document (which involves adopt steps). In that case we'll want to run an upgrade on the element right? Want to make sure I got the correct understanding here. |
So regular append should upgrade elements that have a non-null custom element registry. An element with null registry should retain its null registry state and not get upgraded upon insertion.
Yes, in the cross-document append case, we'd first adopt the element to the destination document at which point the null registry will be replaced with the global registry of the destination document. |
…ing null to createEleent and attachShadow per - whatwg/dom#1424 - whatwg/dom#1423 Also update the existing tests to account for the behavior differences.`
…ing null to createEleent and attachShadow per - whatwg/dom#1424 - whatwg/dom#1423 Also update the existing tests to account for the behavior differences.`
@sorvell My understanding is that the scoped document set is used for searching for upgrade candidates in the case of new definition added, so we'll need to keep those two steps around. |
…ing null to createEleent and attachShadow per (#56108) - whatwg/dom#1424 - whatwg/dom#1423 Also update the existing tests to account for the behavior differences.`
| <li> | ||
| <p>If <var>inclusiveDescendant</var> is a <a for=/>shadow root</a>: | ||
|
|
||
| <ol> | ||
| <li><p>if <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a> | ||
| is null or <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a>'s | ||
| <a for=CustomElementRegistry>is scoped</a> is false, then set | ||
| <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a> to | ||
| <var>document</var>'s <a>effective global custom element registry</a>. |
There was a problem hiding this comment.
| <li> | |
| <p>If <var>inclusiveDescendant</var> is a <a for=/>shadow root</a>: | |
| <ol> | |
| <li><p>if <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a> | |
| is null or <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a>'s | |
| <a for=CustomElementRegistry>is scoped</a> is false, then set | |
| <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a> to | |
| <var>document</var>'s <a>effective global custom element registry</a>. | |
| <li><p>If <var>inclusiveDescendant</var> is a <a for=/>shadow root</a> and either | |
| <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a> is null or | |
| <var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a>'s | |
| <a for=CustomElementRegistry>is scoped</a> is false, then set <var>inclusiveDescendant</var>'s | |
| <a for=ShadowRoot>custom element registry</a> to <var>document</var>'s | |
| <a>effective global custom element registry</a>. |
There was a problem hiding this comment.
But I think this is incorrect and we still need "keep custom element registry null" here. Have you looked at how the HTML Standard sets "keep custom element registry null"? How is the shadowrootcustomelementregistry content attribute supported in this new world?
There was a problem hiding this comment.
In HTML spec where we define the parsing logic for "A start tag whose tag name is "template", we have two repetitive lines describing how shadowrootcustomelementregistry attribute is used:
- If templateStartTag has a shadowrootcustomelementregistry attribute, then set registry to null.
- If templateStartTag has a shadowrootcustomelementregistry attribute, then set shadow's keep custom element registry null to true.
I think we can remove that second line and completely remove the usage of "keep custom element registry null" and we can simply rely on setting the shadowroot's registry to null to support shadowrootcustomelementregistry content attribute behavior.
There was a problem hiding this comment.
How do we distinguish the case of a shadow root from a null document registry without the attribute from one with the attribute?
There was a problem hiding this comment.
Is it possible to have a DSD with null registry due to shadowrootcustomelementregistry in a null registry document though? Assuming my understanding is correct on "DSD is only created during document parsing and we're not able to get a null registry document during document parsing (has to through JS code like document.implementation.createHTMLDocument();)"
In that case, we can safely assign a registry to null registry shadow root on adopt and remove the need of "keep custom element registry" as we don't assign registry on append regardless.
There was a problem hiding this comment.
Yeah, except I think you also need shadowrootclonable attributes.
There was a problem hiding this comment.
yeah, makes sense. Edited example above for clarity.
There was a problem hiding this comment.
Makes sense, I put "keep custom element registry" back and check it during node adoption. It seems like we're missing some WPT coverage on the behavior of "keep custom element registry". Will add it.
There was a problem hiding this comment.
@annevk @sorvell
One thing I was thinking today about the adoption steps:
From the above example, based on the new spec, it's clear that shadowroot should be getting effective global registry or null depending on "keep custom element registry null" and the original factors (null or is using another global registry) during adoption.
But what about x-foo? I think the intended behavior is that x-foo' s registry will remain to be null. However, based on the new spec, the element has null registry originally, so during adoption in above case, it will gain the effective global registry of document, which does not align with the expected behavior.
When we set registry for null registry element during insertion in previous spec, we assign the registry based off the inclusiveDescendant's parent's registry. Do we need to do something similar when we're setting registry for null registry element during adoption? Perhaps something like "set inclusiveDescendant's parent's (or inclusiveDescendant's tree scope's) registry if it is an effective global registry in the new document, otherwise set null"
There was a problem hiding this comment.
For the x-foo in host2, yeah, think you’re right if not otherwise covered. Case:
- Adopted element is null
- Adopting document is global
- But adopted element’s root node is null with keep registry null is true.
99b344b to
ddc27b7
Compare
annevk
left a comment
There was a problem hiding this comment.
I rebased and pushed some nits. This looks good to me now but if people could have another look and answer my questions I'd appreciate it.
| <li> | ||
| <p>If <var>inclusiveDescendant</var> is an <a for=/>element</a>: | ||
| <p>If <var>inclusiveDescendant</var> is an <a for=/>element</a> and | ||
| <var>inclusiveDescendant</var>'s <a for=Element>custom element registry</a> is non-null: |
There was a problem hiding this comment.
I suspect this is not a change in behavior, as we can't upgrade elements with a null registry, but was this change intentional?
There was a problem hiding this comment.
I see what you meant. Given that we can't upgrade element nor getting a connectedCallback with null registry, I think we can leave this null check as is.
| is false; or | ||
|
|
||
| <li><p><var>inclusiveDescendant</var>'s <a for=ShadowRoot>custom element registry</a> | ||
| <a>is a global custom element registry</a>, |
There was a problem hiding this comment.
I changed this to "is a global custom element registry" as this was lacking a null check before.
ddc27b7 to
9821f8a
Compare
…content attribute and specifying null to createEleent and attachShadow per, a=testonly Automatic update from web-platform-tests Add new tests for customelementregistry content attribute and specifying null to createEleent and attachShadow per (#56108) - whatwg/dom#1424 - whatwg/dom#1423 Also update the existing tests to account for the behavior differences.` -- wpt-commits: cc1c056521abd0ff97456455dbebe113617a2bc1 wpt-pr: 56108
|
@annevk A case I’m not sure if we covered in the updated spec and would like to know your thoughts: |
|
I filed #1429 to track that. It does seem problematic. Unfortunately I missed it before I hit merge. |
…content attribute and specifying null to createEleent and attachShadow per, a=testonly Automatic update from web-platform-tests Add new tests for customelementregistry content attribute and specifying null to createEleent and attachShadow per (#56108) - whatwg/dom#1424 - whatwg/dom#1423 Also update the existing tests to account for the behavior differences.` -- wpt-commits: cc1c056521abd0ff97456455dbebe113617a2bc1 wpt-pr: 56108 UltraBlame original commit: bb6f215085587911288d0b50256c14ad6ecd246e
…content attribute and specifying null to createEleent and attachShadow per, a=testonly Automatic update from web-platform-tests Add new tests for customelementregistry content attribute and specifying null to createEleent and attachShadow per (#56108) - whatwg/dom#1424 - whatwg/dom#1423 Also update the existing tests to account for the behavior differences.` -- wpt-commits: cc1c056521abd0ff97456455dbebe113617a2bc1 wpt-pr: 56108 UltraBlame original commit: bb6f215085587911288d0b50256c14ad6ecd246e
…content attribute and specifying null to createEleent and attachShadow per, a=testonly Automatic update from web-platform-tests Add new tests for customelementregistry content attribute and specifying null to createEleent and attachShadow per (#56108) - whatwg/dom#1424 - whatwg/dom#1423 Also update the existing tests to account for the behavior differences.` -- wpt-commits: cc1c056521abd0ff97456455dbebe113617a2bc1 wpt-pr: 56108 UltraBlame original commit: bb6f215085587911288d0b50256c14ad6ecd246e
instead of insert It was discussed over TPAC and outlined in whatwg/dom#1423 that we want to set registry on null registry element during adoption instead of insertion of a node. Bug: 448853877 Change-Id: I63b8a7bcef98ead66bd1375c9b1df69d9b9bd597
instead of insert It was discussed over TPAC and outlined in whatwg/dom#1423 that we want to set registry on null registry element during adoption instead of insertion of a node. Bug: 448853877 Change-Id: I63b8a7bcef98ead66bd1375c9b1df69d9b9bd597
instead of insert It was discussed over TPAC and outlined in whatwg/dom#1423 that we want to set registry on null registry element during adoption instead of insertion of a node. Bug: 448853877 Change-Id: I63b8a7bcef98ead66bd1375c9b1df69d9b9bd597 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7078864 Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Jayson Chen <jaysonchen@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1574942}
instead of insert It was discussed over TPAC and outlined in whatwg/dom#1423 that we want to set registry on null registry element during adoption instead of insertion of a node. Bug: 448853877 Change-Id: I63b8a7bcef98ead66bd1375c9b1df69d9b9bd597 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7078864 Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Jayson Chen <jaysonchen@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1574942}
instead of insert It was discussed over TPAC and outlined in whatwg/dom#1423 that we want to set registry on null registry element during adoption instead of insertion of a node. Bug: 448853877 Change-Id: I63b8a7bcef98ead66bd1375c9b1df69d9b9bd597 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7078864 Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Jayson Chen <jaysonchen@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1574942}
…should get registry on adopt instead of insert, a=testonly Automatic update from web-platform-tests [scoped-registry] Null registry element should get registry on adopt instead of insert It was discussed over TPAC and outlined in whatwg/dom#1423 that we want to set registry on null registry element during adoption instead of insertion of a node. Bug: 448853877 Change-Id: I63b8a7bcef98ead66bd1375c9b1df69d9b9bd597 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7078864 Reviewed-by: Joey Arhar <jarhar@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Jayson Chen <jaysonchen@microsoft.com> Cr-Commit-Position: refs/heads/main@{#1574942} -- wpt-commits: 5b0c22e0813ff10a654adf6148cd57a66d50823e wpt-pr: 57213
In order to support scenarios for elements with a null custom element registry without shadow DOM, we decided to modify the behavior when it comes to setting registry on null registry element. By setting the registry on adopt instead of on insert allows us to have null registry element in light DOM while being backwards-compatible.
Fixes #1413 partially. There are two more spec changes expected to add element attribute and allow null registry option during element creation and shadow attachment as mentioned in #1413 (comment)
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff