-
Notifications
You must be signed in to change notification settings - Fork 14
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
Upstream IDL changes from Trusted Types #78
Conversation
988ac08
to
8e75604
Compare
@travisleithead I'm not sure if you still work on this spec, but if you do it'd be great if you could take a look over this PR. It's fairly minor, just means we don't have to maintain spec patches downstream within TT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm able to approve and merge changes to this spec, since it's supposed to be merged into HTML one day.
Would you be able to work on getting the build passing though, if it's not too much trouble? It looks like it's because of preexisting problems, but it would increase my confidence in merging changes if we had a green baseline. (And, maybe it's required to get the result published anyway, not sure.)
Yeah I'm happy to take a look at that,. shouldn't be too hard to get it passing |
innerHTML, outerHTML, insertAdjacentHTML, and createContextualFragment all now take HTMLString values Also add missing group and xref fields to respecConfig.js
8e75604
to
94a4d34
Compare
Got that working! Just needed a new field in the respecConfig file. |
Just out of curiosity what is the reasoning for this being separated? For example the InnerHTML mixin is implemented in all browsers and seems like it would be better suited to HTML? Is it just a matter of no one has upstreamed it yet? |
It's upstreaming and I think we also had the ambition of actually verifying the tests and ensuring the text is correct with respect to implementations as there's quite a few issues outstanding as well. Though I suspect at this point we'd accept a pure upstreaming patch with an XXX marker for the issues. |
Okay that sounds good. If we get this upstreamed here for now and then when I've got some time I'll probably take a look at upstreaming this document (or at least parts of it) to the HTML spec itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me. It's been a while, but IIRC the algorithms were partially documenting existing behavior and partially aspirational in places where behavior diverged. Especially for XHTML serialization, I'm not sure how likely (or how important) it would be for implementations to make adjustments to their behavior at this point.
I don't have merge rights to this repo so if someone could hit merge that'd be great :) |
Arg. Build still failed in the CI pipeline. @lukewarlow care to dig in? |
@travisleithead: the patch is still stuck in the CI pipeline. Please reopen this ticket. CC @lukewarlow |
It's a PR so it can't be "reopened". I'll take a look at the CI pipeline soon. Pending discussions on the StringContext attribute I might need to do a follow up to this PR that changes the IDL again, so I'll fix it then. Apologies for the delay in getting back to this btw. |
@travisleithead by the looks of the error it's respec related rather than an actual spec issue. I'm not familar with respec so probably not the best person to fix it. (I get a different error locally when running the command even). Retrying it might just fix it? |
So given the pipeline is broken, and we need to upstream this stuff anyway, I've made a start on it whatwg/html#10264. Started with the innerHTML mixin. |
@lukewarlow Please let us know if you want to publish a new version of this spec. |
SHA: cf5826e Reason: push, by travisleithead Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
innerHTML, outerHTML, insertAdjacentHTML, and createContextualFragment all now take HTMLString values
Changes from https://w3c.github.io/trusted-types/dist/spec/#integration-with-dom-parsing
Tested in at https://wpt.live/trusted-types/