Skip to content

Fixed #242025 - Added nonce #503

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

Merged
merged 5 commits into from
Jun 19, 2018
Merged

Conversation

irrationalRock
Copy link
Contributor

This PR addresses the following issue: microsoft/TypeScript#24205.

It adds nonce to both HTMLScriptElement and HTMLStyleElement.

Thanks for considering this PR.

@HolgerJeromin
Copy link
Contributor

Shouldn't this change go to addingTypes.json?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2018

@saschanaz
Copy link
Contributor

@mhegazy #496 already did that. nonce is part of https://html.spec.whatwg.org/multipage/dom.html, but we probably can't add this right now because GlobalEventHandlers mixin should be processed first. Otherwise we'll lose lots of event handlers.

@saschanaz
Copy link
Contributor

saschanaz commented Jun 14, 2018

Also, nonce should be added to HTMLElement (Edit: HTMLOrSVGElement mixin) instead of HTMLScriptElement.

@irrationalRock
Copy link
Contributor Author

Thanks, I appreciate the responses.
I moved nonce to HTMLOrSVGElement and FocusOptions.

@saschanaz
Copy link
Contributor

HTMLOrSVGElement should also be added to implements list of HTMLElement.

FocusOptions should rather be added by https://html.spec.whatwg.org/multipage/interaction.html and also focus() signature should be overridden.

@irrationalRock
Copy link
Contributor Author

Thanks for the feedback, I added HTMLOrSVGElement to HTMLElement's list.
However, this causes HTMLElementMap to lose it's extends of ElementEventMap.
Also, I overridden the focus() signature.
@saschanaz I'm wondering if you can give more information on what you mean by FocusOptions should be added by https://html.spec.whatwg.org/multipage/interaction.html.

@saschanaz
Copy link
Contributor

@irrationalRock extends shouldn't be overridden here, doing so will break EventMap generation which uses the field.

For FocusOptions, you can add this to inputfiles/idlSources.json:

{
  "url": "https://html.spec.whatwg.org/multipage/interaction.html",
  "title": "HTML - User interaction"
}

...and then run npm run fetch && npm run build && npm run baseline-accept. It will automatically import FocusOptions from the spec document.

@irrationalRock
Copy link
Contributor Author

I added FocusOptions with inputfiles/idlSources.json.

If extends shouldn't be used here, what would be the best place for it?

@saschanaz
Copy link
Contributor

saschanaz commented Jun 18, 2018

implements should be overridden but I'm afraid it won't work currently, as the merge function doesn't support arrays very well. Perhaps we may add additional-implements like the current additional-signatures.

@saschanaz
Copy link
Contributor

saschanaz commented Jun 18, 2018

Oops, I was wrong. Just adding implements: ["HTMLOrSVGElement"] will append to the current list.

"focus": {
"name": "focus",
"override-signatures": [
"focus(options?: FocusOptions): void"
Copy link
Contributor

Choose a reason for hiding this comment

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

so why is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mhegazy mhegazy merged commit ed07bae into microsoft:master Jun 19, 2018
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