Refactor fragment parsing algorithm to match implementations#12624
Conversation
|
@shannonbooth do you have a list of WPTs that this fixes? I want to add them to the OP. |
noamr
changed the title
|
Fixed editorial changes, also made the root adjustment for text/comment/PI |
Instead of inserting elements to a fake root element of a connected inert document and then appending them to a `DocumentFragment`, we let the parser insert the root's children straight into that `DocumentFragment`. This is done by having an "adjusted insertion target" that the parser uses to route insertions desined to the root of the stack directly into the fragment. The observable behavior change is in subtle places that check for connectedness when insertion, like radio buttons. This is also creates an enabler that can be used later on for HTML streaming, as the "adjusted insertion target" can be the context element instead of the intermediate fragment. Note that we still need some notion of the intermediate inert document, to avoid creation time side effects like image loading and custom element constructors to occur before sanitization. (This resembles blink implementation, more than the current wording) Closes #11023
My brain is melting trying to understand how this all works 😅 I think it might actually be a behaviour preserving with previous version, due to the order of adoption, but this all seems very non-obvious. #12624 (comment) was where I originally ran into this. With this latest change, I don't see any extra WPTs (at least imported) passing, but I do see that regressing from beforehand. I've spent quite a while again trying to figure this out, but if I'm not misunderstanding how it was before was something along the lines of
Might be wrong with above sorry! looking at all at the end of a long day |
|
I think we should add more tests for the cases AI identified. They are indeed super subtle. |
Added more tests to web-platform-tests/wpt#61042 |
Thanks a lot! I can confirm that for me the new test passes before I make the changes in this PR, fails before the fix in question, and passes after it. Interesting that blink and webkit both fail that test, I wonder both engines are relying on the same behaviour I'm seeing for #12624 (comment) since they also pass that test. I'm probably going to implement it in a way not matching that WPT to match other engines and not cause the regression I'm seeing. NB: After debugging this for quite a bit, I think there seem to be misalignments between spec and WPTs for how custom element registry is selected, I'll try and report these issues separately, e.g for Range.createContextualFragment(). |
So the current PR matches all the relevant WPTs AFAYCT?
which test specifically? |
No, unfortunately "Explicitly use template's custom registry" 9f41a9c causes a harness error for: https://github.com/web-platform-tests/wpt/blob/master/custom-elements/form-associated/form-disabled-callback.html I just asked codex to make a reduction of the test and try summarise (starts throwing): <!doctype html><script>
let constructed = false;
customElements.define("x-face", class extends HTMLElement {
static formAssociated = true;
constructor()
{
super();
constructed = true;
console.log("constructor registry:", this.customElementRegistry);
this.attachInternals(); // Throws if this.customElementRegistry is null.
}
});
const template = document.createElement("template");
/*
Spec path implemented by the last commit:
1. Element.innerHTML: because this is a <template>, target = template.content.
2. Fragment parsing: context = target's host, i.e. the <template>.
3. Registry context = context because context is a <template>, so the parser's
fake root uses document.customElements.
4. Returned fragment's node document = target's node document, i.e. the inert
template contents document, whose custom element registry is null.
That lets parsing find x-face in the global registry, then insert/adopt it into
the null-registry template document before reactions run. The constructor calls
attachInternals(), which looks up the definition through this.customElementRegistry:
null registry => null definition => NotSupportedError.
Before the last commit, adoption into template.content happened after parsing and
reactions, so attachInternals() still saw the global registry. The correct result
for this inert template assignment is no construction at all.
*/
try {
template.innerHTML = "<x-face></x-face>";
console.log("returned; constructed =", constructed);
} catch (e) {
console.log("threw:", e.name, e.message, "constructed =", constructed);
}
</script>
NB: my implementation of this doesn't pass every relevant test, but AFAIKT this does not cause any regressions in WPT from beforehand, and definitely does align the spec closer to implementations. Implementation is here if you're interested: shannonbooth/ladybird@367b872 (highly likely I made some bugs left in the parser W.R.T insertion point currently) |
Aha. So I think section (1) in #12624 (comment) is actually a bug in the current spec, and there is no clean solutions for it in existing implementations given 9f41a9c. @annevk I propose that for the purpose of reflecting current implementations, we remove the template special-casing that came up in that AI review and deal with it as a separate (existing) spec bug with scoped custom element registries, and change the WPT to assert what blink+webkit are currently doing.
That's the scope of this PR. Thanks! |
|
@annevk I've updated this PR to reflect current implementation state around |
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Maing this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. See: whatwg/html#12624 for background.
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Making this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. Ref: whatwg/html#12624
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Making this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. Ref: whatwg/html#12624
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Making this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. Ref: whatwg/html#12624
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Making this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. Ref: whatwg/html#12624
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Making this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. Ref: whatwg/html#12624
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Making this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. Ref: whatwg/html#12624
Refactor the HTML fragment parser to insert the root's children straight into a DocumentFragment instead of a connected temporary document. On top of being a correctness fix, this should also improve performance of fragment parsing. Making this change has other fallout due to the timing of document adoption changing. This results in related other changes needing to be made, including: * The parser having "allow declarative shadow roots" as a parser flag so that it does not read the flag from the wrong document. This prevents some regressions which would happen otherwise, but also aligns our behaviour in general where in some cases the wrong setting was previously being chosen. * Custom element registry is now taken from the *target* node instead of the *context* node. This also prevents regressions as part of the fallout of these changes, but also fixes the selection of the custom element registry for ShadowRoots (which was previously being taken from the root document). * Use a null custom element registry for template.innerHTML. Similar to both cases above, this prevents regressions from the above changes due to the change in timing of document adoption, but fixing this also aligns behaviour closer what other engines implement. Ref: whatwg/html#12624
* Add test-case for DSD when calling document.open() on an XHR response See whatwg/html#12624 This clarifies a scenario where the allow-DSD flag has to be sticky on the document rather than just a parser flag. * Add tests for additional parser edge cases: - Check insertion logic "after body" for template.innerHTML - Check that template.innerHTML takes the custom element registry from the template * Revert template special case
… a=testonly Automatic update from web-platform-tests Cover a few fragment parsing edge cases (#61042) * Add test-case for DSD when calling document.open() on an XHR response See whatwg/html#12624 This clarifies a scenario where the allow-DSD flag has to be sticky on the document rather than just a parser flag. * Add tests for additional parser edge cases: - Check insertion logic "after body" for template.innerHTML - Check that template.innerHTML takes the custom element registry from the template * Revert template special case -- wpt-commits: 35be3b44f3111c4d614b5b201e399493d20e7b38 wpt-pr: 61042
3 participants
This makes a few key changes to the fragment parsing workflow, to better match existing implementations and tests.
DocumentFragment, we let the parser insert the root's children straight into thatDocumentFragment. This introduces an "root insertion target", which is aDocumentFragmentto insert to if the parser wants to insert nodes to the root.allow declarative shadow rootsdirectly on the document, it becomes a parser flag. This removes some confusion that makes some subtle cases use the wrong setting. It is still kept on the document so that it is sticks for the "document opening steps" (document.open()of a document created elsewhere).shadowRoot.innerHTML. Note that this changes the spec'ed behavior oftemplate.innerHTMLto have a null registry - this aligns with current implementations and can be changed in a follow up together with implementation/test changes.DocumentFragment).The above changes are a lot closer to how blink implements fragment parsing (and given the passing tests, likely WebKit and Gecko as well).
Closes #11023
Closes #12661
document.open()on an XHR response(See WHATWG Working Mode: Changes for more details.)
/document-lifecycle.html ( diff )
/dynamic-markup-insertion.html ( diff )
/parsing.html ( diff )
/xhtml.html ( diff )