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

Finalise spec mechanism for event handlers #520

Open
lukewarlow opened this issue Jun 12, 2024 · 9 comments
Open

Finalise spec mechanism for event handlers #520

lukewarlow opened this issue Jun 12, 2024 · 9 comments
Labels
Milestone

Comments

@lukewarlow
Copy link
Member

As of #457 the spec uses the HTML "event handler content attribute" concept. Anne's feedback was that that was ambiguous and we should instead generate a fixed list to check inside of. This issue tracks generating that or an alternative.

cc @koto as spec editor you might know how best to deal with this.

@lukewarlow lukewarlow added this to the v1 milestone Jun 12, 2024
@koto
Copy link
Member

koto commented Jun 28, 2024

@otherdaniel how does Sanitizer deal with this? From memory, the challenge was that there was no centralized and specced list of event handlers we could refer to.

@otherdaniel
Copy link
Member

@otherdaniel how does Sanitizer deal with this? From memory, the challenge was that there was no centralized and specced list of event handlers we could refer to.

Yes, there isn't a spec list of event handlers. Sanitizer has it a bit easier, since it's based on an allow-list: We (manually) write a list of allowed attributes, and we maintain a list of all attributes. Sanitizer doesn't have to decide why an attribute (or element) isn't in the allow-list, e.g. whether it's an event handler or not.

There was some discussion of adding some sort of spec-level 'tags' to any known element or attribute that could then form a basis for these lists; but this mechanism doesn't currently exist. Until it does, it'll be manually maintained lists.

@lukewarlow
Copy link
Member Author

I'm happy to spend some time and put together a list.

