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

Using the is= property in innerHTML and in Range.createContextualFragment isn't using the custom element class #24992

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

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 2, 2019

This is test case "innerHTML should instantiate a customized build-in element" in WPT custom-elements/builtin-coverage.html.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Dec 2, 2019

and case "createContextualFragment on Range must construct a custom element" in WPT custom-elements/reactions/Range.html

@pshaughn pshaughn changed the title Using the is= property in innerHTML isn't using the custom element class Using the is= property in innerHTML and in Range.createContextualFragment isn't using the custom element class Dec 2, 2019
@jdm jdm added the A-content/dom label Dec 2, 2019
@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 Jan 22, 2020

The builtin-coverage.html case has been fixed at some point; the Range.html case continues to exist and I'm taking a look at it now.

@pshaughn pshaughn self-assigned this Jan 22, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 22, 2020

This seems likely to be be a parser problem: createContextualFragment ends up in parse_fragment, where it calls ServoParser::parse_html_fragment. It looks like it's parsing the fragment in the right document context, so the parser should be hitting the custom element registry, but I admit the parser is not a part of Servo I understand very well.

@pshaughn pshaughn removed their assignment Jan 22, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 23, 2020

Using the wrong registry when parsing a fragment also happens in #25007 and there might be some connection.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

It looks like we're actually not supposed to have a registry available while parsing the fragment, since https://html.spec.whatwg.org/#html-fragment-parsing-algorithm says to make a new document for the parser and https://html.spec.whatwg.org/multipage/custom-elements.html#look-up-a-custom-element-definition says not to use the registry if there's no browsing context.

However, we create a DocumentFragment using the context document, and we append children to it, and by the time we are appending the children they need to already be custom or they won't be firing the connectedCallbacks they test wants. We're still inside the InnerHTML setter call at this time, so a queued custom element reaction wouldn't have fired; we have to have had a synchronized one by now, despite having parsed into a document with no browsing context.

There might be an actual hole in the specifications here, but it's honestly more likely that I've overlooked some phrasing detail in the spec and I'll try to work it out again in the morning.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

I was misreading which tests were and weren't expecting a connectedCallback in particular among the callbacks. We do have a problem, but I think my previous comment had the timing of it wrong.

It looks like Servo's problem may be that inserting a customizable node into a document tree correctly causes an upgrade, but inserting a customizable node into a detached tree does not cause an upgrade even if the tree is made of elements created on a document with a registry. I'm going to look deeper under that hypothesis.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Feb 11, 2020

I now believe this is a mistake in nested step ordering of the DOM spec; whatwg/dom#833

bors-servo added a commit that referenced this issue Feb 11, 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.

---
<!-- 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 #25004

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

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