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

Custom element inside <template> uses registry of owner document #25007

Open
pshaughn opened this issue Dec 2, 2019 · 7 comments
Open

Custom element inside <template> uses registry of owner document #25007

pshaughn opened this issue Dec 2, 2019 · 7 comments

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 2, 2019

WPT custom-elements/parser/parser-uses-registry-of-owner-document is testing this; document.write('<template><my-custom-element></my-custom-element></template>') is having my-custom-element interpreted in terms of the custom element registry, which it shouldn't because it's inside a template.

@jdm jdm added this to To do in web-platform-test failures via automation Dec 2, 2019
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 23, 2019

What's supposed to be happening here is that, during parse of a template, new elements are added to a document fragment associated with the "template contents owner document", as described in https://html.spec.whatwg.org/multipage/scripting.html#appropriate-template-contents-owner-document. This document isn't supposed to share a custom element registry with the main document being parsed, and so its registry should be empty during this test, meaning we should have an element with a custom element state of "undefined" and an is value of "my-custom-element". The test then inspects this without moving it into the main document, so it should never be upgraded to the custom element constructor.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

My previous comment was assuming we were hitting the synchronous upgrade during parse; another possibility that needs to be distinguished is that we might be parsing it without upgrading, but then upgrading when we add the template node to the document. If that's the case, then the problem is that we're recursing into the contents of the template's fragment when we iterate over its descendants. Since templates in HTML documents seem to be mostly behaving themselves, I'm leaning towards the parser hypothesis but I haven't made any tests to distinguish the possibilities yet.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

Both things can go wrong, the parser and the append.

custom-elements/parser/parser-uses-registry-of-owner-document.html fails because the parser is synchronously creating the custom element when it should be in the template state.

custom-elements/reactions/Document.html "succeeds" right now because a disconnected node is never getting upgrades, but after the fix of #24992 it fails because the innerHTML setter is appending the customizable element and a reaction is enqueing from the append.

I don't know if I can fix the parser one but I think I can fix the append one.

@pshaughn pshaughn self-assigned this Feb 11, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

The parser case is a result of #18277, and it looks like #18536 was a strong but abandoned move towards solving the parser case both in servo and in html5ever.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

https://w3c.github.io/DOM-Parsing/#the-innerhtml-mixin directs us to https://w3c.github.io/DOM-Parsing/#dfn-fragment-parsing-algorithm and this happens before the step that cares whether we're in a template. The "context element" passed to the fragment parsing algorithm is the template itself.
https://w3c.github.io/DOM-Parsing/#dfn-fragment-parsing-algorithm says create a fragment that has the owner doc of the parent node, and then to append into it, specifically so that the appended nodes will end up in the "correct" node document.
However, if the parent node was a template, this is not the correct owner document!

The obvious thing to try here is just swapping steps 1 and 2 so we're parsing on the fragment instead of on the template; I'll see if that works (a fragment isn't an "element" so doing literally that does not work; trying something similar)

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

I might be wrong about this, but Servo can pass the test if it checks for templateness a little more aggressively; filed w3c/DOM-Parsing#61

bors-servo added a commit that referenced this issue Feb 13, 2020
Upgrade nodes to custom elements on insertion, even if they're not connected to a Document

<!-- Please describe your changes on the following line: -->
After comparing tests to specs, I filed whatwg/dom#833 and made this change to match what I believe the spec should be saying to reflect Firefox/Chrome/Safari shared behavior.

I also filed w3c/DOM-Parsing#61 because this change revealed that custom elements created in a template's innerHTML setter weren't behaving themselves and the spec seemed not to reflect reality about them.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #24992 and fix #25004; they are relevant to #25007 but don't fix all the subcases thereof.

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 14, 2020

It's possible the part of #25730 that relates to templates is worth keeping even if the rest of it turns out to be following a bad WPT test.

@pshaughn pshaughn removed their assignment Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.