But from a spec maintainer and implementation point of view a block list of element event handler attributes seems far from ideal. Any time someone adds a new one they have to remember to adjust that list. Rather than current implementations which just use the IDL (which I'm realistically gonna use as the basis for my list). Especially tricky if the event handler is specified in a random wicg spec or something like that. Also if there's non standard events browsers have they should also be protected but won't be specced.

That being said I'm happy to defer to others on whether that's okay in practice.

@koto
Copy link
Member

koto commented Jun 28, 2024

So, rephrasing, the choice is to be either ambiguous and refer to other spec concepts (event handler content attribute), or explicit, and maintain a separate list (here, or in HTML?). I actually think the first choice is better, from normative perspective.

@otherdaniel
Copy link
Member

otherdaniel commented Jun 28, 2024

But from a spec maintainer and implementation point of view a block list of element event handler attributes seems far from ideal.

From an implementation point of view, there's two easy ways:

  • Check whether the attribute name [Edit: of a spec-defined attribute] starts with "on". HTML has been quite consistent with that.
  • All event handlers in WebIDL are of type EventHandler, OnBeforeUnloadEventHandler, or OnErrorEventHandler.

But I don't think this is how we can spec things.

@lukewarlow
Copy link
Member Author

check whether the attribute name starts with "on". HTML has been quite consistent with that

Unfortunately custom attributes would fall foul of that and apparently chrome's had issues with it.

@otherdaniel
Copy link
Member

check whether the attribute name starts with "on". HTML has been quite consistent with that

Unfortunately custom attributes would fall foul of that and apparently chrome's had issues with it.

What I meant is, whether it's a spec-defined attribute and starts with "on". Getting a list of attributes described in the spec and filtering for the "on" prefix gets you the list of event handler attributes, as they've been consistent with the naming.

@annevk
Copy link
Member

annevk commented Jul 1, 2024

"Specification-defined attribute" is also not a concept that exists though, currently.

@fred-wang
Copy link
Contributor

Skimming over existing WPT tests, it seems all the event handler attributes used in tests are coming from GlobalEventHandlers. Conversely, all event handler attributes from GlobalEventHandlers are covered by trusted-types-event-handlers.html (which tries all the names of a div's proto starting with 'on' of at least 3 characters)

Trying to implement getAttributeType in Firefox, it's not clear from the current text of the spec whether the attributes below should also be covered:

  • WindowEventHandlers from the on HTMLBodyElement and HTMLFrameSetElement, some of them also supported by the <svg> element.
  • onencrypted and onwaitingforkey on HTMLMediaElement from the Encrypted Media Extensions.
  • SMIL attributes used for SVG animations (namely onbegin, onend and onrepeat).

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 23, 2024
…tes.

The spec is not really clear, for now add a tentative test with the
cases detected while implementing the method in Firefox. For details,
see w3c/trusted-types#520

Differential Revision: https://phabricator.services.mozilla.com/D226442

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1917783
gecko-commit: 474afb5197ff041a421591d890dd339a2b23fe08
gecko-reviewers: smaug
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 23, 2024
…ontent attributes. r=smaug

The spec is not really clear, for now add a tentative test with the
cases detected while implementing the method in Firefox. For details,
see w3c/trusted-types#520

Differential Revision: https://phabricator.services.mozilla.com/D226442
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 23, 2024
…tes.

The spec is not really clear, for now add a tentative test with the
cases detected while implementing the method in Firefox. For details,
see w3c/trusted-types#520

Differential Revision: https://phabricator.services.mozilla.com/D226442

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1917783
gecko-commit: 474afb5197ff041a421591d890dd339a2b23fe08
gecko-reviewers: smaug
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 23, 2024
…tes.

The spec is not really clear, for now add a tentative test with the
cases detected while implementing the method in Firefox. For details,
see w3c/trusted-types#520

Differential Revision: https://phabricator.services.mozilla.com/D226442

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1917783
gecko-commit: 474afb5197ff041a421591d890dd339a2b23fe08
gecko-reviewers: smaug
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Oct 23, 2024
…ontent attributes. r=smaug

The spec is not really clear, for now add a tentative test with the
cases detected while implementing the method in Firefox. For details,
see w3c/trusted-types#520

Differential Revision: https://phabricator.services.mozilla.com/D226442
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Oct 24, 2024
…ontent attributes. r=smaug

The spec is not really clear, for now add a tentative test with the
cases detected while implementing the method in Firefox. For details,
see w3c/trusted-types#520

Differential Revision: https://phabricator.services.mozilla.com/D226442
lukewarlow added a commit to web-platform-tests/wpt that referenced this issue Dec 2, 2024
- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
lukewarlow added a commit to web-platform-tests/wpt that referenced this issue Dec 2, 2024
- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 5, 2024
…test, a=testonly

Automatic update from web-platform-tests
Update Trusted Types content attributes test (#49473)

- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
--

wpt-commits: 59d1f701fbfaff1b24accfb35b8db182cd19077c
wpt-pr: 49473
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Dec 6, 2024
…test, a=testonly

Automatic update from web-platform-tests
Update Trusted Types content attributes test (#49473)

- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
--

wpt-commits: 59d1f701fbfaff1b24accfb35b8db182cd19077c
wpt-pr: 49473
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Dec 13, 2024
…test, a=testonly

Automatic update from web-platform-tests
Update Trusted Types content attributes test (#49473)

- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
--

wpt-commits: 59d1f701fbfaff1b24accfb35b8db182cd19077c
wpt-pr: 49473

UltraBlame original commit: 4ab121f6dc236438b12ff96939229befac539038
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Dec 13, 2024
…test, a=testonly

Automatic update from web-platform-tests
Update Trusted Types content attributes test (#49473)

- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
--

wpt-commits: 59d1f701fbfaff1b24accfb35b8db182cd19077c
wpt-pr: 49473

UltraBlame original commit: 4ab121f6dc236438b12ff96939229befac539038
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Dec 13, 2024
…test, a=testonly

Automatic update from web-platform-tests
Update Trusted Types content attributes test (#49473)

- Skip attributes that are in WPT IDL but are not implemented in browser IDL.

- Add comment explaining why each set of attributes are included.

encrypted-media event listeners are an interesting case, they're only a TT sink in Firefox currently but as specced they should be in all browsers, so I've kept the tests for them.

See w3c/trusted-types#520 for further discussion on the spec.
--

wpt-commits: 59d1f701fbfaff1b24accfb35b8db182cd19077c
wpt-pr: 49473

UltraBlame original commit: 4ab121f6dc236438b12ff96939229befac539038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants