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

Update IDL for script enforcement #484

Merged
merged 4 commits into from
Apr 23, 2024
Merged

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented Mar 18, 2024

  • Node/textContent, and Element/innerText are both now shadowed on HTMLScriptElement.

HTMLScriptElement/textContent uses a union type rather than [StringContext] because it uses a nullable type.

All of the properties are using union types as that seems to be the direction the spec is going to go in. But I can revert that commit and go back to using the extended attribute.

This PR is part of 1 of addressing #437 it specifically doesn't address #483 or #252


Preview | Diff

@lukewarlow lukewarlow marked this pull request as draft March 18, 2024 14:02
@lukewarlow lukewarlow requested a review from koto March 18, 2024 14:03
@lukewarlow lukewarlow marked this pull request as ready for review March 18, 2024 14:03
@lukewarlow
Copy link
Member Author

cc @mbrodesser-Igalia might be of interest to you as you start implementing more in Gecko

spec/index.bs Outdated Show resolved Hide resolved
@lukewarlow
Copy link
Member Author

lukewarlow commented Apr 2, 2024

@annevk hopefully this is now in a better shape. Shadows the properties onto HTMLScriptElement rather than messing with Node/textContent and Element/innerText. Uses the IDL attribute where possible so algorithms are mostly just setting the internal slot and calling the parent steps

HTMLScriptElement/textContent uses a union type instead of the IDL extended attribute to avoid issues with nullable IDL type handling.

spec/index.bs Outdated Show resolved Hide resolved
@lukewarlow lukewarlow marked this pull request as ready for review April 2, 2024 12:56
@lukewarlow lukewarlow force-pushed the fix-script-idl branch 2 times, most recently from 2d2077c to dedb72a Compare April 3, 2024 14:38
@lukewarlow
Copy link
Member Author

Updated this to link to the correct steps in the DOM and html specs so it should read better.

- Node/textContent, and Element/innerText are both now shadowed on HTMLScriptElement.

HTMLScriptElement/textContent uses a union type rather than [StringContext] because it uses a nullable type.
@lukewarlow lukewarlow marked this pull request as ready for review April 17, 2024 15:03
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@lukewarlow lukewarlow requested a review from annevk April 22, 2024 13:20
@lukewarlow
Copy link
Member Author

@koto and @annevk does this seem ready to merge into the spec now?

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.

I think I have the same question as I have for the HTML PR. Do we need to store a redundant copy of the text, or is the actual model invalidation of some kind of trusted bit.

spec/index.bs Outdated Show resolved Hide resolved
@lukewarlow
Copy link
Member Author

lukewarlow commented Apr 22, 2024

@annevk

Currently: We validate during prepare the text so we do need to store the text. The "invalidation" is currently only something we propose for the parsing behaviour.

It's possible that if all the ways to change the children of a script element end up firing children changed steps, and we get the neccessary changes there (still need to open a DOM PR) that we can do away with the script text entirely and just use invalidation and a trusted boolean "slot".

I'll raise a separate issue to discuss that as that's tangental to this PR itself.

@lukewarlow
Copy link
Member Author

Raised #507 to discuss the "slot" vs invalidation model. Aside from that do the changes to the IDL here make sense?

@annevk
Copy link
Member

annevk commented Apr 23, 2024

I think so.

@lukewarlow
Copy link
Member Author

I'm going to go ahead and get this merged, we'll get another chance when I upstream it to HTML to double check it all makes sense within the wider context of the full script enforcement.

@lukewarlow lukewarlow merged commit 5592e30 into w3c:main Apr 23, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Apr 23, 2024
SHA: 5592e30
Reason: push, by lukewarlow

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.

4 participants