Skip to content

Add missing IDL changes to Parent and Child Node mixins from dom spec#440

Closed
lukewarlow wants to merge 2 commits intow3c:mainfrom
lukewarlow:node-idl-updates
Closed

Add missing IDL changes to Parent and Child Node mixins from dom spec#440
lukewarlow wants to merge 2 commits intow3c:mainfrom
lukewarlow:node-idl-updates

Conversation

@lukewarlow
Copy link
Copy Markdown
Member

@lukewarlow lukewarlow commented Feb 20, 2024

Address #438

I haven't done the DOM parts change but have made a follow up issue for tracking that so it doesn't get lost

cc @mbrodesser-Igalia


Preview | Diff

@lukewarlow lukewarlow force-pushed the node-idl-updates branch 2 times, most recently from 7af8cd1 to 49f3af5 Compare February 20, 2024 16:34
@lukewarlow lukewarlow marked this pull request as ready for review February 20, 2024 16:34
@lukewarlow lukewarlow requested a review from koto February 20, 2024 16:34
@ghost
Copy link
Copy Markdown

ghost commented Feb 22, 2024

@lukewarlow could you please re-create this PR or refresh it (don't know if that's possible) so that diff and preview are generated (see #451 (comment), verified in #453)?

- Add missing IDL changes to Parent and Child Node mixins from dom spec

- Update `converting nodes into a node` algorithm to enforce trusted types
@lukewarlow
Copy link
Copy Markdown
Member Author

Nice! that's working. Just had to rebase with main.

@ghost
Copy link
Copy Markdown

ghost commented Feb 22, 2024

Nice! that's working. Just had to rebase with main.

@lukewarlow could you please rebase the node-idl-updates branch too? Currently the generated Diff contains also changes for "execCommand" and perhaps more.

@ghost
Copy link
Copy Markdown

ghost commented Feb 22, 2024

Nice! that's working. Just had to rebase with main.

@lukewarlow could you please rebase the node-idl-updates branch too? Currently the generated Diff contains also changes for "execCommand" and perhaps more.

Or presumably your fork's main branch has to be rebased too.

@lukewarlow
Copy link
Copy Markdown
Member Author

That seems to be a bug with the differ as this is fully rebased.

@ghost
Copy link
Copy Markdown

ghost commented Feb 22, 2024

That seems to be a bug with the differ as this is fully rebased.

To further simplify reviewing: couldn't this be a PR for the DOM spec? The diff would be simpler to consume. It has to move there eventually.

@ghost
Copy link
Copy Markdown

ghost commented Feb 22, 2024

That seems to be a bug with the differ as this is fully rebased.

To further simplify reviewing: couldn't this be a PR for the DOM spec? The diff would be simpler to consume. It has to move there eventually.

@lukewarlow WDYT about creating a draft PR for the DOM spec and link to that PR from a "Integration with the DOM" section in the TT spec?

@lukewarlow
Copy link
Copy Markdown
Member Author

Issue with that is it relies on trusted types algorithms so I think should be moved when we eventually move everything else?

@ghost
Copy link
Copy Markdown

ghost commented Feb 22, 2024

Issue with that is it relies on trusted types algorithms so I think should be moved when we eventually move everything else?

Another draft relies on trusted types algos, too: https://github.com/whatwg/dom/pull/1247/files. As long as it's a draft PR, it's harmless anyway. There seems to be no advantage of keeping it in the TT spec.

Comment thread spec/index.bs

<pre class="idl exclude">
partial interface mixin ParentNode {
[CEReactions, Unscopable] undefined prepend((Node or DOMString or TrustedScript)... nodes);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Confused: when to use [StringContext=TrustedScript] DOMString and when to use DOMString or TrustedScript?

The former is specified in https://w3c.github.io/trusted-types/dist/spec/#StringContext.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TrustedScript is the object itself, where as [StringContext=TrustedScript] DOMString (or its alias ScriptString) pass the string itself through.

These methods need the raw object because the processing happens at the algorithm level (as it needs to process TextNode content AND strings through a policy)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TrustedScript is the object itself, where as [StringContext=TrustedScript] DOMString (or its alias ScriptString) pass the string itself through.

These methods need the raw object because the processing happens at the algorithm level (as it needs to process TextNode content AND strings through a policy)

Doesn't it imply someParentNode.prepend(someString) may be called, even if trusted types are enforced?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The linked DOM spec PR updates the relevant algorithms so that the string gets put through the trusted types algos.

@lukewarlow
Copy link
Copy Markdown
Member Author

Another draft relies on trusted types algos, too: https://github.com/whatwg/dom/pull/1247/files. As long as it's a draft PR, it's harmless anyway. There seems to be no advantage of keeping it in the TT spec.

Okay yeah fair I forgot about that one. I'll move this across to a draft PR there

@lukewarlow
Copy link
Copy Markdown
Member Author

I've opened whatwg/dom#1258

- Add missing IDL changes to Parent and Child Node mixins from dom spec

- Link to DOM spec PR with IDL and algorithm changes
@lukewarlow
Copy link
Copy Markdown
Member Author

@koto I see you've updated IDL in your DOM spec PR should the IDL changes specifically be included in the trusted types spec too? Or should we just link out to the relevant place in the DOM spec (once merged)?

I want to ensure we have some unified place to come to for all trusted types changes but also understand we don't want to monkeypatch and duplicate effort.

I've updated this PR to the IDL change and a link to my PR only. But it'd be good to understand what we want the end goal to be. It'd be especially interesting to know what people think the plan should be for ongoing maintenence. Should we make a PR to HTML for example updating the relevant IDL with the new types?

@koto
Copy link
Copy Markdown
Member

koto commented Feb 26, 2024

I think we should prioritize upstreaming all the integrations, as monkeypatching just isn't easy to work with. That includes DOM, DOM Parsing, HTML and WebIDL at the least.

@lukewarlow
Copy link
Copy Markdown
Member Author

Closing this as these integrations will all be upstreamed.

@lukewarlow lukewarlow closed this Mar 13, 2024
@lukewarlow lukewarlow deleted the node-idl-updates branch March 27, 2024 18:17
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.

2 participants