-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[Fix] : Added global event listeners #3506
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3506 +/- ##
=======================================
Coverage 97.57% 97.58%
=======================================
Files 130 130
Lines 9211 9216 +5
Branches 3341 3343 +2
=======================================
+ Hits 8988 8993 +5
Misses 223 223
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I don't think this is a viable approach. For example, A better change would be allowing |
// Media events allowed only on audio and video tags, see https://github.com/facebook/react/blob/256aefbea1449869620fb26f6ec695536ab453f5/CHANGELOG.md#notable-enhancements | ||
onAbort: ['audio', 'video'], | ||
onCancel: ['dialog'], | ||
onCanPlay: ['audio', 'video'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eg, onCanPlay
isn't a defensible attribute on an element that doesn't play things
// Note: Most events can be added to any HTML element, even if not specified in the standard | ||
// see https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think our standard should be higher than "whatever wacky things HTML allows" in a linter whose goal is to prevent bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fair. I can update it to follow a more sensible philosophy. I do agree that the HTML spec is overly permissive with these.
Though since it is allowed, I can use a different message in these cases, like "While this is allowed in HTML, this is probably not what you want".
Personally I think this case would ideally be a whole different rule, so that users can do things like set "accepted but probably not desired" props to be warnings and truly disallowed props to be shown as errors, but I don't feel strongly enough to create one at this point.
Happy to reopen if there's a viable path forward here. |
Fixes #3505.
Added some attributes that should be able to be used on any element, along with a way to exclude only specific elements for some properties (such as
onFocus
on a<body>
element).For more info on these global events, see the "event handler" section in the MDN documentation on global attributes, and the HTML specification on global event handlers here.