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

Define securitypolicyviolation event #568

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

tidoust
Copy link
Member

@tidoust tidoust commented Oct 4, 2022

Following discussions in whatwg/html#8340, the events index in the HTML spec will be restricted to event types that gets fired within the HTML spec. The securitypolicyviolation event will be dropped from the index as a result.

This turns the mention of securitypolicyviolation into a proper event type definition so that other specs (including the HTML spec to map onsecuritypolicyviolation to securitypolicyviolation) can reference it.

This also defines the event targets more precisely. In HTML, the event was defined as firing on HTMLElement. In practice, if I read the spec correcty, the event fires on interfaces that derive from GlobalEventHandlers in general (HTMLElement, MathMLElement, SVGElement, Document), and on WorkerGlobalScope.

I note that the definition of WorkerGlobalScope should probably also be amended in HTML to add an onsecuritypolicyviolation IDL attribute, as was done for GlobalEventHandlers.

The definition of "policy" suggests that the event could in theory also fire on WorkletGlobalScope but that interface does not inherit from EventTarget. That seems to be adequately covered by the "Report a violation" algorithm. If firing at WorkletGlobalScope is also expected, more changes would be needed.

@antosart
Copy link
Member

antosart commented Oct 4, 2022

Note: rebasing on main should fix the failing check.

Copy link
Member

@antosart antosart left a comment

Choose a reason for hiding this comment

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

Thanks. This LGTM, let's wait for whatwg/html#8351 for merging.

@antosart
Copy link
Member

antosart commented Oct 4, 2022

All things above make sense, thanks for the detailed explanation. As for worklets, we don't want to fire events on them, see #549 and #545.

Following discussions in whatwg/html#8340, the events
index in the HTML spec will be restricted to event types that gets fired within
the HTML spec. The `securitypolicyviolation` event will be dropped from the
index as a result.

This turns the mention of `securitypolicyviolation` into a proper event type
definition so that other specs (including the HTML spec to map
`onsecuritypolicyviolation` to `securitypolicyviolation`) can reference it.

This also defines the event targets more precisely. In HTML, the event was
defined as firing on `HTMLElement`. In practice, if I read the spec correcty,
the event fires on interfaces that derive from `GlobalEventHandlers` in general
(`HTMLElement`, `MathMLElement`, `SVGElement`, `Document`), and on
`WorkerGlobalScope`.

I note that the definition of `WorkerGlobalScope` should probably also be
amended in HTML to add an `onsecuritypolicyviolation` IDL attribute, as was done
for `GlobalEventHandlers`.

The definition of "policy" suggests that the event could in theory also fire on
`WorkletGlobalScope` but that interface does not inherit from `EventTarget`.
That seems to be adequately covered by the "Report a violation" algorithm. If
firing at `WorkletGlobalScope` is also expected, more changes would be needed.
@tidoust tidoust force-pushed the event-securitypolicyviolation branch from 7c7a894 to 18c0935 Compare October 4, 2022 12:02
@tidoust
Copy link
Member Author

tidoust commented Oct 4, 2022

I just rebased the pull request. That fixes the CI failure.

Thanks. This LGTM, let's wait for whatwg/html#8351 for merging.

Actually, this PR should get merged before the one in HTML because the PR in HTML references the definition that this PR creates.

@antosart
Copy link
Member

antosart commented Oct 4, 2022

Actually, this PR should get merged before the one in HTML because the PR in HTML references the definition that this PR creates.

Makes sense, let's just wait for it to be ready then!

@dontcallmedom
Copy link
Member

For sake of clarity, I'll mark the HTML ready once this PR is merged (and its sister DOM PR) - in other words, this PR should not be seen as having dependency on anything happening in HTML at this stage.

@antosart
Copy link
Member

antosart commented Oct 4, 2022

Thanks for clarifying. I hadn't look in detail, the PR on html was marked as draft, so I assumed work just started there. Looking closer, it looks that PR is basically done, so fine with merging this.

@antosart antosart merged commit 886ac5d into w3c:main Oct 4, 2022
github-actions bot added a commit that referenced this pull request Oct 4, 2022
SHA: 886ac5d
Reason: push, by @antosart

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.

None yet

3 participants