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

Problem with SecurityPolicyViolationEvent constructor and optional init dict #631

Closed
evilpie opened this issue Dec 13, 2023 · 9 comments · Fixed by #645
Closed

Problem with SecurityPolicyViolationEvent constructor and optional init dict #631

evilpie opened this issue Dec 13, 2023 · 9 comments · Fixed by #645
Assignees

Comments

@evilpie
Copy link
Contributor

evilpie commented Dec 13, 2023

See whatwg/webidl#1378 for a description of this issue.

TLDR: Having an optional eventInitDict parameter in the SecurityPolicyViolationEvent constructor doesn't really make sense, considering that dictonary SecurityPolicyViolationEventInit has required properties.

After adjusting Gecko to follow the spec to the letter, new SecurityPolicyViolationEvent("securitypolicyviolation") fails, which currently works on all browsers and is tested by https://wpt.fyi/results/content-security-policy/securitypolicyviolation/idlharness.window.html.

This worked in Gecko before, because we didn't make any of the properties required and seemingly in Chrome because it has a second non-standard constructor with just one argument.

Either all browsers should align with spec or the spec needs to be updated to match.

@mikewest
Copy link
Member

I don't have much of an opinion here, and I expect the constructor's usage is low enough to allow ~any approach. What outcome would you prefer?

@annevk
Copy link
Member

annevk commented Dec 14, 2023

I'd suggest we drop required. Throwing less exceptions when it doesn't matter is pretty much always better. And it seems implementations already have a way of dealing with it when it's not supplied which we could reuse in the specification.

@mikewest
Copy link
Member

I'm fine with dropping it. I think that means we should simply define default values for those required items. Chromium ends up with empty strings for the USVString and DOMString items, enforce for the disposition, and 0 for the unsigned short. Is that acceptable to y'all?

@annevk
Copy link
Member

annevk commented Dec 14, 2023

Seems fine to me.

@annevk
Copy link
Member

annevk commented Dec 14, 2023

And createEvent() does not work for this class so you can indeed rely on defaulting I think.

@mikewest
Copy link
Member

@SaeidEid wanted to grab this as well, and it should be a very straightforward change to the spec and test.

@evilpie
Copy link
Contributor Author

evilpie commented Dec 14, 2023

I'm fine with dropping it. I think that means we should simply define default values for those required items. Chromium ends up with empty strings for the USVString and DOMString items, enforce for the disposition, and 0 for the unsigned short. Is that acceptable to y'all?

Sounds good to me.
This largely matches Firefox's WebIDL. We do use report instead of enforce, but I am not concerned about that.

@SaeidEid
Copy link
Collaborator

I create an issue for this in chromium: https://issues.chromium.org/issues/325291983

@antosart
Copy link
Member

Just wanted to notice that Gecko seems to set report as default value for the disposition, see https://wpt.fyi/results/content-security-policy/securitypolicyviolation/constructor-required-fields.html?diff&filter=ADC&run_id=5069213999300608&run_id=5069891194847232.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants