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

Integration with trusted types inside of node conversion #1258

Merged
merged 12 commits into from
Apr 18, 2024

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented Feb 22, 2024

Fixes w3c/trusted-types#438

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@lukewarlow
Copy link
Member Author

Bolt on to #1247

@mbrodesser-Igalia
Copy link
Member

@lukewarlow you mentioned somewhere you'd appreciate a review in order to simplify the change. Since it's not primarily about correctness, how helpful would an imminent review be?

@lukewarlow
Copy link
Member Author

@mbrodesser-Igalia that was for an editorial PR to trusted types (which has been merged) rather than this one, so nothing urgent here.

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks complicated. Any reason we cannot convert to Text nodes first and then potentially throw for Text nodes if they pose some kind of danger in combination with a script element?

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@lukewarlow
Copy link
Member Author

This looks complicated. Any reason we cannot convert to Text nodes first and then potentially throw for Text nodes if they pose some kind of danger in combination with a script element?

Ah yeah that's a good idea, can process text nodes and raw strings at the same time them and then can just handle the trusted script after the other processing (because it needs to hold onto the fact it's safe). I knew there was probably a nicer way.

@lukewarlow lukewarlow force-pushed the trusted-types-nodes branch 3 times, most recently from caa217c to 77dcd61 Compare March 5, 2024 14:55
@lukewarlow lukewarlow marked this pull request as ready for review March 13, 2024 13:18
@lukewarlow lukewarlow requested a review from annevk March 13, 2024 14:16
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@lukewarlow lukewarlow requested a review from annevk March 14, 2024 13:08
@lukewarlow
Copy link
Member Author

@annevk is there anything left that's needed for this PR? Wanna make sure I'm not holding anything up.

@lukewarlow lukewarlow mentioned this pull request Apr 11, 2024
5 tasks
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@lukewarlow
Copy link
Member Author

LGTM @annevk thanks for your help getting this across the line.

@annevk annevk merged commit 720a4fc into whatwg:main Apr 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Integration with DOM APIs
3 participants