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

Upstream IDL changes from Trusted Types #78

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

lukewarlow
Copy link
Member

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/

@lukewarlow
Copy link
Member Author

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

Copy link
Collaborator

@domenic domenic left a 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.)

@lukewarlow
Copy link
Member Author

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
@lukewarlow
Copy link
Member Author

Got that working! Just needed a new field in the respecConfig file.

@lukewarlow
Copy link
Member Author

It's supposed to be merged into HTML one day.

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?

@annevk
Copy link
Member

annevk commented Mar 15, 2024

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.

@lukewarlow
Copy link
Member Author

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.

Copy link
Member

@travisleithead travisleithead left a 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.

@lukewarlow
Copy link
Member Author

I don't have merge rights to this repo so if someone could hit merge that'd be great :)

@travisleithead travisleithead merged commit cf5826e into w3c:main Mar 20, 2024
2 checks passed
@travisleithead
Copy link
Member

travisleithead commented Mar 20, 2024

Arg. Build still failed in the CI pipeline. @lukewarlow care to dig in?

lukewarlow added a commit to lukewarlow/trusted-types that referenced this pull request Mar 25, 2024
koto pushed a commit to w3c/trusted-types that referenced this pull request Mar 25, 2024
@mbrodesser-Igalia
Copy link

@travisleithead: the patch is still stuck in the CI pipeline. Please reopen this ticket.

CC @lukewarlow

@lukewarlow
Copy link
Member Author

lukewarlow commented Apr 8, 2024

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.

@lukewarlow
Copy link
Member Author

lukewarlow commented Apr 8, 2024

@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?

@lukewarlow
Copy link
Member Author

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.

@siusin
Copy link
Contributor

siusin commented Apr 10, 2024

@lukewarlow
I believe this spec will be merged into whatwg/html or whatwg/dom someday. I've given you the write access to this repo.

Please let us know if you want to publish a new version of this spec.

github-actions bot added a commit that referenced this pull request Apr 10, 2024
SHA: cf5826e
Reason: push, by travisleithead

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants